From ad3ab3ad211650fe0cf7c304a4032c87326a0d10 Mon Sep 17 00:00:00 2001 From: rdb Date: Mon, 30 Jul 2018 17:27:09 +0200 Subject: [PATCH] Define stable ordering for WeakPointerTo for use as map/set key Currently, the WeakPointerTo comparison operators compare the raw pointers, but this is not useful as it may cause a false equality if one weak pointer in the comparison is expired and points to memory that has since been reused. Instead, we can define a comparison based on the control block pointer, which exists since the new weak pointer implementation in 0bb81a43c9e4fffb37cc2234c1b0fbae42020ceb. This is implemented in the owner_before method, matching C++11 std::weak_ptr semantics. I would now recommend deprecating most comparison operators of WeakPointerTo or redefining them to make more sense, ie. comparing equal if they (once) referred to the same object and not if they simply point to the same memory address. This has not yet been done, though code that uses the comparison operators has been fixed in this commit. Overloads of std::owner_less have been provided for creating a map or set with Weak(Const)PointerTo keys. --- dtool/src/dtoolbase/dtoolbase_cc.h | 2 + panda/src/chan/partBundle.h | 2 +- panda/src/char/characterJointEffect.I | 6 +- panda/src/dxgsg9/dxGeomMunger9.cxx | 14 ++-- panda/src/express/pointerTo.h | 20 ++++++ panda/src/express/pointerToBase.h | 1 + panda/src/express/weakPointerTo.h | 19 ++++++ panda/src/express/weakPointerToBase.I | 94 +++++++++++++++++++------- panda/src/express/weakPointerToBase.h | 5 ++ panda/src/glstuff/glGeomMunger_src.cxx | 28 +++++--- panda/src/pgraph/nodePath.cxx | 14 ++-- panda/src/pgraph/weakNodePath.I | 21 +++--- 12 files changed, 167 insertions(+), 59 deletions(-) diff --git a/dtool/src/dtoolbase/dtoolbase_cc.h b/dtool/src/dtoolbase/dtoolbase_cc.h index b4ce1d597c..3af3c9c250 100644 --- a/dtool/src/dtoolbase/dtoolbase_cc.h +++ b/dtool/src/dtoolbase/dtoolbase_cc.h @@ -111,6 +111,8 @@ namespace std { template typename remove_reference::type &&move(T &&t) { return static_cast::type&&>(t); } + + template struct owner_less; }; #endif diff --git a/panda/src/chan/partBundle.h b/panda/src/chan/partBundle.h index 7a611a80de..f8fda4bd47 100644 --- a/panda/src/chan/partBundle.h +++ b/panda/src/chan/partBundle.h @@ -172,7 +172,7 @@ private: typedef pvector Nodes; Nodes _nodes; - typedef pmap AppliedTransforms; + typedef pmap > AppliedTransforms; AppliedTransforms _applied_transforms; double _update_delay; diff --git a/panda/src/char/characterJointEffect.I b/panda/src/char/characterJointEffect.I index e4d596dafe..34e2502afa 100644 --- a/panda/src/char/characterJointEffect.I +++ b/panda/src/char/characterJointEffect.I @@ -35,5 +35,9 @@ get_character() const { */ INLINE bool CharacterJointEffect:: matches_character(Character *character) const { - return _character == character; + // This works because while the Character is destructing, the ref count will + // be 0 but was_deleted() will still return false. We cannot construct a + // PointerTo to the character (via lock() or otherwise) when the reference + // count is 0 since that will cause double deletion. + return _character.get_orig() == character && !_character.was_deleted(); } diff --git a/panda/src/dxgsg9/dxGeomMunger9.cxx b/panda/src/dxgsg9/dxGeomMunger9.cxx index 8aa539c11f..638b121f27 100644 --- a/panda/src/dxgsg9/dxGeomMunger9.cxx +++ b/panda/src/dxgsg9/dxGeomMunger9.cxx @@ -300,8 +300,11 @@ compare_to_impl(const GeomMunger *other) const { if (_filtered_texture != om->_filtered_texture) { return _filtered_texture < om->_filtered_texture ? -1 : 1; } - if (_tex_gen != om->_tex_gen) { - return _tex_gen < om->_tex_gen ? -1 : 1; + if (_tex_gen.owner_before(om->_tex_gen)) { + return -1; + } + if (om->_tex_gen.owner_before(_tex_gen)) { + return 1; } return StandardMunger::compare_to_impl(other); @@ -321,8 +324,11 @@ geom_compare_to_impl(const GeomMunger *other) const { if (_filtered_texture != om->_filtered_texture) { return _filtered_texture < om->_filtered_texture ? -1 : 1; } - if (_tex_gen != om->_tex_gen) { - return _tex_gen < om->_tex_gen ? -1 : 1; + if (_tex_gen.owner_before(om->_tex_gen)) { + return -1; + } + if (om->_tex_gen.owner_before(_tex_gen)) { + return 1; } return StandardMunger::geom_compare_to_impl(other); diff --git a/panda/src/express/pointerTo.h b/panda/src/express/pointerTo.h index e9250a2f8a..cb790ebd81 100644 --- a/panda/src/express/pointerTo.h +++ b/panda/src/express/pointerTo.h @@ -215,6 +215,26 @@ void swap(ConstPointerTo &one, ConstPointerTo &two) noexcept { } +// Define owner_less specializations, for completeness' sake. +namespace std { + template + struct owner_less > { + bool operator () (const PointerTo &lhs, + const PointerTo &rhs) const noexcept { + return lhs < rhs; + } + }; + + template + struct owner_less > { + bool operator () (const ConstPointerTo &lhs, + const ConstPointerTo &rhs) const noexcept { + return lhs < rhs; + } + }; +} + + // Finally, we'll define a couple of handy abbreviations to save on all that // wasted typing time. diff --git a/panda/src/express/pointerToBase.h b/panda/src/express/pointerToBase.h index 959574aba7..832717e14e 100644 --- a/panda/src/express/pointerToBase.h +++ b/panda/src/express/pointerToBase.h @@ -53,6 +53,7 @@ protected: // This is needed to be able to access the privates of other instantiations. template friend class PointerToBase; + template friend class WeakPointerToBase; PUBLISHED: ALWAYS_INLINE void clear(); diff --git a/panda/src/express/weakPointerTo.h b/panda/src/express/weakPointerTo.h index ee50b0ebc3..bd302e0ec9 100644 --- a/panda/src/express/weakPointerTo.h +++ b/panda/src/express/weakPointerTo.h @@ -152,6 +152,25 @@ PUBLISHED: INLINE void clear() { WeakPointerToBase::clear(); } }; +// Provide specializations of std::owner_less, for using a WPT as a map key. +namespace std { + template + struct owner_less > { + bool operator () (const WeakPointerTo &lhs, + const WeakPointerTo &rhs) const noexcept { + return lhs.owner_before(rhs); + } + }; + + template + struct owner_less > { + bool operator () (const WeakConstPointerTo &lhs, + const WeakConstPointerTo &rhs) const noexcept { + return lhs.owner_before(rhs); + } + }; +} + #define WPT(type) WeakPointerTo< type > #define WCPT(type) WeakConstPointerTo< type > diff --git a/panda/src/express/weakPointerToBase.I b/panda/src/express/weakPointerToBase.I index 5410903fdd..c57ea31719 100644 --- a/panda/src/express/weakPointerToBase.I +++ b/panda/src/express/weakPointerToBase.I @@ -12,7 +12,8 @@ */ /** - * + * Constructs a weak pointer from a plain pointer (or nullptr). It is the + * caller's responsibility to ensure that it points to a valid object. */ template INLINE WeakPointerToBase:: @@ -27,7 +28,7 @@ WeakPointerToBase(To *ptr) { } /** - * + * Constructs a weak pointer from a reference-counting pointer. */ template INLINE WeakPointerToBase:: @@ -42,24 +43,25 @@ WeakPointerToBase(const PointerToBase ©) { } /** - * + * Copies a weak pointer. This is always safe, even for expired pointers. */ template INLINE WeakPointerToBase:: WeakPointerToBase(const WeakPointerToBase ©) { _void_ptr = copy._void_ptr; - // Don't bother increasing the weak reference count if the object was - // already deleted. + // While it is tempting to stop maintaining the control block pointer after + // the object has been deleted, we still need it in order to define a + // consistent ordering in owner_before. WeakReferenceList *weak_ref = copy._weak_ref; - if (weak_ref != nullptr && !weak_ref->was_deleted()) { + if (weak_ref != nullptr/* && !weak_ref->was_deleted()*/) { _weak_ref = copy._weak_ref; _weak_ref->ref(); } } /** - * + * Moves a weak pointer. This is always safe, even for expired pointers. */ template INLINE WeakPointerToBase:: @@ -71,7 +73,7 @@ WeakPointerToBase(WeakPointerToBase &&from) noexcept { } /** - * + * Moves a weak pointer from a cast-convertible weak pointer type. */ template template @@ -148,10 +150,11 @@ reassign(const WeakPointerToBase ©) { WeakReferenceList *old_ref = (WeakReferenceList *)_weak_ref; _void_ptr = new_ptr; - // Don't bother increasing the weak reference count if the object was - // already deleted. + // While it is tempting to stop maintaining the control block pointer + // after the object has been deleted, we still need it in order to define + // a consistent ordering in owner_before. WeakReferenceList *weak_ref = copy._weak_ref; - if (weak_ref != nullptr && !weak_ref->was_deleted()) { + if (weak_ref != nullptr/* && !weak_ref->was_deleted()*/) { weak_ref->ref(); _weak_ref = weak_ref; } else { @@ -371,7 +374,11 @@ operator >= (std::nullptr_t) const { } /** - * + * Returns true if both pointers have the same raw pointer value. For this to + * be meaningful, neither pointer may have expired, since if one has expired + * while the other was allocated at the expired pointer's memory address, this + * comparison will be true even though they didn't refer to the same object. + * @see owner_before */ template INLINE bool WeakPointerToBase:: @@ -380,7 +387,7 @@ operator == (const WeakPointerToBase &other) const { } /** - * + * @see operator == */ template INLINE bool WeakPointerToBase:: @@ -389,7 +396,8 @@ operator != (const WeakPointerToBase &other) const { } /** - * + * Defines an ordering between WeakPointerTo based on their raw pointer value. + * @deprecated Do not use this. Use owner_before or std::owner_less instead. */ template INLINE bool WeakPointerToBase:: @@ -398,7 +406,8 @@ operator > (const WeakPointerToBase &other) const { } /** - * + * Defines an ordering between WeakPointerTo based on their raw pointer value. + * @deprecated Do not use this. Use owner_before or std::owner_less instead. */ template INLINE bool WeakPointerToBase:: @@ -407,7 +416,8 @@ operator <= (const WeakPointerToBase &other) const { } /** - * + * Defines an ordering between WeakPointerTo based on their raw pointer value. + * @deprecated Do not use this. Use owner_before or std::owner_less instead. */ template INLINE bool WeakPointerToBase:: @@ -416,7 +426,7 @@ operator >= (const WeakPointerToBase &other) const { } /** - * + * Returns true if both pointers point to the same object. */ template INLINE bool WeakPointerToBase:: @@ -425,7 +435,7 @@ operator == (const PointerToBase &other) const { } /** - * + * Returns false if both pointers point to the same object. */ template INLINE bool WeakPointerToBase:: @@ -479,7 +489,8 @@ operator < (std::nullptr_t) const { } /** - * + * Defines an ordering between WeakPointerTo based on their raw pointer value. + * @deprecated Do not use this. Use owner_before or std::owner_less instead. */ template INLINE bool WeakPointerToBase:: @@ -498,6 +509,35 @@ operator < (const PointerToBase &other) const { #endif // CPPPARSER +/** + * Defines an ordering that is guaranteed to remain consistent even after the + * weak pointers have expired. This may result in two pointers with the same + * get_orig() value comparing unequal if one of them is a new object that was + * allocated at the same memory address as the older, expired pointer. + */ +template +template +INLINE bool WeakPointerToBase:: +owner_before(const WeakPointerToBase &other) const noexcept { + return _weak_ref < other._weak_ref; +} + +/** + * Defines an ordering that is guaranteed to remain consistent even after this + * weak pointer has expired. This may result in two pointers with the same + * get_orig() value comparing unequal if one of them is a new object that was + * allocated at the same memory address as the older, expired pointer. + */ +template +template +INLINE bool WeakPointerToBase:: +owner_before(const PointerToBase &other) const noexcept { + // Unfortunately, this may needlessly cause a control block to be allocated, + // but I do not see a more efficient solution. + return (other._void_ptr != nullptr) && + (_void_ptr == nullptr || _weak_ref < ((const Y *)other._void_ptr)->get_weak_list()); +} + /** * A convenient way to set the PointerTo object to NULL. (Assignment to a NULL * pointer also works, of course.) @@ -539,9 +579,17 @@ template INLINE void WeakPointerToBase:: output(std::ostream &out) const { out << _void_ptr; - if (was_deleted()) { - out << ":deleted"; - } else if (_void_ptr != nullptr) { - out << ":" << ((To *)_void_ptr)->get_ref_count(); + + WeakReferenceList *weak_ref = this->_weak_ref; + if (weak_ref != nullptr) { + weak_ref->_lock.lock(); + if (!weak_ref->was_deleted()) { + out << ":" << ((To *)_void_ptr)->get_ref_count(); + } else { + out << ":deleted"; + } + weak_ref->_lock.unlock(); + } else { + out << ":invalid"; } } diff --git a/panda/src/express/weakPointerToBase.h b/panda/src/express/weakPointerToBase.h index 94efd051eb..60be402c4c 100644 --- a/panda/src/express/weakPointerToBase.h +++ b/panda/src/express/weakPointerToBase.h @@ -89,6 +89,11 @@ public: INLINE bool operator < (const PointerToBase &other) const; #endif // CPPPARSER + template + INLINE bool owner_before(const WeakPointerToBase &other) const noexcept; + template + INLINE bool owner_before(const PointerToBase &other) const noexcept; + PUBLISHED: INLINE void clear(); INLINE void refresh() const; diff --git a/panda/src/glstuff/glGeomMunger_src.cxx b/panda/src/glstuff/glGeomMunger_src.cxx index 27f226a6cc..923b8e5b76 100644 --- a/panda/src/glstuff/glGeomMunger_src.cxx +++ b/panda/src/glstuff/glGeomMunger_src.cxx @@ -426,11 +426,17 @@ premunge_format_impl(const GeomVertexFormat *orig) { int CLP(GeomMunger):: compare_to_impl(const GeomMunger *other) const { const CLP(GeomMunger) *om = (CLP(GeomMunger) *)other; - if (_texture != om->_texture) { - return _texture < om->_texture ? -1 : 1; + if (_texture.owner_before(om->_texture)) { + return -1; } - if (_tex_gen != om->_tex_gen) { - return _tex_gen < om->_tex_gen ? -1 : 1; + if (om->_texture.owner_before(_texture)) { + return 1; + } + if (_tex_gen.owner_before(om->_tex_gen)) { + return -1; + } + if (om->_tex_gen.owner_before(_tex_gen)) { + return 1; } if (_flags != om->_flags) { return _flags < om->_flags ? -1 : 1; @@ -447,11 +453,17 @@ compare_to_impl(const GeomMunger *other) const { int CLP(GeomMunger):: geom_compare_to_impl(const GeomMunger *other) const { const CLP(GeomMunger) *om = (CLP(GeomMunger) *)other; - if (_texture != om->_texture) { - return _texture < om->_texture ? -1 : 1; + if (_texture.owner_before(om->_texture)) { + return -1; } - if (_tex_gen != om->_tex_gen) { - return _tex_gen < om->_tex_gen ? -1 : 1; + if (om->_texture.owner_before(_texture)) { + return 1; + } + if (_tex_gen.owner_before(om->_tex_gen)) { + return -1; + } + if (om->_tex_gen.owner_before(_tex_gen)) { + return 1; } if (_flags != om->_flags) { return _flags < om->_flags ? -1 : 1; diff --git a/panda/src/pgraph/nodePath.cxx b/panda/src/pgraph/nodePath.cxx index 9123a951b5..a21df38d19 100644 --- a/panda/src/pgraph/nodePath.cxx +++ b/panda/src/pgraph/nodePath.cxx @@ -5177,7 +5177,7 @@ get_stashed_ancestor(Thread *current_thread) const { */ bool NodePath:: operator == (const WeakNodePath &other) const { - return _head == other._head; + return (other == *this); } /** @@ -5185,7 +5185,7 @@ operator == (const WeakNodePath &other) const { */ bool NodePath:: operator != (const WeakNodePath &other) const { - return _head != other._head; + return (other != *this); } /** @@ -5196,7 +5196,7 @@ operator != (const WeakNodePath &other) const { */ bool NodePath:: operator < (const WeakNodePath &other) const { - return _head < other._head; + return other.compare_to(*this) > 0; } /** @@ -5211,13 +5211,7 @@ operator < (const WeakNodePath &other) const { */ int NodePath:: compare_to(const WeakNodePath &other) const { - // Nowadays, the NodePathComponents at the head are pointerwise equivalent - // if and only if the NodePaths are equivalent. So we only have to compare - // pointers. - if (_head != other._head) { - return _head < other._head ? -1 : 1; - } - return 0; + return -other.compare_to(*this); } /** diff --git a/panda/src/pgraph/weakNodePath.I b/panda/src/pgraph/weakNodePath.I index c2d4b0a523..3096f99c41 100644 --- a/panda/src/pgraph/weakNodePath.I +++ b/panda/src/pgraph/weakNodePath.I @@ -124,7 +124,7 @@ node() const { */ INLINE bool WeakNodePath:: operator == (const NodePath &other) const { - return _head == other._head; + return _head.get_orig() == other._head && !_head.was_deleted(); } /** @@ -132,7 +132,7 @@ operator == (const NodePath &other) const { */ INLINE bool WeakNodePath:: operator != (const NodePath &other) const { - return _head != other._head; + return !operator == (other); } /** @@ -143,7 +143,7 @@ operator != (const NodePath &other) const { */ INLINE bool WeakNodePath:: operator < (const NodePath &other) const { - return _head < other._head; + return _head.owner_before(other._head); } /** @@ -158,8 +158,8 @@ operator < (const NodePath &other) const { */ INLINE int WeakNodePath:: compare_to(const NodePath &other) const { - if (_head != other._head) { - return _head < other._head ? -1 : 1; + if (operator != (other)) { + return _head.owner_before(other._head) ? -1 : 1; } return 0; } @@ -170,7 +170,7 @@ compare_to(const NodePath &other) const { */ INLINE bool WeakNodePath:: operator == (const WeakNodePath &other) const { - return _head == other._head; + return !_head.owner_before(other._head) && !other._head.owner_before(_head); } /** @@ -178,7 +178,7 @@ operator == (const WeakNodePath &other) const { */ INLINE bool WeakNodePath:: operator != (const WeakNodePath &other) const { - return _head != other._head; + return _head.owner_before(other._head) || other._head.owner_before(_head); } /** @@ -189,7 +189,7 @@ operator != (const WeakNodePath &other) const { */ INLINE bool WeakNodePath:: operator < (const WeakNodePath &other) const { - return _head < other._head; + return _head.owner_before(other._head); } /** @@ -204,10 +204,7 @@ operator < (const WeakNodePath &other) const { */ INLINE int WeakNodePath:: compare_to(const WeakNodePath &other) const { - if (_head != other._head) { - return _head < other._head ? -1 : 1; - } - return 0; + return other._head.owner_before(_head) - _head.owner_before(other._head); } /**