From f14a1046e7242222300bbe88d530c3b531fc7678 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Wed, 16 Feb 2011 20:23:43 -0800 Subject: [PATCH] Fix some issues with RefBase debugging. First slipt sp<> out of RefBase into StrongPointer.h so it can be reused more easily and to make it clear that it doesn't require RefBase. Note: the rest of the change only affects the system when DEBUG_REFS is enabled. The main problem we fix here is that the owner id associated with each reference could get out of date when a sp<> or wp<> was moved, for instance when they're used in a Vector< >. We fix this issue by calling into RefBase::moveReferences from a template specialization for sp and wp of the type helpers. RefBase::moveReferences() has then a chance to update the owner ids. There is a little bit of trickery to implement this generically in RefBase, where we need to use a templatized functor that can turn a sp* casted to a void* into a RefBase*. Introduced a new debug option DEBUG_REFS_FATAL_SANITY_CHECKS currently set to 0 by default as there seem to be an issue with sp which trips the sanity checks. Change-Id: I4825b21c8ec47d4a0ef35d760760ae0c9cdfbd7f --- include/utils/RefBase.h | 277 +++++++++++++--------------------- include/utils/StrongPointer.h | 224 +++++++++++++++++++++++++++ include/utils/TypeHelpers.h | 13 -- libs/utils/RefBase.cpp | 171 ++++++++++++--------- 4 files changed, 426 insertions(+), 259 deletions(-) create mode 100644 include/utils/StrongPointer.h diff --git a/include/utils/RefBase.h b/include/utils/RefBase.h index f8d96cf1e..9b0e7d8a4 100644 --- a/include/utils/RefBase.h +++ b/include/utils/RefBase.h @@ -22,16 +22,16 @@ #include #include #include +#include + +#include // --------------------------------------------------------------------------- namespace android { class TextOutput; -TextOutput& printStrongPointer(TextOutput& to, const void* val); TextOutput& printWeakPointer(TextOutput& to, const void* val); -template class wp; - // --------------------------------------------------------------------------- #define COMPARE_WEAK(_op_) \ @@ -50,15 +50,15 @@ inline bool operator _op_ (const U* o) const { \ return m_ptr _op_ o; \ } -#define COMPARE(_op_) \ -COMPARE_WEAK(_op_) \ -inline bool operator _op_ (const wp& o) const { \ - return m_ptr _op_ o.m_ptr; \ -} \ -template \ -inline bool operator _op_ (const wp& o) const { \ - return m_ptr _op_ o.m_ptr; \ -} +// --------------------------------------------------------------------------- + +class ReferenceMover; +class ReferenceConverterBase { +public: + virtual size_t getReferenceTypeSize() const = 0; + virtual void* getReferenceBase(void const*) const = 0; + inline virtual ~ReferenceConverterBase() { } +}; // --------------------------------------------------------------------------- @@ -115,6 +115,8 @@ public: getWeakRefs()->trackMe(enable, retain); } + typedef RefBase basetype; + protected: RefBase(); virtual ~RefBase(); @@ -137,6 +139,11 @@ protected: virtual bool onIncStrongAttempted(uint32_t flags, const void* id); virtual void onLastWeakRef(const void* id); +private: + friend class ReferenceMover; + static void moveReferences(void* d, void const* s, size_t n, + const ReferenceConverterBase& caster); + private: friend class weakref_type; class weakref_impl; @@ -166,76 +173,23 @@ public: inline int32_t getStrongCount() const { return mCount; } - + + typedef LightRefBase basetype; + protected: inline ~LightRefBase() { } - + +private: + friend class ReferenceMover; + inline static void moveReferences(void* d, void const* s, size_t n, + const ReferenceConverterBase& caster) { } + private: mutable volatile int32_t mCount; }; // --------------------------------------------------------------------------- -template -class sp -{ -public: - typedef typename RefBase::weakref_type weakref_type; - - inline sp() : m_ptr(0) { } - - sp(T* other); - sp(const sp& other); - template sp(U* other); - template sp(const sp& other); - - ~sp(); - - // Assignment - - sp& operator = (T* other); - sp& operator = (const sp& other); - - template sp& operator = (const sp& other); - template sp& operator = (U* other); - - //! Special optimization for use by ProcessState (and nobody else). - void force_set(T* other); - - // Reset - - void clear(); - - // Accessors - - inline T& operator* () const { return *m_ptr; } - inline T* operator-> () const { return m_ptr; } - inline T* get() const { return m_ptr; } - - // Operators - - COMPARE(==) - COMPARE(!=) - COMPARE(>) - COMPARE(<) - COMPARE(<=) - COMPARE(>=) - -private: - template friend class sp; - template friend class wp; - - // Optimization for wp::promote(). - sp(T* p, weakref_type* refs); - - T* m_ptr; -}; - -template -TextOutput& operator<<(TextOutput& to, const sp& val); - -// --------------------------------------------------------------------------- - template class wp { @@ -329,112 +283,11 @@ private: template TextOutput& operator<<(TextOutput& to, const wp& val); -#undef COMPARE #undef COMPARE_WEAK // --------------------------------------------------------------------------- // No user serviceable parts below here. -template -sp::sp(T* other) - : m_ptr(other) -{ - if (other) other->incStrong(this); -} - -template -sp::sp(const sp& other) - : m_ptr(other.m_ptr) -{ - if (m_ptr) m_ptr->incStrong(this); -} - -template template -sp::sp(U* other) : m_ptr(other) -{ - if (other) other->incStrong(this); -} - -template template -sp::sp(const sp& other) - : m_ptr(other.m_ptr) -{ - if (m_ptr) m_ptr->incStrong(this); -} - -template -sp::~sp() -{ - if (m_ptr) m_ptr->decStrong(this); -} - -template -sp& sp::operator = (const sp& other) { - T* otherPtr(other.m_ptr); - if (otherPtr) otherPtr->incStrong(this); - if (m_ptr) m_ptr->decStrong(this); - m_ptr = otherPtr; - return *this; -} - -template -sp& sp::operator = (T* other) -{ - if (other) other->incStrong(this); - if (m_ptr) m_ptr->decStrong(this); - m_ptr = other; - return *this; -} - -template template -sp& sp::operator = (const sp& other) -{ - U* otherPtr(other.m_ptr); - if (otherPtr) otherPtr->incStrong(this); - if (m_ptr) m_ptr->decStrong(this); - m_ptr = otherPtr; - return *this; -} - -template template -sp& sp::operator = (U* other) -{ - if (other) other->incStrong(this); - if (m_ptr) m_ptr->decStrong(this); - m_ptr = other; - return *this; -} - -template -void sp::force_set(T* other) -{ - other->forceIncStrong(this); - m_ptr = other; -} - -template -void sp::clear() -{ - if (m_ptr) { - m_ptr->decStrong(this); - m_ptr = 0; - } -} - -template -sp::sp(T* p, weakref_type* refs) - : m_ptr((p && refs->attemptIncStrong(this)) ? p : 0) -{ -} - -template -inline TextOutput& operator<<(TextOutput& to, const sp& val) -{ - return printStrongPointer(to, val.get()); -} - -// --------------------------------------------------------------------------- - template wp::wp(T* other) : m_ptr(other) @@ -572,7 +425,8 @@ void wp::set_object_and_refs(T* other, weakref_type* refs) template sp wp::promote() const { - return sp(m_ptr, m_refs); + T* p = (m_ptr && m_refs->attemptIncStrong(this)) ? m_ptr : 0; + return sp(p, true); } template @@ -590,6 +444,77 @@ inline TextOutput& operator<<(TextOutput& to, const wp& val) return printWeakPointer(to, val.unsafe_get()); } +// --------------------------------------------------------------------------- + +// this class just serves as a namespace so TYPE::moveReferences can stay +// private. + +class ReferenceMover { + // StrongReferenceCast and WeakReferenceCast do the impedance matching + // between the generic (void*) implementation in Refbase and the strongly typed + // template specializations below. + + template + struct StrongReferenceCast : public ReferenceConverterBase { + virtual size_t getReferenceTypeSize() const { return sizeof( sp ); } + virtual void* getReferenceBase(void const* p) const { + sp const* sptr(reinterpret_cast const*>(p)); + return static_cast(sptr->get()); + } + }; + + template + struct WeakReferenceCast : public ReferenceConverterBase { + virtual size_t getReferenceTypeSize() const { return sizeof( wp ); } + virtual void* getReferenceBase(void const* p) const { + wp const* sptr(reinterpret_cast const*>(p)); + return static_cast(sptr->unsafe_get()); + } + }; + +public: + template static inline + void move_references(sp* d, sp const* s, size_t n) { + memmove(d, s, n*sizeof(sp)); + StrongReferenceCast caster; + TYPE::moveReferences(d, s, n, caster); + } + template static inline + void move_references(wp* d, wp const* s, size_t n) { + memmove(d, s, n*sizeof(wp)); + WeakReferenceCast caster; + TYPE::moveReferences(d, s, n, caster); + } +}; + +// specialization for moving sp<> and wp<> types. +// these are used by the [Sorted|Keyed]Vector<> implementations +// sp<> and wp<> need to be handled specially, because they do not +// have trivial copy operation in the general case (see RefBase.cpp +// when DEBUG ops are enabled), but can be implemented very +// efficiently in most cases. + +template inline +void move_forward_type(sp* d, sp const* s, size_t n) { + ReferenceMover::move_references(d, s, n); +} + +template inline +void move_backward_type(sp* d, sp const* s, size_t n) { + ReferenceMover::move_references(d, s, n); +} + +template inline +void move_forward_type(wp* d, wp const* s, size_t n) { + ReferenceMover::move_references(d, s, n); +} + +template inline +void move_backward_type(wp* d, wp const* s, size_t n) { + ReferenceMover::move_references(d, s, n); +} + + }; // namespace android // --------------------------------------------------------------------------- diff --git a/include/utils/StrongPointer.h b/include/utils/StrongPointer.h new file mode 100644 index 000000000..5daccf4f0 --- /dev/null +++ b/include/utils/StrongPointer.h @@ -0,0 +1,224 @@ +/* + * Copyright (C) 2005 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef ANDROID_STRONG_POINTER_H +#define ANDROID_STRONG_POINTER_H + +#include + +#include +#include +#include + +// --------------------------------------------------------------------------- +namespace android { + +class TextOutput; +TextOutput& printStrongPointer(TextOutput& to, const void* val); + +template class wp; + +// --------------------------------------------------------------------------- + +#define COMPARE(_op_) \ +inline bool operator _op_ (const sp& o) const { \ + return m_ptr _op_ o.m_ptr; \ +} \ +inline bool operator _op_ (const T* o) const { \ + return m_ptr _op_ o; \ +} \ +template \ +inline bool operator _op_ (const sp& o) const { \ + return m_ptr _op_ o.m_ptr; \ +} \ +template \ +inline bool operator _op_ (const U* o) const { \ + return m_ptr _op_ o; \ +} \ +inline bool operator _op_ (const wp& o) const { \ + return m_ptr _op_ o.m_ptr; \ +} \ +template \ +inline bool operator _op_ (const wp& o) const { \ + return m_ptr _op_ o.m_ptr; \ +} + +// --------------------------------------------------------------------------- + +template +class sp +{ +public: + inline sp() : m_ptr(0) { } + + sp(T* other); + sp(const sp& other); + template sp(U* other); + template sp(const sp& other); + + ~sp(); + + // Assignment + + sp& operator = (T* other); + sp& operator = (const sp& other); + + template sp& operator = (const sp& other); + template sp& operator = (U* other); + + //! Special optimization for use by ProcessState (and nobody else). + void force_set(T* other); + + // Reset + + void clear(); + + // Accessors + + inline T& operator* () const { return *m_ptr; } + inline T* operator-> () const { return m_ptr; } + inline T* get() const { return m_ptr; } + + // Operators + + COMPARE(==) + COMPARE(!=) + COMPARE(>) + COMPARE(<) + COMPARE(<=) + COMPARE(>=) + +private: + template friend class sp; + template friend class wp; + + // Optimization for wp::promote(). + sp(T* p, bool); + + T* m_ptr; +}; + +#undef COMPARE + +template +TextOutput& operator<<(TextOutput& to, const sp& val); + +// --------------------------------------------------------------------------- +// No user serviceable parts below here. + +template +sp::sp(T* other) +: m_ptr(other) + { + if (other) other->incStrong(this); + } + +template +sp::sp(const sp& other) +: m_ptr(other.m_ptr) + { + if (m_ptr) m_ptr->incStrong(this); + } + +template template +sp::sp(U* other) : m_ptr(other) +{ + if (other) other->incStrong(this); +} + +template template +sp::sp(const sp& other) +: m_ptr(other.m_ptr) + { + if (m_ptr) m_ptr->incStrong(this); + } + +template +sp::~sp() +{ + if (m_ptr) m_ptr->decStrong(this); +} + +template +sp& sp::operator = (const sp& other) { + T* otherPtr(other.m_ptr); + if (otherPtr) otherPtr->incStrong(this); + if (m_ptr) m_ptr->decStrong(this); + m_ptr = otherPtr; + return *this; +} + +template +sp& sp::operator = (T* other) +{ + if (other) other->incStrong(this); + if (m_ptr) m_ptr->decStrong(this); + m_ptr = other; + return *this; +} + +template template +sp& sp::operator = (const sp& other) +{ + U* otherPtr(other.m_ptr); + if (otherPtr) otherPtr->incStrong(this); + if (m_ptr) m_ptr->decStrong(this); + m_ptr = otherPtr; + return *this; +} + +template template +sp& sp::operator = (U* other) +{ + if (other) other->incStrong(this); + if (m_ptr) m_ptr->decStrong(this); + m_ptr = other; + return *this; +} + +template +void sp::force_set(T* other) +{ + other->forceIncStrong(this); + m_ptr = other; +} + +template +void sp::clear() +{ + if (m_ptr) { + m_ptr->decStrong(this); + m_ptr = 0; + } +} + +template +sp::sp(T* p, bool) +: m_ptr(p) + { + } + +template +inline TextOutput& operator<<(TextOutput& to, const sp& val) +{ + return printStrongPointer(to, val.get()); +} + +}; // namespace android + +// --------------------------------------------------------------------------- + +#endif // ANDROID_STRONG_POINTER_H diff --git a/include/utils/TypeHelpers.h b/include/utils/TypeHelpers.h index 2ff2749ba..a1663f30e 100644 --- a/include/utils/TypeHelpers.h +++ b/include/utils/TypeHelpers.h @@ -37,18 +37,6 @@ template struct trait_trivial_move { enum { value = false }; }; template struct trait_pointer { enum { value = false }; }; template struct trait_pointer { enum { value = true }; }; -// sp<> can be trivially moved -template class sp; -template struct trait_trivial_move< sp >{ - enum { value = true }; -}; - -// wp<> can be trivially moved -template class wp; -template struct trait_trivial_move< wp >{ - enum { value = true }; -}; - template struct traits { enum { @@ -217,7 +205,6 @@ void move_backward_type(TYPE* d, const TYPE* s, size_t n = 1) { } } - // --------------------------------------------------------------------------- /* diff --git a/libs/utils/RefBase.cpp b/libs/utils/RefBase.cpp index f934eec80..0fd404d66 100644 --- a/libs/utils/RefBase.cpp +++ b/libs/utils/RefBase.cpp @@ -20,7 +20,6 @@ #include #include -#include #include #include #include @@ -35,6 +34,7 @@ // compile with refcounting debugging enabled #define DEBUG_REFS 0 +#define DEBUG_REFS_FATAL_SANITY_CHECKS 0 #define DEBUG_REFS_ENABLED_BY_DEFAULT 1 #define DEBUG_REFS_CALLSTACK_ENABLED 1 @@ -70,8 +70,10 @@ public: void addStrongRef(const void* /*id*/) { } void removeStrongRef(const void* /*id*/) { } + void renameStrongRefId(const void* /*old_id*/, const void* /*new_id*/) { } void addWeakRef(const void* /*id*/) { } void removeWeakRef(const void* /*id*/) { } + void renameWeakRefId(const void* /*old_id*/, const void* /*new_id*/) { } void printRefs() const { } void trackMe(bool, bool) { } @@ -87,39 +89,73 @@ public: , mTrackEnabled(!!DEBUG_REFS_ENABLED_BY_DEFAULT) , mRetain(false) { - //LOGI("NEW weakref_impl %p for RefBase %p", this, base); } ~weakref_impl() { - LOG_ALWAYS_FATAL_IF(!mRetain && mStrongRefs != NULL, "Strong references remain!"); - LOG_ALWAYS_FATAL_IF(!mRetain && mWeakRefs != NULL, "Weak references remain!"); + bool dumpStack = false; + if (!mRetain && mStrongRefs != NULL) { + dumpStack = true; +#if DEBUG_REFS_FATAL_SANITY_CHECKS + LOG_ALWAYS_FATAL("Strong references remain!"); +#else + LOGE("Strong references remain!"); +#endif + } + + if (!mRetain && mWeakRefs != NULL) { + dumpStack = true; +#if DEBUG_REFS_FATAL_SANITY_CHECKS + LOG_ALWAYS_FATAL("Weak references remain!"); +#else + LOGE("Weak references remain!"); +#endif + } + + if (dumpStack) { + CallStack stack; + stack.update(); + stack.dump(); + } } - void addStrongRef(const void* id) - { + void addStrongRef(const void* id) { + //LOGD_IF(mTrackEnabled, + // "addStrongRef: RefBase=%p, id=%p", mBase, id); addRef(&mStrongRefs, id, mStrong); } - void removeStrongRef(const void* id) - { - if (!mRetain) + void removeStrongRef(const void* id) { + //LOGD_IF(mTrackEnabled, + // "removeStrongRef: RefBase=%p, id=%p", mBase, id); + if (!mRetain) { removeRef(&mStrongRefs, id); - else + } else { addRef(&mStrongRefs, id, -mStrong); + } } - void addWeakRef(const void* id) - { + void renameStrongRefId(const void* old_id, const void* new_id) { + //LOGD_IF(mTrackEnabled, + // "renameStrongRefId: RefBase=%p, oid=%p, nid=%p", + // mBase, old_id, new_id); + renameRefsId(mStrongRefs, old_id, new_id); + } + + void addWeakRef(const void* id) { addRef(&mWeakRefs, id, mWeak); } - void removeWeakRef(const void* id) - { - if (!mRetain) + void removeWeakRef(const void* id) { + if (!mRetain) { removeRef(&mWeakRefs, id); - else + } else { addRef(&mWeakRefs, id, -mWeak); + } + } + + void renameWeakRefId(const void* old_id, const void* new_id) { + renameRefsId(mWeakRefs, old_id, new_id); } void trackMe(bool track, bool retain) @@ -133,8 +169,7 @@ public: String8 text; { - AutoMutex _l(const_cast(this)->mMutex); - + Mutex::Autolock _l(const_cast(this)->mMutex); char buf[128]; sprintf(buf, "Strong references on RefBase %p (weakref_type %p):\n", mBase, this); text.append(buf); @@ -173,6 +208,7 @@ private: { if (mTrackEnabled) { AutoMutex _l(mMutex); + ref_entry* ref = new ref_entry; // Reference count at the time of the snapshot, but before the // update. Positive value means we increment, negative--we @@ -182,7 +218,6 @@ private: #if DEBUG_REFS_CALLSTACK_ENABLED ref->stack.update(2); #endif - ref->next = *refs; *refs = ref; } @@ -200,13 +235,37 @@ private: delete ref; return; } - refs = &ref->next; ref = *refs; } - - LOG_ALWAYS_FATAL("RefBase: removing id %p on RefBase %p (weakref_type %p) that doesn't exist!", - id, mBase, this); + +#if DEBUG_REFS_FATAL_SANITY_CHECKS + LOG_ALWAYS_FATAL("RefBase: removing id %p on RefBase %p" + "(weakref_type %p) that doesn't exist!", + id, mBase, this); +#endif + + LOGE("RefBase: removing id %p on RefBase %p" + "(weakref_type %p) that doesn't exist!", + id, mBase, this); + + CallStack stack; + stack.update(); + stack.dump(); + } + } + + void renameRefsId(ref_entry* r, const void* old_id, const void* new_id) + { + if (mTrackEnabled) { + AutoMutex _l(mMutex); + ref_entry* ref = r; + while (ref != NULL) { + if (ref->id == old_id) { + ref->id = new_id; + } + ref = ref->next; + } } } @@ -236,44 +295,6 @@ private: // on removeref that match the address ones. bool mRetain; -#if 0 - void addRef(KeyedVector* refs, const void* id) - { - AutoMutex _l(mMutex); - ssize_t i = refs->indexOfKey(id); - if (i >= 0) { - ++(refs->editValueAt(i)); - } else { - i = refs->add(id, 1); - } - } - - void removeRef(KeyedVector* refs, const void* id) - { - AutoMutex _l(mMutex); - ssize_t i = refs->indexOfKey(id); - LOG_ALWAYS_FATAL_IF(i < 0, "RefBase: removing id %p that doesn't exist!", id); - if (i >= 0) { - int32_t val = --(refs->editValueAt(i)); - if (val == 0) { - refs->removeItemsAt(i); - } - } - } - - void printRefs(const KeyedVector& refs) - { - const size_t N=refs.size(); - for (size_t i=0; i mStrongRefs; - KeyedVector mWeakRefs; -#endif - #endif }; @@ -282,7 +303,6 @@ private: void RefBase::incStrong(const void* id) const { weakref_impl* const refs = mRefs; - refs->addWeakRef(id); refs->incWeak(id); refs->addStrongRef(id); @@ -314,14 +334,12 @@ void RefBase::decStrong(const void* id) const delete this; } } - refs->removeWeakRef(id); refs->decWeak(id); } void RefBase::forceIncStrong(const void* id) const { weakref_impl* const refs = mRefs; - refs->addWeakRef(id); refs->incWeak(id); refs->addStrongRef(id); @@ -373,7 +391,7 @@ void RefBase::weakref_type::decWeak(const void* id) if (impl->mStrong == INITIAL_STRONG_VALUE) delete impl->mBase; else { -// LOGV("Freeing refs %p of old RefBase %p\n", this, impl->mBase); + // LOGV("Freeing refs %p of old RefBase %p\n", this, impl->mBase); delete impl; } } else { @@ -433,7 +451,6 @@ bool RefBase::weakref_type::attemptIncStrong(const void* id) } } - impl->addWeakRef(id); impl->addStrongRef(id); #if PRINT_REFS @@ -451,7 +468,7 @@ bool RefBase::weakref_type::attemptIncStrong(const void* id) bool RefBase::weakref_type::attemptIncWeak(const void* id) { weakref_impl* const impl = static_cast(this); - + int32_t curCount = impl->mWeak; LOG_ASSERT(curCount >= 0, "attemptIncWeak called on %p after underflow", this); @@ -498,14 +515,11 @@ RefBase::weakref_type* RefBase::getWeakRefs() const RefBase::RefBase() : mRefs(new weakref_impl(this)) { -// LOGV("Creating refs %p with RefBase %p\n", mRefs, this); } RefBase::~RefBase() { -// LOGV("Destroying RefBase %p (refs %p)\n", this, mRefs); if (mRefs->mWeak == 0) { -// LOGV("Freeing refs %p of old RefBase %p\n", mRefs, this); delete mRefs; } } @@ -534,6 +548,23 @@ void RefBase::onLastWeakRef(const void* /*id*/) // --------------------------------------------------------------------------- +void RefBase::moveReferences(void* dst, void const* src, size_t n, + const ReferenceConverterBase& caster) +{ +#if DEBUG_REFS + const size_t itemSize = caster.getReferenceTypeSize(); + for (size_t i=0 ; i(intptr_t(dst) + i*itemSize); + void const* s = reinterpret_cast(intptr_t(src) + i*itemSize); + RefBase* ref(reinterpret_cast(caster.getReferenceBase(d))); + ref->mRefs->renameStrongRefId(s, d); + ref->mRefs->renameWeakRefId(s, d); + } +#endif +} + +// --------------------------------------------------------------------------- + TextOutput& printStrongPointer(TextOutput& to, const void* val) { to << "sp<>(" << val << ")";