Merge "Revert "Revert "Fix wp and sp comparison bugs""" (bca89d7b) · Commits · e / os / android_system_core

libutils/RefBase_test.cpp

+150 −0

Original line number	Diff line number	Diff line
		@@ -45,6 +45,44 @@ private:
		bool* mDeleted;
		};

		// A version of Foo that ensures that all objects are allocated at the same
		// address. No more than one can be allocated at a time. Thread-hostile.
		class FooFixedAlloc : public RefBase {
		public:
		static void* operator new(size_t size) {
		if (mAllocCount != 0) {
		abort();
		}
		mAllocCount = 1;
		if (theMemory == nullptr) {
		theMemory = malloc(size);
		}
		return theMemory;
		}

		static void operator delete(void *p) {
		if (mAllocCount != 1 \|\| p != theMemory) {
		abort();
		}
		mAllocCount = 0;
		}

		FooFixedAlloc(bool* deleted_check) : mDeleted(deleted_check) {
		*mDeleted = false;
		}

		~FooFixedAlloc() {
		*mDeleted = true;
		}
		private:
		bool* mDeleted;
		static int mAllocCount;
		static void* theMemory;
		};

		int FooFixedAlloc::mAllocCount(0);
		void* FooFixedAlloc::theMemory(nullptr);

		TEST(RefBase, StrongMoves) {
		bool isDeleted;
		Foo* foo = new Foo(&isDeleted);
		@@ -90,6 +128,118 @@ TEST(RefBase, WeakCopies) {
		ASSERT_FALSE(isDeleted) << "Deletion on wp destruction should no longer occur";
		}

		TEST(RefBase, Comparisons) {
		bool isDeleted, isDeleted2, isDeleted3;
		Foo* foo = new Foo(&isDeleted);
		Foo* foo2 = new Foo(&isDeleted2);
		sp<Foo> sp1(foo);
		sp<Foo> sp2(foo2);
		wp<Foo> wp1(sp1);
		wp<Foo> wp2(sp1);
		wp<Foo> wp3(sp2);
		ASSERT_TRUE(wp1 == wp2);
		ASSERT_TRUE(wp1 == sp1);
		ASSERT_TRUE(wp3 == sp2);
		ASSERT_TRUE(wp1 != sp2);
		ASSERT_TRUE(wp1 <= wp2);
		ASSERT_TRUE(wp1 >= wp2);
		ASSERT_FALSE(wp1 != wp2);
		ASSERT_FALSE(wp1 > wp2);
		ASSERT_FALSE(wp1 < wp2);
		ASSERT_FALSE(sp1 == sp2);
		ASSERT_TRUE(sp1 != sp2);
		bool sp1_smaller = sp1 < sp2;
		wp<Foo>wp_smaller = sp1_smaller ? wp1 : wp3;
		wp<Foo>wp_larger = sp1_smaller ? wp3 : wp1;
		ASSERT_TRUE(wp_smaller < wp_larger);
		ASSERT_TRUE(wp_smaller != wp_larger);
		ASSERT_TRUE(wp_smaller <= wp_larger);
		ASSERT_FALSE(wp_smaller == wp_larger);
		ASSERT_FALSE(wp_smaller > wp_larger);
		ASSERT_FALSE(wp_smaller >= wp_larger);
		sp2 = nullptr;
		ASSERT_TRUE(isDeleted2);
		ASSERT_FALSE(isDeleted);
		ASSERT_FALSE(wp3 == sp2);
		// Comparison results on weak pointers should not be affected.
		ASSERT_TRUE(wp_smaller < wp_larger);
		ASSERT_TRUE(wp_smaller != wp_larger);
		ASSERT_TRUE(wp_smaller <= wp_larger);
		ASSERT_FALSE(wp_smaller == wp_larger);
		ASSERT_FALSE(wp_smaller > wp_larger);
		ASSERT_FALSE(wp_smaller >= wp_larger);
		wp2 = nullptr;
		ASSERT_FALSE(wp1 == wp2);
		ASSERT_TRUE(wp1 != wp2);
		wp1.clear();
		ASSERT_TRUE(wp1 == wp2);
		ASSERT_FALSE(wp1 != wp2);
		wp3.clear();
		ASSERT_TRUE(wp1 == wp3);
		ASSERT_FALSE(wp1 != wp3);
		ASSERT_FALSE(isDeleted);
		sp1.clear();
		ASSERT_TRUE(isDeleted);
		ASSERT_TRUE(sp1 == sp2);
		// Try to check that null pointers are properly initialized.
		{
		// Try once with non-null, to maximize chances of getting junk on the
		// stack.
		sp<Foo> sp3(new Foo(&isDeleted3));
		wp<Foo> wp4(sp3);
		wp<Foo> wp5;
		ASSERT_FALSE(wp4 == wp5);
		ASSERT_TRUE(wp4 != wp5);
		ASSERT_FALSE(sp3 == wp5);
		ASSERT_FALSE(wp5 == sp3);
		ASSERT_TRUE(sp3 != wp5);
		ASSERT_TRUE(wp5 != sp3);
		ASSERT_TRUE(sp3 == wp4);
		}
		{
		sp<Foo> sp3;
		wp<Foo> wp4(sp3);
		wp<Foo> wp5;
		ASSERT_TRUE(wp4 == wp5);
		ASSERT_FALSE(wp4 != wp5);
		ASSERT_TRUE(sp3 == wp5);
		ASSERT_TRUE(wp5 == sp3);
		ASSERT_FALSE(sp3 != wp5);
		ASSERT_FALSE(wp5 != sp3);
		ASSERT_TRUE(sp3 == wp4);
		}
		}

		// Check whether comparison against dead wp works, even if the object referenced
		// by the new wp happens to be at the same address.
		TEST(RefBase, ReplacedComparison) {
		bool isDeleted, isDeleted2;
		FooFixedAlloc* foo = new FooFixedAlloc(&isDeleted);
		sp<FooFixedAlloc> sp1(foo);
		wp<FooFixedAlloc> wp1(sp1);
		ASSERT_TRUE(wp1 == sp1);
		sp1.clear(); // Deallocates the object.
		ASSERT_TRUE(isDeleted);
		FooFixedAlloc* foo2 = new FooFixedAlloc(&isDeleted2);
		ASSERT_FALSE(isDeleted2);
		ASSERT_EQ(foo, foo2); // Not technically a legal comparison, but ...
		sp<FooFixedAlloc> sp2(foo2);
		wp<FooFixedAlloc> wp2(sp2);
		ASSERT_TRUE(sp2 == wp2);
		ASSERT_FALSE(sp2 != wp2);
		ASSERT_TRUE(sp2 != wp1);
		ASSERT_FALSE(sp2 == wp1);
		ASSERT_FALSE(sp2 == sp1); // sp1 is null.
		ASSERT_FALSE(wp1 == wp2); // wp1 refers to old object.
		ASSERT_TRUE(wp1 != wp2);
		ASSERT_TRUE(wp1 > wp2 \|\| wp1 < wp2);
		ASSERT_TRUE(wp1 >= wp2 \|\| wp1 <= wp2);
		ASSERT_FALSE(wp1 >= wp2 && wp1 <= wp2);
		ASSERT_FALSE(wp1 == nullptr);
		wp1 = sp2;
		ASSERT_TRUE(wp1 == wp2);
		ASSERT_FALSE(wp1 != wp2);
		}

		// Set up a situation in which we race with visit2AndRremove() to delete
		// 2 strong references. Bar destructor checks that there are no early

libutils/include/utils/RefBase.h

+73 −37

Original line number	Diff line number	Diff line
		@@ -171,6 +171,8 @@
		#define ANDROID_REF_BASE_H

		#include <atomic>
		#include <functional>
		#include <type_traits> // for common_type.

		#include <stdint.h>
		#include <sys/types.h>
		@@ -192,19 +194,26 @@ TextOutput& printWeakPointer(TextOutput& to, const void* val);
		// ---------------------------------------------------------------------------

		#define COMPARE_WEAK(_op_) \
		inline bool operator _op_ (const sp<T>& o) const { \
		return m_ptr _op_ o.m_ptr; \
		template<typename U> \
		inline bool operator _op_ (const U* o) const { \
		return m_ptr _op_ o; \
		} \
		/* Needed to handle type inference for nullptr: */ \
		inline bool operator _op_ (const T* o) const { \
		return m_ptr _op_ o; \
		} \
		template<typename U> \
		inline bool operator _op_ (const sp<U>& o) const { \
		return m_ptr _op_ o.m_ptr; \
		} \
		}

		template<template<typename C> class comparator, typename T, typename U>
		static inline bool _wp_compare_(T* a, U* b) {
		return comparator<typename std::common_type<T, U>::type>()(a, b);
		}

		// Use std::less and friends to avoid undefined behavior when ordering pointers
		// to different objects.
		#define COMPARE_WEAK_FUNCTIONAL(_op_, _compare_) \
		template<typename U> \
		inline bool operator _op_ (const U* o) const { \
		return m_ptr _op_ o; \
		return _wp_compare_<_compare_>(m_ptr, o); \
		}

		// ---------------------------------------------------------------------------
		@@ -354,7 +363,7 @@ class wp
		public:
		typedef typename RefBase::weakref_type weakref_type;

		inline wp() : m_ptr(nullptr) { }
		inline wp() : m_ptr(nullptr), m_refs(nullptr) { }

		wp(T* other); // NOLINT(implicit)
		wp(const wp<T>& other);
		@@ -395,39 +404,51 @@ public:

		COMPARE_WEAK(==)
		COMPARE_WEAK(!=)
		COMPARE_WEAK(>)
		COMPARE_WEAK(<)
		COMPARE_WEAK(<=)
		COMPARE_WEAK(>=)
		COMPARE_WEAK_FUNCTIONAL(>, std::greater)
		COMPARE_WEAK_FUNCTIONAL(<, std::less)
		COMPARE_WEAK_FUNCTIONAL(<=, std::less_equal)
		COMPARE_WEAK_FUNCTIONAL(>=, std::greater_equal)

		inline bool operator == (const wp<T>& o) const {
		return (m_ptr == o.m_ptr) && (m_refs == o.m_refs);
		}
		template<typename U>
		inline bool operator == (const wp<U>& o) const {
		return m_ptr == o.m_ptr;
		return m_refs == o.m_refs; // Implies m_ptr == o.mptr; see invariants below.
		}

		template<typename U>
		inline bool operator == (const sp<U>& o) const {
		// Just comparing m_ptr fields is often dangerous, since wp<> may refer to an older
		// object at the same address.
		if (o == nullptr) {
		return m_ptr == nullptr;
		} else {
		return m_refs == o->getWeakRefs(); // Implies m_ptr == o.mptr.
		}
		}

		inline bool operator > (const wp<T>& o) const {
		return (m_ptr == o.m_ptr) ? (m_refs > o.m_refs) : (m_ptr > o.m_ptr);
		template<typename U>
		inline bool operator != (const sp<U>& o) const {
		return !(*this == o);
		}

		template<typename U>
		inline bool operator > (const wp<U>& o) const {
		return (m_ptr == o.m_ptr) ? (m_refs > o.m_refs) : (m_ptr > o.m_ptr);
		if (m_ptr == o.m_ptr) {
		return _wp_compare_<std::greater>(m_refs, o.m_refs);
		} else {
		return _wp_compare_<std::greater>(m_ptr, o.m_ptr);
		}

		inline bool operator < (const wp<T>& o) const {
		return (m_ptr == o.m_ptr) ? (m_refs < o.m_refs) : (m_ptr < o.m_ptr);
		}

		template<typename U>
		inline bool operator < (const wp<U>& o) const {
		return (m_ptr == o.m_ptr) ? (m_refs < o.m_refs) : (m_ptr < o.m_ptr);
		if (m_ptr == o.m_ptr) {
		return _wp_compare_<std::less>(m_refs, o.m_refs);
		} else {
		return _wp_compare_<std::less>(m_ptr, o.m_ptr);
		}
		}
		inline bool operator != (const wp<T>& o) const { return m_refs != o.m_refs; }
		template<typename U> inline bool operator != (const wp<U>& o) const { return !operator == (o); }
		inline bool operator <= (const wp<T>& o) const { return !operator > (o); }
		template<typename U> inline bool operator <= (const wp<U>& o) const { return !operator > (o); }
		inline bool operator >= (const wp<T>& o) const { return !operator < (o); }
		template<typename U> inline bool operator >= (const wp<U>& o) const { return !operator < (o); }

		private:
		@@ -446,11 +467,27 @@ TextOutput& operator<<(TextOutput& to, const wp<T>& val);
		// ---------------------------------------------------------------------------
		// No user serviceable parts below here.

		// Implementation invariants:
		// Either
		// 1) m_ptr and m_refs are both null, or
		// 2) m_refs == m_ptr->mRefs, or
		// 3) *m_ptr is no longer live, and m_refs points to the weakref_type object that corresponded
		// to m_ptr while it was live. *m_refs remains live while a wp<> refers to it.
		//
		// The m_refs field in a RefBase object is allocated on construction, unique to that RefBase
		// object, and never changes. Thus if two wp's have identical m_refs fields, they are either both
		// null or point to the same object. If two wp's have identical m_ptr fields, they either both
		// point to the same live object and thus have the same m_ref fields, or at least one of the
		// objects is no longer live.
		//
		// Note that the above comparison operations go out of their way to provide an ordering consistent
		// with ordinary pointer comparison; otherwise they could ignore m_ptr, and just compare m_refs.

		template<typename T>
		wp<T>::wp(T* other)
		: m_ptr(other)
		{
		if (other) m_refs = other->createWeak(this);
		m_refs = other ? m_refs = other->createWeak(this) : nullptr;
		}

		template<typename T>
		@@ -464,16 +501,14 @@ template<typename T>
		wp<T>::wp(const sp<T>& other)
		: m_ptr(other.m_ptr)
		{
		if (m_ptr) {
		m_refs = m_ptr->createWeak(this);
		}
		m_refs = m_ptr ? m_ptr->createWeak(this) : nullptr;
		}

		template<typename T> template<typename U>
		wp<T>::wp(U* other)
		: m_ptr(other)
		{
		if (other) m_refs = other->createWeak(this);
		m_refs = other ? other->createWeak(this) : nullptr;
		}

		template<typename T> template<typename U>
		@@ -483,6 +518,8 @@ wp<T>::wp(const wp<U>& other)
		if (m_ptr) {
		m_refs = other.m_refs;
		m_refs->incWeak(this);
		} else {
		m_refs = nullptr;
		}
		}

		@@ -490,9 +527,7 @@ template<typename T> template<typename U>
		wp<T>::wp(const sp<U>& other)
		: m_ptr(other.m_ptr)
		{
		if (m_ptr) {
		m_refs = m_ptr->createWeak(this);
		}
		m_refs = m_ptr ? m_ptr->createWeak(this) : nullptr;
		}

		template<typename T>
		@@ -595,6 +630,7 @@ void wp<T>::clear()
		{
		if (m_ptr) {
		m_refs->decWeak(this);
		m_refs = 0;
		m_ptr = 0;
		}
		}

libutils/include/utils/StrongPointer.h

+45 −19

Original line number	Diff line number	Diff line
		@@ -17,6 +17,9 @@
		#ifndef ANDROID_STRONG_POINTER_H
		#define ANDROID_STRONG_POINTER_H

		#include <functional>
		#include <type_traits> // for common_type.

		// ---------------------------------------------------------------------------
		namespace android {

		@@ -24,13 +27,12 @@ template<typename T> class wp;

		// ---------------------------------------------------------------------------

		#define COMPARE(_op_) \
		inline bool operator _op_ (const sp<T>& o) const { \
		return m_ptr _op_ o.m_ptr; \
		} \
		inline bool operator _op_ (const T* o) const { \
		return m_ptr _op_ o; \
		} \
		// TODO: Maybe remove sp<> ? wp<> comparison? These are dangerous: If the wp<>
		// was created before the sp<>, and they point to different objects, they may
		// compare equal even if they are entirely unrelated. E.g. CameraService
		// currently performa such comparisons.

		#define COMPARE_STRONG(_op_) \
		template<typename U> \
		inline bool operator _op_ (const sp<U>& o) const { \
		return m_ptr _op_ o.m_ptr; \
		@@ -39,14 +41,27 @@ template<typename U> \
		inline bool operator _op_ (const U* o) const { \
		return m_ptr _op_ o; \
		} \
		inline bool operator _op_ (const wp<T>& o) const { \
		return m_ptr _op_ o.m_ptr; \
		/* Needed to handle type inference for nullptr: */ \
		inline bool operator _op_ (const T* o) const { \
		return m_ptr _op_ o; \
		}

		template<template<typename C> class comparator, typename T, typename U>
		static inline bool _sp_compare_(T* a, U* b) {
		return comparator<typename std::common_type<T, U>::type>()(a, b);
		}

		// Use std::less and friends to avoid undefined behavior when ordering pointers
		// to different objects.
		#define COMPARE_STRONG_FUNCTIONAL(_op_, _compare_) \
		template<typename U> \
		inline bool operator _op_ (const sp<U>& o) const { \
		return _sp_compare_<_compare_>(m_ptr, o.m_ptr); \
		} \
		template<typename U> \
		inline bool operator _op_ (const wp<U>& o) const { \
		return m_ptr _op_ o.m_ptr; \
		inline bool operator _op_ (const U* o) const { \
		return _sp_compare_<_compare_>(m_ptr, o); \
		}

		// ---------------------------------------------------------------------------

		template<typename T>
		@@ -89,12 +104,23 @@ public:

		// Operators

		COMPARE(==)
		COMPARE(!=)
		COMPARE(>)
		COMPARE(<)
		COMPARE(<=)
		COMPARE(>=)
		COMPARE_STRONG(==)
		COMPARE_STRONG(!=)
		COMPARE_STRONG_FUNCTIONAL(>, std::greater)
		COMPARE_STRONG_FUNCTIONAL(<, std::less)
		COMPARE_STRONG_FUNCTIONAL(<=, std::less_equal)
		COMPARE_STRONG_FUNCTIONAL(>=, std::greater_equal)

		// Punt these to the wp<> implementation.
		template<typename U>
		inline bool operator == (const wp<U>& o) const {
		return o == *this;
		}

		template<typename U>
		inline bool operator != (const wp<U>& o) const {
		return o != *this;
		}

		private:
		template<typename Y> friend class sp;