tequilaOS/platform_system_core
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Revert "Revert "Fix wp and sp comparison bugs""

Browse source 
Fix wp and sp comparison bugs

Make clear() actually clear wp m_refs, so that nulls compare equal.

Make equality consistent with < and >, ensuring that a weak pointer
cannot be both equal to and greater than another.

Don't rely on the built-in < and > operators to correctly order
different objects. The standard does not guarantee that, and there is
a risk of compiler relying on that lack of guarantee.

Remove unnecessary comparison overloads, especially those
comparing a wp<> to an sp<>.

Change the remaining wp<> to sp<> comparisons to check for equivalence
of the mRefs pointer instead of the object address, thus eliminating
the dubious equal comparison result for a dead wp<> and an sp<> that
happen to point to the same object address.

Add comparison tests.

This reverts commit a2a2ad8057.

The original code, and my original CL, both failed to initialize m_refs
in various wp<> constructors. This now became more important, since
comparisons now rely more on m_refs. However I believe it was always
a bug, since some comparisons always relied on m_refs.

Test: Treehugger, boot AOSP, atest RefBase
Bug: 126922090
This reverts commit a2a2ad8057.

Reason for revert: Reapply after constructor fixes.

Change-Id: I2c8917416a2306e36d2b6bb7b397f653020e5688
This commit is contained in:
Hans Boehm 2019-03-13 17:34:46 +00:00
parent e61e8c6b62
commit 6e75ad6e13
3 changed files with 268 additions and 56 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

150
libutils/RefBase_test.cpp
View file

@ -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

110
libutils/include/utils/RefBase.h
View file

@ -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; \
} \
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<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<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 _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.
}
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 {
// 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.
}
}
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;
}
}

64
libutils/include/utils/StrongPointer.h
View file

@ -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; \
} \
template<typename U> \
inline bool operator _op_ (const wp<U>& 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 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;