Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit a2a2ad80 authored by Hans Boehm's avatar Hans Boehm
Browse files

Revert "Fix wp and sp comparison bugs" 

This reverts commit 029b12eb.

Reason for revert: There appear to be problems with null comparisons. Reported failure in HwcBufferCacheTest.

Change-Id: I19745bb281dabe8b05c2df3fe95e7be7a49dcd51
parent 029b12eb

libutils/RefBase_test.cpp

+0 −123
Original line number Diff line number Diff line
@@ -45,44 +45,6 @@ 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);
@@ -128,91 +90,6 @@ TEST(RefBase, WeakCopies) { 
    ASSERT_FALSE(isDeleted) << "Deletion on wp destruction should no longer occur";

}



TEST(RefBase, Comparisons) {

    bool isDeleted, isDeleted2;

    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);

}



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

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



#include <atomic>

#include <functional>

#include <type_traits>  // for common_type.



#include <stdint.h>

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



#define COMPARE_WEAK(_op_)                                      \

template<typename U>                                            \

inline bool operator _op_ (const U* o) const {                  \

    return m_ptr _op_ o;                                        \

inline bool operator _op_ (const sp<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 _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 sp<U>& o) const {              \

    return m_ptr _op_ o.m_ptr;                                  \

}                                                               \

template<typename U>                                            \

inline bool operator _op_ (const U* o) const {                  \

    return _wp_compare_<_compare_>(m_ptr, o);                    \

    return m_ptr _op_ o;                                        \

}



// ---------------------------------------------------------------------------
@@ -404,51 +395,39 @@ public: 


    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)

    COMPARE_WEAK(>)

    COMPARE_WEAK(<)

    COMPARE_WEAK(<=)

    COMPARE_WEAK(>=)



    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_refs == o.m_refs;  // Implies m_ptr == o.mptr; see invariants below.

        return 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.

        }

    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 {

        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);

        }

        return (m_ptr == o.m_ptr) ? (m_refs > o.m_refs) : (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 {

        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);

        }

        return (m_ptr == o.m_ptr) ? (m_refs < o.m_refs) : (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:
@@ -467,22 +446,6 @@ 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)
@@ -632,7 +595,6 @@ void wp<T>::clear() 
{

    if (m_ptr) {

        m_refs->decWeak(this);

        m_refs = 0;

        m_ptr = 0;

    }

}

libutils/include/utils/StrongPointer.h

+19 −45
Original line number Diff line number Diff line
@@ -17,9 +17,6 @@ 
#ifndef ANDROID_STRONG_POINTER_H

#define ANDROID_STRONG_POINTER_H



#include <functional>

#include <type_traits>  // for common_type.



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

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


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



// 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_)                                           \

#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;                                        \

}                                                               \

template<typename U>                                            \

inline bool operator _op_ (const sp<U>& o) const {              \

    return m_ptr _op_ o.m_ptr;                                  \
@@ -41,27 +39,14 @@ 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 _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);              \

inline bool operator _op_ (const wp<T>& o) const {              \

    return m_ptr _op_ o.m_ptr;                                  \

}                                                               \

template<typename U>                                            \

inline bool operator _op_ (const U* o) const {                   \

    return _sp_compare_<_compare_>(m_ptr, o);                    \

inline bool operator _op_ (const wp<U>& o) const {              \

    return m_ptr _op_ o.m_ptr;                                  \

}



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



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


    // Operators



    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;

    }

    COMPARE(==)

    COMPARE(!=)

    COMPARE(>)

    COMPARE(<)

    COMPARE(<=)

    COMPARE(>=)



private:    

    template<typename Y> friend class sp;