From 9d5b897600a8f9405ec37a141b9417f34a11c557 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 2 Dec 2019 14:39:24 -0500 Subject: DRTVWR-494: Defend LLInstanceTracker against multi-thread usage. The previous implementation went to some effort to crash if anyone attempted to create or destroy an LLInstanceTracker subclass instance during traversal. That restriction is manageable within a single thread, but becomes unworkable if it's possible that a given subclass might be used on more than one thread. Remove LLInstanceTracker::instance_iter, beginInstances(), endInstances(), also key_iter, beginKeys() and endKeys(). Instead, introduce key_snapshot() and instance_snapshot(), the only means of iterating over LLInstanceTracker instances. (These are intended to resemble functions, but in fact the current implementation simply presents the classes.) Iterating over a captured snapshot defends against container modifications during traversal. The term 'snapshot' reminds the coder that a new instance created during traversal will not be considered. To defend against instance deletion during traversal, a snapshot stores std::weak_ptrs which it lazily dereferences, skipping on the fly any that have expired. Dereferencing instance_snapshot::iterator gets you a reference rather than a pointer. Because some use cases want to delete all existing instances, add an instance_snapshot::deleteAll() method that extracts the pointer. Those cases used to require explicitly copying instance pointers into a separate container; instance_snapshot() now takes care of that. It remains the caller's responsibility to ensure that all instances of that LLInstanceTracker subclass were allocated on the heap. Replace unkeyed static LLInstanceTracker::getInstance(T*) -- which returned nullptr if that instance had been destroyed -- with new getWeak() method returning std::weak_ptr. Caller must detect expiration of that weak_ptr. Adjust tests accordingly. Use of std::weak_ptr to detect expired instances requires engaging std::shared_ptr in the constructor. We now store shared_ptrs in the static containers (std::map for keyed, std::set for unkeyed). Make LLInstanceTrackerBase a template parameterized on the type of the static data it manages. For that reason, hoist static data class declarations out of the class definitions to an LLInstanceTrackerStuff namespace. Remove the static atomic sIterationNestDepth and its methods incrementDepth(), decrementDepth() and getDepth(), since they were used only to forbid creation and destruction during traversal. Add a std::mutex to static data. Introduce an internal LockStatic class that locks the mutex while providing a pointer to static data, making that the only way to access the static data. The LLINSTANCETRACKER_DTOR_NOEXCEPT macro goes away because we no longer expect ~LLInstanceTracker() to throw an exception in test programs. That affects LLTrace::StatBase as well as LLInstanceTracker itself. Adapt consumers to the new LLInstanceTracker API. --- indra/llcommon/llinstancetracker.h | 706 +++++++++++++++++++++---------------- 1 file changed, 404 insertions(+), 302 deletions(-) (limited to 'indra/llcommon/llinstancetracker.h') diff --git a/indra/llcommon/llinstancetracker.h b/indra/llcommon/llinstancetracker.h index 363d0bcbd5..76b201ad8c 100644 --- a/indra/llcommon/llinstancetracker.h +++ b/indra/llcommon/llinstancetracker.h @@ -28,354 +28,456 @@ #ifndef LL_LLINSTANCETRACKER_H #define LL_LLINSTANCETRACKER_H -#include #include +#include +#include #include +#include +#include +#include -#include "llstringtable.h" #include #include +#include -// As of 2017-05-06, as far as nat knows, only clang supports __has_feature(). -// Unfortunately VS2013's preprocessor shortcut logic doesn't prevent it from -// producing (fatal) warnings for defined(__clang__) && __has_feature(...). -// Have to work around that. -#if ! defined(__clang__) -#define __has_feature(x) 0 -#endif // __clang__ - -#if defined(LL_TEST_llinstancetracker) && __has_feature(cxx_noexcept) -// ~LLInstanceTracker() performs llassert_always() validation. That's fine in -// production code, since the llassert_always() is implemented as an LL_ERRS -// message, which will crash-with-message. In our integration test executable, -// though, this llassert_always() throws an exception instead so we can test -// error conditions and continue running the test. However -- as of C++11, -// destructors are implicitly noexcept(true). Unless we mark -// ~LLInstanceTracker() noexcept(false), the test executable crashes even on -// the ATTEMPT to throw. -#define LLINSTANCETRACKER_DTOR_NOEXCEPT noexcept(false) -#else -// If we're building for production, or in fact building *any other* test, or -// we're using a compiler that doesn't support __has_feature(), or we're not -// compiling with a C++ version that supports noexcept -- don't specify it. -#define LLINSTANCETRACKER_DTOR_NOEXCEPT -#endif - +/***************************************************************************** +* LLInstanceTrackerBase +*****************************************************************************/ /** * Base class manages "class-static" data that must actually have singleton * semantics: one instance per process, rather than one instance per module as * sometimes happens with data simply declared static. */ +namespace LLInstanceTrackerStuff +{ + struct StaticBase + { + // We need to be able to lock static data while manipulating it. + typedef std::mutex mutex_t; + mutex_t mMutex; + }; +} // namespace LLInstanceTrackerStuff + +template class LL_COMMON_API LLInstanceTrackerBase { protected: - /// It's not essential to derive your STATICDATA (for use with - /// getStatic()) from StaticBase; it's just that both known - /// implementations do. - struct StaticBase + typedef Static StaticData; + + // Instantiate this class to obtain a pointer to the canonical static + // instance of class Static while holding a lock on that instance. Use of + // Static::mMutex presumes either that Static is derived from StaticBase, + // or that Static declares some other suitable mMutex. + class LockStatic { - StaticBase(): - sIterationNestDepth(0) + typedef std::unique_lock lock_t; + public: + LockStatic(): + mData(getStatic()), + mLock(mData->mMutex) {} - - void incrementDepth(); - void decrementDepth(); - U32 getDepth(); - private: -#ifdef LL_WINDOWS - std::atomic_uint32_t sIterationNestDepth; -#else - std::atomic_uint sIterationNestDepth; -#endif - }; + Static* get() const { return mData; } + operator Static*() const { return get(); } + Static* operator->() const { return get(); } + // sometimes we must explicitly unlock... + void unlock() + { + // but once we do, access is no longer permitted + mData = nullptr; + mLock.unlock(); + } + protected: + Static* mData; + lock_t mLock; + private: + Static* getStatic() + { + static Static sData; + return &sData; + } + }; }; -LL_COMMON_API void assert_main_thread(); - +/***************************************************************************** +* LLInstanceTracker with key +*****************************************************************************/ enum EInstanceTrackerAllowKeyCollisions { - LLInstanceTrackerErrorOnCollision, - LLInstanceTrackerReplaceOnCollision + LLInstanceTrackerErrorOnCollision, + LLInstanceTrackerReplaceOnCollision }; +namespace LLInstanceTrackerStuff +{ + template + struct StaticMap: public StaticBase + { + typedef std::map InstanceMap; + InstanceMap mMap; + }; +} // LLInstanceTrackerStuff + /// This mix-in class adds support for tracking all instances of the specified class parameter T /// The (optional) key associates a value of type KEY with a given instance of T, for quick lookup /// If KEY is not provided, then instances are stored in a simple set /// @NOTE: see explicit specialization below for default KEY==void case -/// @NOTE: this class is not thread-safe unless used as read-only -template -class LLInstanceTracker : public LLInstanceTrackerBase +template +class LLInstanceTracker : + public LLInstanceTrackerBase>> { - typedef LLInstanceTracker self_t; - typedef typename std::multimap InstanceMap; - struct StaticData: public StaticBase - { - InstanceMap sMap; - }; - static StaticData& getStatic() { static StaticData sData; return sData;} - static InstanceMap& getMap_() { return getStatic().sMap; } + typedef LLInstanceTrackerBase>> super; + using typename super::StaticData; + using typename super::LockStatic; + typedef typename StaticData::InstanceMap InstanceMap; public: - class instance_iter : public boost::iterator_facade - { - public: - typedef boost::iterator_facade super_t; - - instance_iter(const typename InstanceMap::iterator& it) - : mIterator(it) - { - getStatic().incrementDepth(); - } - - ~instance_iter() - { - getStatic().decrementDepth(); - } - - - private: - friend class boost::iterator_core_access; - - void increment() { mIterator++; } - bool equal(instance_iter const& other) const - { - return mIterator == other.mIterator; - } - - T& dereference() const - { - return *(mIterator->second); - } - - typename InstanceMap::iterator mIterator; - }; - - class key_iter : public boost::iterator_facade - { - public: - typedef boost::iterator_facade super_t; - - key_iter(typename InstanceMap::iterator it) - : mIterator(it) - { - getStatic().incrementDepth(); - } - - key_iter(const key_iter& other) - : mIterator(other.mIterator) - { - getStatic().incrementDepth(); - } - - ~key_iter() - { - getStatic().decrementDepth(); - } - - - private: - friend class boost::iterator_core_access; - - void increment() { mIterator++; } - bool equal(key_iter const& other) const - { - return mIterator == other.mIterator; - } - - KEY& dereference() const - { - return const_cast(mIterator->first); - } - - typename InstanceMap::iterator mIterator; - }; - - static T* getInstance(const KEY& k) - { - const InstanceMap& map(getMap_()); - typename InstanceMap::const_iterator found = map.find(k); - return (found == map.end()) ? NULL : found->second; - } - - static instance_iter beginInstances() - { - return instance_iter(getMap_().begin()); - } - - static instance_iter endInstances() - { - return instance_iter(getMap_().end()); - } - - static S32 instanceCount() - { - return getMap_().size(); - } - - static key_iter beginKeys() - { - return key_iter(getMap_().begin()); - } - static key_iter endKeys() - { - return key_iter(getMap_().end()); - } + // snapshot of std::pair> pairs + class snapshot + { + // It's very important that what we store in this snapshot are + // weak_ptrs, NOT shared_ptrs. That's how we discover whether any + // instance has been deleted during the lifespan of a snapshot. + typedef std::vector>> VectorType; + // Dereferencing our iterator produces a std::shared_ptr for each + // instance that still exists. Since we store weak_ptrs, that involves + // two chained transformations: + // - a transform_iterator to lock the weak_ptr and return a shared_ptr + // - a filter_iterator to skip any shared_ptr that has become invalid. + // It is very important that we filter lazily, that is, during + // traversal. Any one of our stored weak_ptrs might expire during + // traversal. + typedef std::pair> strong_pair; + // Note for future reference: nat has not yet had any luck (up to + // Boost 1.67) trying to use boost::transform_iterator with a hand- + // coded functor, only with actual functions. In my experience, an + // internal boost::result_of() operation fails, even with an explicit + // result_type typedef. But this works. + static strong_pair strengthen(typename VectorType::value_type& pair) + { + return { pair.first, pair.second.lock() }; + } + static bool dead_skipper(const strong_pair& pair) + { + return bool(pair.second); + } + + public: + snapshot(): + // populate our vector with a snapshot of (locked!) InstanceMap + // note, this assigns pair to pair + mData(mLock->mMap.begin(), mLock->mMap.end()) + { + // release the lock once we've populated mData + mLock.unlock(); + } + + // You can't make a transform_iterator (or anything else) that + // literally stores a C++ function (decltype(strengthen)) -- but you + // can make a transform_iterator based on a _function pointer._ + typedef boost::transform_iterator strong_iterator; + typedef boost::filter_iterator iterator; + + iterator begin() { return make_iterator(mData.begin()); } + iterator end() { return make_iterator(mData.end()); } + + private: + iterator make_iterator(typename VectorType::iterator iter) + { + // transform_iterator only needs the base iterator and the transform. + // filter_iterator wants the predicate and both ends of the range. + return iterator(dead_skipper, + strong_iterator(iter, strengthen), + strong_iterator(mData.end(), strengthen)); + } + + LockStatic mLock; // lock static data during construction + VectorType mData; + }; + + // iterate over this for references to each instance + class instance_snapshot: public snapshot + { + private: + static T& instance_getter(typename snapshot::iterator::reference pair) + { + return *pair.second; + } + public: + typedef boost::transform_iterator iterator; + iterator begin() { return iterator(snapshot::begin(), instance_getter); } + iterator end() { return iterator(snapshot::end(), instance_getter); } + + void deleteAll() + { + for (auto it(snapshot::begin()), end(snapshot::end()); it != end; ++it) + { + delete it->second.get(); + } + } + }; + + // iterate over this for each key + class key_snapshot: public snapshot + { + private: + static KEY key_getter(typename snapshot::iterator::reference pair) + { + return pair.first; + } + public: + typedef boost::transform_iterator iterator; + iterator begin() { return iterator(snapshot::begin(), key_getter); } + iterator end() { return iterator(snapshot::end(), key_getter); } + }; + + static T* getInstance(const KEY& k) + { + LockStatic lock; + const InstanceMap& map(lock->mMap); + typename InstanceMap::const_iterator found = map.find(k); + return (found == map.end()) ? NULL : found->second.get(); + } + + static S32 instanceCount() + { + return LockStatic()->mMap.size(); + } protected: - LLInstanceTracker(const KEY& key) - { - // make sure static data outlives all instances - getStatic(); - add_(key); - } - virtual ~LLInstanceTracker() LLINSTANCETRACKER_DTOR_NOEXCEPT - { - // it's unsafe to delete instances of this type while all instances are being iterated over. - llassert_always(getStatic().getDepth() == 0); - remove_(); - } - virtual void setKey(KEY key) { remove_(); add_(key); } - virtual const KEY& getKey() const { return mInstanceKey; } + LLInstanceTracker(const KEY& key) + { + // We do not intend to manage the lifespan of this object with + // shared_ptr, so give it a no-op deleter. We store shared_ptrs in our + // InstanceMap specifically so snapshot can store weak_ptrs so we can + // detect deletions during traversals. + std::shared_ptr ptr(static_cast(this), [](T*){}); + LockStatic lock; + add_(lock, key, ptr); + } +public: + virtual ~LLInstanceTracker() + { + LockStatic lock; + remove_(lock); + } +protected: + virtual void setKey(KEY key) + { + LockStatic lock; + // Even though the shared_ptr we store in our map has a no-op deleter + // for T itself, letting the use count decrement to 0 will still + // delete the use-count object. Capture the shared_ptr we just removed + // and re-add it to the map with the new key. + auto ptr = remove_(lock); + add_(lock, key, ptr); + } +public: + virtual const KEY& getKey() const { return mInstanceKey; } private: - LLInstanceTracker( const LLInstanceTracker& ); - const LLInstanceTracker& operator=( const LLInstanceTracker& ); - - void add_(const KEY& key) - { - mInstanceKey = key; - InstanceMap& map = getMap_(); - typename InstanceMap::iterator insertion_point_it = map.lower_bound(key); - if (insertion_point_it != map.end() - && insertion_point_it->first == key) - { // found existing entry with that key - switch(KEY_COLLISION_BEHAVIOR) - { - case LLInstanceTrackerErrorOnCollision: - { - // use assert here instead of LL_ERRS(), otherwise the error will be ignored since this call is made during global object initialization - llassert_always_msg(false, "Instance with this same key already exists!"); - break; - } - case LLInstanceTrackerReplaceOnCollision: - { - // replace pointer, but leave key (should have compared equal anyway) - insertion_point_it->second = static_cast(this); - break; - } - default: - break; - } - } - else - { // new key - map.insert(insertion_point_it, std::make_pair(key, static_cast(this))); - } - } - void remove_() - { - InstanceMap& map = getMap_(); - typename InstanceMap::iterator iter = map.find(mInstanceKey); - if (iter != map.end()) - { - map.erase(iter); - } - } + LLInstanceTracker( const LLInstanceTracker& ) = delete; + LLInstanceTracker& operator=( const LLInstanceTracker& ) = delete; + + // for logging + template + static K report(K key) { return key; } + static std::string report(const std::string& key) { return "'" + key + "'"; } + static std::string report(const char* key) { return report(std::string(key)); } + + // caller must instantiate LockStatic + void add_(LockStatic& lock, const KEY& key, const std::shared_ptr& ptr) + { + mInstanceKey = key; + InstanceMap& map = lock->mMap; + switch(KEY_COLLISION_BEHAVIOR) + { + case LLInstanceTrackerErrorOnCollision: + { + // map stores shared_ptr to self + auto pair = map.emplace(key, ptr); + if (! pair.second) + { + LL_ERRS("LLInstanceTracker") << "Instance with key " << report(key) + << " already exists!" << LL_ENDL; + } + break; + } + case LLInstanceTrackerReplaceOnCollision: + map[key] = ptr; + break; + default: + break; + } + } + std::shared_ptr remove_(LockStatic& lock) + { + InstanceMap& map = lock->mMap; + typename InstanceMap::iterator iter = map.find(mInstanceKey); + if (iter != map.end()) + { + auto ret = iter->second; + map.erase(iter); + return ret; + } + return {}; + } private: - KEY mInstanceKey; + KEY mInstanceKey; }; +/***************************************************************************** +* LLInstanceTracker without key +*****************************************************************************/ +namespace LLInstanceTrackerStuff +{ + template + struct StaticSet: public StaticBase + { + typedef std::set InstanceSet; + InstanceSet mSet; + }; +} // LLInstanceTrackerStuff + +// TODO: +// - For the case of omitted KEY template parameter, consider storing +// std::map> instead of std::set>. +// That might let us share more of the implementation between KEY and +// non-KEY LLInstanceTracker subclasses. +// - Even if not that, consider trying to unify the snapshot implementations. +// The trouble is that the 'iterator' published by each (and by their +// subclasses) must reflect the specific type of the callables that +// distinguish them. (Maybe make instance_snapshot() and key_snapshot() +// factory functions that pass lambdas to a factory function for the generic +// template class?) + /// explicit specialization for default case where KEY is void /// use a simple std::set template -class LLInstanceTracker : public LLInstanceTrackerBase +class LLInstanceTracker : + public LLInstanceTrackerBase>> { - typedef LLInstanceTracker self_t; - typedef typename std::set InstanceSet; - struct StaticData: public StaticBase - { - InstanceSet sSet; - }; - static StaticData& getStatic() { static StaticData sData; return sData; } - static InstanceSet& getSet_() { return getStatic().sSet; } + typedef LLInstanceTrackerBase>> super; + using typename super::StaticData; + using typename super::LockStatic; + typedef typename StaticData::InstanceSet InstanceSet; public: + /** + * Storing a dumb T* somewhere external is a bad idea, since + * LLInstanceTracker subclasses are explicitly destroyed rather than + * managed by smart pointers. It's legal to declare stack instances of an + * LLInstanceTracker subclass. But it's reasonable to store a + * std::weak_ptr, which will become invalid when the T instance is + * destroyed. + */ + std::weak_ptr getWeak() + { + return mSelf; + } + + static S32 instanceCount() { return LockStatic()->mSet.size(); } - /** - * Does a particular instance still exist? Of course, if you already have - * a T* in hand, you need not call getInstance() to @em locate the - * instance -- unlike the case where getInstance() accepts some kind of - * key. Nonetheless this method is still useful to @em validate a - * particular T*, since each instance's destructor removes itself from the - * underlying set. - */ - static T* getInstance(T* k) - { - const InstanceSet& set(getSet_()); - typename InstanceSet::const_iterator found = set.find(k); - return (found == set.end())? NULL : *found; - } - static S32 instanceCount() { return getSet_().size(); } - - class instance_iter : public boost::iterator_facade - { - public: - instance_iter(const typename InstanceSet::iterator& it) - : mIterator(it) - { - getStatic().incrementDepth(); - } - - instance_iter(const instance_iter& other) - : mIterator(other.mIterator) - { - getStatic().incrementDepth(); - } - - ~instance_iter() - { - getStatic().decrementDepth(); - } - - private: - friend class boost::iterator_core_access; - - void increment() { mIterator++; } - bool equal(instance_iter const& other) const - { - return mIterator == other.mIterator; - } - - T& dereference() const - { - return **mIterator; - } - - typename InstanceSet::iterator mIterator; - }; - - static instance_iter beginInstances() { return instance_iter(getSet_().begin()); } - static instance_iter endInstances() { return instance_iter(getSet_().end()); } + // snapshot of std::shared_ptr pointers + class snapshot + { + // It's very important that what we store in this snapshot are + // weak_ptrs, NOT shared_ptrs. That's how we discover whether any + // instance has been deleted during the lifespan of a snapshot. + typedef std::vector> VectorType; + // Dereferencing our iterator produces a std::shared_ptr for each + // instance that still exists. Since we store weak_ptrs, that involves + // two chained transformations: + // - a transform_iterator to lock the weak_ptr and return a shared_ptr + // - a filter_iterator to skip any shared_ptr that has become invalid. + typedef std::shared_ptr strong_ptr; + static strong_ptr strengthen(typename VectorType::value_type& ptr) + { + return ptr.lock(); + } + static bool dead_skipper(const strong_ptr& ptr) + { + return bool(ptr); + } + + public: + snapshot(): + // populate our vector with a snapshot of (locked!) InstanceSet + // note, this assigns stored shared_ptrs to weak_ptrs for snapshot + mData(mLock->mSet.begin(), mLock->mSet.end()) + { + // release the lock once we've populated mData + mLock.unlock(); + } + + typedef boost::transform_iterator strong_iterator; + typedef boost::filter_iterator iterator; + + iterator begin() { return make_iterator(mData.begin()); } + iterator end() { return make_iterator(mData.end()); } + + private: + iterator make_iterator(typename VectorType::iterator iter) + { + // transform_iterator only needs the base iterator and the transform. + // filter_iterator wants the predicate and both ends of the range. + return iterator(dead_skipper, + strong_iterator(iter, strengthen), + strong_iterator(mData.end(), strengthen)); + } + + LockStatic mLock; // lock static data during construction + VectorType mData; + }; + + // iterate over this for references to each instance + struct instance_snapshot: public snapshot + { + typedef boost::indirect_iterator iterator; + iterator begin() { return iterator(snapshot::begin()); } + iterator end() { return iterator(snapshot::end()); } + + void deleteAll() + { + for (auto it(snapshot::begin()), end(snapshot::end()); it != end; ++it) + { + delete it->get(); + } + } + }; protected: - LLInstanceTracker() - { - // make sure static data outlives all instances - getStatic(); - getSet_().insert(static_cast(this)); - } - virtual ~LLInstanceTracker() LLINSTANCETRACKER_DTOR_NOEXCEPT - { - // it's unsafe to delete instances of this type while all instances are being iterated over. - llassert_always(getStatic().getDepth() == 0); - getSet_().erase(static_cast(this)); - } - - LLInstanceTracker(const LLInstanceTracker& other) - { - getSet_().insert(static_cast(this)); - } + LLInstanceTracker() + { + // Since we do not intend for this shared_ptr to manage lifespan, give + // it a no-op deleter. + std::shared_ptr ptr(static_cast(this), [](T*){}); + // save corresponding weak_ptr for future reference + mSelf = ptr; + // Also store it in our class-static set to track this instance. + LockStatic()->mSet.emplace(ptr); + } +public: + virtual ~LLInstanceTracker() + { + // convert weak_ptr to shared_ptr because that's what we store in our + // InstanceSet + LockStatic()->mSet.erase(mSelf.lock()); + } +protected: + LLInstanceTracker(const LLInstanceTracker& other): + LLInstanceTracker() + {} + +private: + // Storing a weak_ptr to self is a bit like deriving from + // std::enable_shared_from_this(), except more explicit. + std::weak_ptr mSelf; }; #endif -- cgit v1.2.3 From 7915dc45624e714706c497b45b5f2b663fa0cdc2 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 2 Dec 2019 15:40:02 -0500 Subject: DRTVWR-494: Quiet VS warnings about its own header. --- indra/llcommon/llinstancetracker.h | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) (limited to 'indra/llcommon/llinstancetracker.h') diff --git a/indra/llcommon/llinstancetracker.h b/indra/llcommon/llinstancetracker.h index 76b201ad8c..3c8a5e3fb6 100644 --- a/indra/llcommon/llinstancetracker.h +++ b/indra/llcommon/llinstancetracker.h @@ -32,10 +32,20 @@ #include #include #include -#include #include #include +#if LL_WINDOWS +#pragma warning (push) +#pragma warning (disable:4265) +#endif +// 'std::_Pad' : class has virtual functions, but destructor is not virtual +#include + +#if LL_WINDOWS +#pragma warning (pop) +#endif + #include #include #include -- cgit v1.2.3 From b080b06b422db6405982bee603118ee68e6c2500 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 3 Dec 2019 11:45:14 -0500 Subject: DRTVWR-494: Encapsulate redundant VS boilerplate around . --- indra/llcommon/llinstancetracker.h | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) (limited to 'indra/llcommon/llinstancetracker.h') diff --git a/indra/llcommon/llinstancetracker.h b/indra/llcommon/llinstancetracker.h index 3c8a5e3fb6..272ad8086e 100644 --- a/indra/llcommon/llinstancetracker.h +++ b/indra/llcommon/llinstancetracker.h @@ -35,16 +35,7 @@ #include #include -#if LL_WINDOWS -#pragma warning (push) -#pragma warning (disable:4265) -#endif -// 'std::_Pad' : class has virtual functions, but destructor is not virtual -#include - -#if LL_WINDOWS -#pragma warning (pop) -#endif +#include "mutex.h" #include #include -- cgit v1.2.3 From 1f7335fde34abdb9889e0c7b437fc02870570fcf Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 3 Dec 2019 20:44:26 -0500 Subject: DRTVWR-494: Extract LockStatic as a standalone template class. The pattern of requiring a lock to permit *any* access to a static instance of something seems generally useful. Break out lockstatic.h; recast LLInstanceTracker to use it. Moving LockStatic to an external template class instead of a nested class in LLInstanceTrackerBase leaves LLInstanceTrackerBase pretty empty. Get rid of it. And *that* means we can move the definition of the StaticData used by each LLInstanceTracker specialization into the class itself, rather than having to define it beforehand in namespace LLInstanceTrackerStuff. --- indra/llcommon/llinstancetracker.h | 98 +++++++------------------------------- 1 file changed, 18 insertions(+), 80 deletions(-) (limited to 'indra/llcommon/llinstancetracker.h') diff --git a/indra/llcommon/llinstancetracker.h b/indra/llcommon/llinstancetracker.h index 272ad8086e..cfb40c25f0 100644 --- a/indra/llcommon/llinstancetracker.h +++ b/indra/llcommon/llinstancetracker.h @@ -41,64 +41,20 @@ #include #include +#include "lockstatic.h" + /***************************************************************************** -* LLInstanceTrackerBase +* StaticBase *****************************************************************************/ -/** - * Base class manages "class-static" data that must actually have singleton - * semantics: one instance per process, rather than one instance per module as - * sometimes happens with data simply declared static. - */ namespace LLInstanceTrackerStuff { struct StaticBase { // We need to be able to lock static data while manipulating it. - typedef std::mutex mutex_t; - mutex_t mMutex; + std::mutex mMutex; }; } // namespace LLInstanceTrackerStuff -template -class LL_COMMON_API LLInstanceTrackerBase -{ -protected: - typedef Static StaticData; - - // Instantiate this class to obtain a pointer to the canonical static - // instance of class Static while holding a lock on that instance. Use of - // Static::mMutex presumes either that Static is derived from StaticBase, - // or that Static declares some other suitable mMutex. - class LockStatic - { - typedef std::unique_lock lock_t; - public: - LockStatic(): - mData(getStatic()), - mLock(mData->mMutex) - {} - Static* get() const { return mData; } - operator Static*() const { return get(); } - Static* operator->() const { return get(); } - // sometimes we must explicitly unlock... - void unlock() - { - // but once we do, access is no longer permitted - mData = nullptr; - mLock.unlock(); - } - protected: - Static* mData; - lock_t mLock; - private: - Static* getStatic() - { - static Static sData; - return &sData; - } - }; -}; - /***************************************************************************** * LLInstanceTracker with key *****************************************************************************/ @@ -108,29 +64,20 @@ enum EInstanceTrackerAllowKeyCollisions LLInstanceTrackerReplaceOnCollision }; -namespace LLInstanceTrackerStuff -{ - template - struct StaticMap: public StaticBase - { - typedef std::map InstanceMap; - InstanceMap mMap; - }; -} // LLInstanceTrackerStuff - /// This mix-in class adds support for tracking all instances of the specified class parameter T /// The (optional) key associates a value of type KEY with a given instance of T, for quick lookup /// If KEY is not provided, then instances are stored in a simple set /// @NOTE: see explicit specialization below for default KEY==void case template -class LLInstanceTracker : - public LLInstanceTrackerBase>> +class LLInstanceTracker { - typedef LLInstanceTrackerBase>> super; - using typename super::StaticData; - using typename super::LockStatic; - typedef typename StaticData::InstanceMap InstanceMap; + typedef std::map> InstanceMap; + struct StaticData: public LLInstanceTrackerStuff::StaticBase + { + InstanceMap mMap; + }; + typedef llthread::LockStatic LockStatic; public: // snapshot of std::pair> pairs @@ -334,16 +281,6 @@ private: /***************************************************************************** * LLInstanceTracker without key *****************************************************************************/ -namespace LLInstanceTrackerStuff -{ - template - struct StaticSet: public StaticBase - { - typedef std::set InstanceSet; - InstanceSet mSet; - }; -} // LLInstanceTrackerStuff - // TODO: // - For the case of omitted KEY template parameter, consider storing // std::map> instead of std::set>. @@ -359,13 +296,14 @@ namespace LLInstanceTrackerStuff /// explicit specialization for default case where KEY is void /// use a simple std::set template -class LLInstanceTracker : - public LLInstanceTrackerBase>> +class LLInstanceTracker { - typedef LLInstanceTrackerBase>> super; - using typename super::StaticData; - using typename super::LockStatic; - typedef typename StaticData::InstanceSet InstanceSet; + typedef std::set> InstanceSet; + struct StaticData: public LLInstanceTrackerStuff::StaticBase + { + InstanceSet mSet; + }; + typedef llthread::LockStatic LockStatic; public: /** -- cgit v1.2.3 From 2506fd78824d92e512931d4bc2ff5cef4fc8c9c6 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 6 Dec 2019 15:04:11 -0500 Subject: DRTVWR-494: Move LL_ERRS out of llinstancetracker.h header file. Add a namespaced free function in .cpp file to report LL_ERRS as needed. Per code review, use a more indicative namespace name. --- indra/llcommon/llinstancetracker.h | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) (limited to 'indra/llcommon/llinstancetracker.h') diff --git a/indra/llcommon/llinstancetracker.h b/indra/llcommon/llinstancetracker.h index cfb40c25f0..196bc5c0dd 100644 --- a/indra/llcommon/llinstancetracker.h +++ b/indra/llcommon/llinstancetracker.h @@ -42,18 +42,21 @@ #include #include "lockstatic.h" +#include "stringize.h" /***************************************************************************** * StaticBase *****************************************************************************/ -namespace LLInstanceTrackerStuff +namespace LLInstanceTrackerPrivate { struct StaticBase { // We need to be able to lock static data while manipulating it. std::mutex mMutex; }; -} // namespace LLInstanceTrackerStuff + + void logerrs(const char* cls, const std::string&, const std::string&, const std::string&); +} // namespace LLInstanceTrackerPrivate /***************************************************************************** * LLInstanceTracker with key @@ -73,7 +76,7 @@ template> InstanceMap; - struct StaticData: public LLInstanceTrackerStuff::StaticBase + struct StaticData: public LLInstanceTrackerPrivate::StaticBase { InstanceMap mMap; }; @@ -232,7 +235,7 @@ private: // for logging template - static K report(K key) { return key; } + static std::string report(K key) { return stringize(key); } static std::string report(const std::string& key) { return "'" + key + "'"; } static std::string report(const char* key) { return report(std::string(key)); } @@ -249,8 +252,8 @@ private: auto pair = map.emplace(key, ptr); if (! pair.second) { - LL_ERRS("LLInstanceTracker") << "Instance with key " << report(key) - << " already exists!" << LL_ENDL; + LLInstanceTrackerPrivate::logerrs(typeid(*this).name(), " instance with key ", + report(key), " already exists!"); } break; } @@ -299,7 +302,7 @@ template class LLInstanceTracker { typedef std::set> InstanceSet; - struct StaticData: public LLInstanceTrackerStuff::StaticBase + struct StaticData: public LLInstanceTrackerPrivate::StaticBase { InstanceSet mSet; }; -- cgit v1.2.3 From 557a74fbddac609ce238a7bbafc3138b95956575 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 11 Dec 2019 15:41:01 -0500 Subject: DRTVWR-476: Adapt LLInstanceTracker::snapshot for VS limitations. --- indra/llcommon/llinstancetracker.h | 38 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) (limited to 'indra/llcommon/llinstancetracker.h') diff --git a/indra/llcommon/llinstancetracker.h b/indra/llcommon/llinstancetracker.h index 196bc5c0dd..402333cca7 100644 --- a/indra/llcommon/llinstancetracker.h +++ b/indra/llcommon/llinstancetracker.h @@ -143,7 +143,24 @@ public: strong_iterator(mData.end(), strengthen)); } - LockStatic mLock; // lock static data during construction + // lock static data during construction +#if ! LL_WINDOWS + LockStatic mLock; +#else // LL_WINDOWS + // We want to be able to use (e.g.) our instance_snapshot subclass as: + // for (auto& inst : T::instance_snapshot()) ... + // But when this snapshot base class directly contains LockStatic, as + // above, Visual Studio 2017 requires us to code instead: + // for (auto& inst : std::move(T::instance_snapshot())) ... + // nat thinks this should be unnecessary, as an anonymous class + // instance is already a temporary. It shouldn't need to be cast to + // rvalue reference (the role of std::move()). clang evidently agrees, + // as the short form works fine with Xcode on Mac. + // To support the succinct usage, instead of directly storing + // LockStatic, store std::shared_ptr, which is copyable. + std::shared_ptr mLockp{std::make_shared()}; + LockStatic& mLock{*mLockp}; +#endif // LL_WINDOWS VectorType mData; }; @@ -373,7 +390,24 @@ public: strong_iterator(mData.end(), strengthen)); } - LockStatic mLock; // lock static data during construction + // lock static data during construction +#if ! LL_WINDOWS + LockStatic mLock; +#else // LL_WINDOWS + // We want to be able to use our instance_snapshot subclass as: + // for (auto& inst : T::instance_snapshot()) ... + // But when this snapshot base class directly contains LockStatic, as + // above, Visual Studio 2017 requires us to code instead: + // for (auto& inst : std::move(T::instance_snapshot())) ... + // nat thinks this should be unnecessary, as an anonymous class + // instance is already a temporary. It shouldn't need to be cast to + // rvalue reference (the role of std::move()). clang evidently agrees, + // as the short form works fine with Xcode on Mac. + // To support the succinct usage, instead of directly storing + // LockStatic, store std::shared_ptr, which is copyable. + std::shared_ptr mLockp{std::make_shared()}; + LockStatic& mLock{*mLockp}; +#endif // LL_WINDOWS VectorType mData; }; -- cgit v1.2.3