From 63103afb65d67359a77e3f440ac868a62a258b1b Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 14 Aug 2019 13:43:08 -0400 Subject: No such thing as 'virtual static' --- indra/newview/llimagefiltersmanager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra') diff --git a/indra/newview/llimagefiltersmanager.cpp b/indra/newview/llimagefiltersmanager.cpp index ee6b39efac..c23cdc8103 100644 --- a/indra/newview/llimagefiltersmanager.cpp +++ b/indra/newview/llimagefiltersmanager.cpp @@ -48,7 +48,7 @@ LLImageFiltersManager::~LLImageFiltersManager() { } -// virtual static +// virtual void LLImageFiltersManager::initSingleton() { loadAllFilters(); -- cgit v1.2.3 From 0e6a6c7775816a338801c1a74a5741b2eabe801b Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 14 Aug 2019 13:57:13 -0400 Subject: DRTVWR-493: Work around static initialization order problem. LLParamSingleton contained a static member mutex. Unfortunately that wasn't guaranteed to be initialized by the time its getInstance() was entered. Use a function-local static instead. --- indra/llcommon/llsingleton.h | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-) (limited to 'indra') diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 03b7d5a349..e0d75ed72a 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -502,6 +502,10 @@ class LLParamSingleton : public LLSingleton { private: typedef LLSingleton super; + // Use a recursive_mutex in case of constructor circularity. With a + // non-recursive mutex, that would result in deadlock rather than the + // logerrs() call in getInstance(). + typedef std::recursive_mutex mutex_t; public: using super::deleteSingleton; @@ -515,7 +519,7 @@ public: // In case racing threads both call initParamSingleton() at the same // time, serialize them. One should initialize; the other should see // mInitState already set. - std::unique_lock lk(mMutex); + std::unique_lock lk(getMutex()); // For organizational purposes this function shouldn't be called twice if (super::sData.mInitState != super::UNINITIALIZED) { @@ -534,7 +538,7 @@ public: { // In case racing threads call getInstance() at the same moment as // initParamSingleton(), serialize the calls. - std::unique_lock lk(mMutex); + std::unique_lock lk(getMutex()); switch (super::sData.mInitState) { @@ -573,15 +577,30 @@ public: } private: - // Use a recursive_mutex in case of constructor circularity. With a - // non-recursive mutex, that would result in deadlock rather than the - // logerrs() call coded above. - static std::recursive_mutex mMutex; + // sMutex must be a function-local static rather than a static member. One + // of the essential features of LLSingleton and friends is that they must + // support getInstance() even when the containing module's static + // variables have not yet been runtime-initialized. A mutex requires + // construction. A static class member might not yet have been + // constructed. + // + // We could store a dumb mutex_t*, notice when it's NULL and allocate a + // heap mutex -- but that's vulnerable to race conditions. And we can't + // defend the dumb pointer with another mutex. + // + // We could store a std::atomic -- but a default-constructed + // std::atomic does not contain a valid T, even a default-constructed + // T! Which means std::atomic, too, requires runtime initialization. + // + // But a function-local static is guaranteed to be initialized exactly + // once, the first time control reaches that declaration. + static mutex_t& getMutex() + { + static mutex_t sMutex; + return sMutex; + } }; -template -typename std::recursive_mutex LLParamSingleton::mMutex; - /** * Initialization locked singleton, only derived class can decide when to initialize. * Starts locked. @@ -634,7 +653,7 @@ public: * file, use 'inline' (unless it's a template class) to avoid duplicate-symbol * errors at link time. */ -#define LLSINGLETON(DERIVED_CLASS, ...) \ +#define LLSINGLETON(DERIVED_CLASS, ...) \ private: \ /* implement LLSingleton pure virtual method whose sole purpose */ \ /* is to remind people to use this macro */ \ -- cgit v1.2.3 From d10b06dc4fd69a8b9bc0baf42f88f88c39ed17e4 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 19 Aug 2019 10:00:09 -0400 Subject: DRTVWR-493: When a test fails due to exception, display exception. --- indra/test/test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'indra') diff --git a/indra/test/test.cpp b/indra/test/test.cpp index d4cd4b951e..b14c2eb255 100644 --- a/indra/test/test.cpp +++ b/indra/test/test.cpp @@ -253,7 +253,7 @@ public: break; case tut::test_result::ex: ++mFailedTests; - out << "exception"; + out << "exception: " << tr.exception_typeid; break; case tut::test_result::warn: ++mFailedTests; @@ -264,7 +264,7 @@ public: out << "abnormal termination"; break; case tut::test_result::skip: - ++mSkippedTests; + ++mSkippedTests; out << "skipped known failure"; break; default: -- cgit v1.2.3 From 310db14beefc29ee72e0d13f0cb63cb2958ebf68 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 19 Aug 2019 11:44:56 -0400 Subject: DRTVWR-493: Improve exception safety of LLSingleton initialization. Add try/catch clauses to constructSingleton() (to catch exceptions in the subclass constructor) and finishInitializing() (to catch exceptions in the subclass initSingleton() method). Each of these catch clauses rethrows the exception -- they're for cleanup, not for ultimate handling. Introduce LLSingletonBase::reset_initializing(list_t::size_t). The idea is that since we can't know whether the exception happened before or after the push_initializing() call in LLSingletonBase's constructor, we can't just pop the stack. Instead, constructSingleton() captures the stack size before attempting to construct the new LLSingleton subclass. On exception, it calls reset_initializing() to restore the stack to that size. Naturally that requires a corresponding LLSingleton_manage_master method, whose MasterList specialization is a no-op. finishInitializing()'s exception handling is a bit simpler because it has a constructed LLSingleton subclass instance in hand, therefore push_initializing() has definitely been called, therefore it can call pop_initializing(). Break out new static capture_dependency() method from finishInitializing() because, in the previous LLSingleton::getInstance() implementation, the logic now wrapped in capture_dependency() was reached even in the INITIALIZED case. TODO: Add a new EInitState to differentiate "have been constructed, now calling initSingleton()" from "fully initialized, normal case" -- in the latter control path we should not be calling capture_dependency(). The LLSingleton_manage_master specialization's get_initializing() function (which called get_initializing_from()) was potentially dangerous. get_initializing() is called by push_initializing(), which (in the general case) is called by LLSingletonBase's constructor. If somehow the MasterList's LLSingletonBase constructor ended up calling get_initializing(), it would have called get_initializing_from(), passing an LLSingletonBase which had not yet been constructed into the MasterList. In particular, its mInitializing map would not yet have been initialized at all. Since the MasterList must not, by design, depend on any other LLSingletons, LLSingleton_manage_master::get_initializing() need not return a list from the official mInitializing map anyway. It can, and should, and now does, return a static dummy list. That obviates get_initializing_from(), which is removed. That in turn means we no longer need to pass get_initializing() an LLSingletonBase*. Remove that parameter. --- indra/llcommon/llsingleton.cpp | 31 +++++++++++--- indra/llcommon/llsingleton.h | 97 +++++++++++++++++++++++++++++++++++------- 2 files changed, 106 insertions(+), 22 deletions(-) (limited to 'indra') diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp index 9fbd78a000..adf72bf700 100644 --- a/indra/llcommon/llsingleton.cpp +++ b/indra/llcommon/llsingleton.cpp @@ -134,12 +134,6 @@ LLSingletonBase::list_t& LLSingletonBase::get_initializing() return LLSingletonBase::MasterList::instance().get_initializing_(); } -//static -LLSingletonBase::list_t& LLSingletonBase::get_initializing_from(MasterList* master) -{ - return master->get_initializing_(); -} - LLSingletonBase::~LLSingletonBase() {} void LLSingletonBase::push_initializing(const char* name) @@ -186,6 +180,31 @@ void LLSingletonBase::pop_initializing() log_initializing("Popping", typeid(*back).name()); } +void LLSingletonBase::reset_initializing(list_t::size_type size) +{ + // called for cleanup in case the LLSingleton subclass constructor throws + // an exception + + // The tricky thing about this, the reason we have a separate method + // instead of just calling pop_initializing(), is (hopefully remote) + // possibility that the exception happened *before* the + // push_initializing() call in LLSingletonBase's constructor. So only + // remove the stack top if in fact we've pushed something more than the + // previous size. + list_t& list(get_initializing()); + + while (list.size() > size) + { + list.pop_back(); + } + + // as in pop_initializing() + if (list.empty()) + { + MasterList::instance().cleanup_initializing_(); + } +} + //static void LLSingletonBase::log_initializing(const char* verb, const char* name) { diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index e0d75ed72a..de6990efd4 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -55,7 +55,6 @@ private: // This, on the other hand, is a stack whose top indicates the LLSingleton // currently being initialized. static list_t& get_initializing(); - static list_t& get_initializing_from(MasterList*); // Produce a vector of master list, in dependency order. typedef std::vector vec_t; static vec_t dep_sort(); @@ -112,6 +111,9 @@ protected: // That being the case, we control exactly when it happens -- and we can // pop the stack immediately thereafter. void pop_initializing(); + // Remove 'this' from the init stack in case of exception in the + // LLSingleton subclass constructor. + static void reset_initializing(list_t::size_type size); private: // logging static void log_initializing(const char* verb, const char* name); @@ -190,7 +192,15 @@ struct LLSingleton_manage_master void remove(LLSingletonBase* sb) { sb->remove_master(); } void push_initializing(LLSingletonBase* sb) { sb->push_initializing(typeid(T).name()); } void pop_initializing (LLSingletonBase* sb) { sb->pop_initializing(); } - LLSingletonBase::list_t& get_initializing(T*) { return LLSingletonBase::get_initializing(); } + // used for init stack cleanup in case an LLSingleton subclass constructor + // throws an exception + void reset_initializing(LLSingletonBase::list_t::size_type size) + { + LLSingletonBase::reset_initializing(size); + } + // For any LLSingleton subclass except the MasterList, obtain the init + // stack from the MasterList singleton instance. + LLSingletonBase::list_t& get_initializing() { return LLSingletonBase::get_initializing(); } }; // But for the specific case of LLSingletonBase::MasterList, don't. @@ -201,9 +211,14 @@ struct LLSingleton_manage_master void remove(LLSingletonBase*) {} void push_initializing(LLSingletonBase*) {} void pop_initializing (LLSingletonBase*) {} - LLSingletonBase::list_t& get_initializing(LLSingletonBase::MasterList* instance) + // since we never pushed, no need to clean up + void reset_initializing(LLSingletonBase::list_t::size_type size) {} + LLSingletonBase::list_t& get_initializing() { - return LLSingletonBase::get_initializing_from(instance); + // The MasterList shouldn't depend on any other LLSingletons. We'd + // get into trouble if we tried to recursively engage that machinery. + static LLSingletonBase::list_t sDummyList; + return sDummyList; } }; @@ -213,7 +228,12 @@ LLSingletonBase::LLSingletonBase(tag): mCleaned(false), mDeleteSingleton(NULL) { - // Make this the currently-initializing LLSingleton. + // This is the earliest possible point at which we can push this new + // instance onto the init stack. LLSingleton::constructSingleton() can't + // do it before calling the constructor, because it doesn't have an + // instance pointer until the constructor returns. Fortunately this + // constructor is guaranteed to be called before any subclass constructor. + // Make this new instance the currently-initializing LLSingleton. LLSingleton_manage_master().push_initializing(this); } @@ -298,8 +318,27 @@ private: static void constructSingleton(Args&&... args) { sData.mInitState = CONSTRUCTING; - sData.mInstance = new DERIVED_TYPE(std::forward(args)...); - sData.mInitState = INITIALIZING; + auto prev_size = LLSingleton_manage_master().get_initializing().size(); + try + { + sData.mInstance = new DERIVED_TYPE(std::forward(args)...); + sData.mInitState = INITIALIZING; + } + catch (const std::exception& err) + { + logwarns("Error constructing ", demangle(typeid(DERIVED_TYPE).name()).c_str(), + ": ", err.what()); + // There isn't a separate EInitState value meaning "we attempted + // to construct this LLSingleton subclass but could not," so use + // DELETED. That seems slightly more appropriate than UNINITIALIZED. + sData.mInitState = DELETED; + // LLSingletonBase might -- or might not -- have pushed the new + // instance onto the init stack before the exception. Reset the + // init stack to its previous size. + LLSingleton_manage_master().reset_initializing(prev_size); + // propagate the exception + throw; + } } static void finishInitializing() @@ -310,16 +349,41 @@ private: // initialize singleton after constructing it so that it can // reference other singletons which in turn depend on it, thus // breaking cyclic dependencies - sData.mInstance->initSingleton(); - // pop this off stack of initializing singletons - LLSingleton_manage_master().pop_initializing(sData.mInstance); - - // The remaining top of that stack, if any, is an LLSingleton that - // directly depends on DERIVED_TYPE. If getInstance() was called by - // another LLSingleton, rather than from vanilla application code, - // record the dependency. + try + { + sData.mInstance->initSingleton(); + + // pop this off stack of initializing singletons + LLSingleton_manage_master().pop_initializing(sData.mInstance); + + // record the dependency, if any + capture_dependency(); + } + catch (const std::exception& err) + { + logwarns("Error in ", demangle(typeid(DERIVED_TYPE).name()).c_str(), + "::initSingleton(): ", err.what()); + // pop this off stack of initializing singletons here, too + LLSingleton_manage_master().pop_initializing(sData.mInstance); + // and get rid of the instance entirely + deleteSingleton(); + // propagate the exception + throw; + } + } + + // Without this 'using' declaration, the static method we're declaring + // here would hide the base-class method we want it to call. + using LLSingletonBase::capture_dependency; + static void capture_dependency() + { + // By this point, if DERIVED_TYPE was pushed onto the initializing + // stack, it has been popped off. So the top of that stack, if any, is + // an LLSingleton that directly depends on DERIVED_TYPE. If + // getInstance() was called by another LLSingleton, rather than from + // vanilla application code, record the dependency. sData.mInstance->capture_dependency( - LLSingleton_manage_master().get_initializing(sData.mInstance), + LLSingleton_manage_master().get_initializing(), sData.mInitState); } @@ -427,6 +491,7 @@ public: case INITIALIZED: // normal subsequent calls + capture_dependency(); break; case DELETED: -- cgit v1.2.3 From 54b98cb8c1302a1da5779d8968e54e5be641e644 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 20 Aug 2019 12:36:06 -0400 Subject: DRTVWR-493: Defend LL[Param]Singleton against ctor/init exceptions. An exception in the LLSingleton subclass constructor, or in its initSingleton() method, could leave the LLSingleton machinery in a bad state: the failing instance would remain in the MasterList, also on the stack of initializing LLSingletons. Catch exceptions in either and perform relevant cleanup. This problem is highlighted by test programs, in which LL_ERRS throws an exception rather than crashing the whole process. In the relevant catch clauses, clean up the initializing stack BEFORE logging. Otherwise we get tangled up recording bogus dependencies. Move capture_dependency() out of finishInitializing(): it must be called by every valid getInstance() call, both from LLSingleton and LLParamSingleton. Introduce new CONSTRUCTED EInitState value to distinguish "have called the constructor but not yet the initSingleton() method" from "currently within initSingleton() method." This is transient, but we execute the 'switch' on state within that moment. One could argue that the previous enum used INITIALIZING for current CONSTRUCTED, and INITIALIZED meant INITIALIZING too, but this is clearer. Introduce template LLSingletonBase::classname() helper methods to clarify verbose demangle(typeid(stuff).name()) calls. Similarly, introduce LLSingleton::pop_initializing() shorthand method. --- indra/llcommon/llsingleton.cpp | 40 +++++++++------ indra/llcommon/llsingleton.h | 111 +++++++++++++++++++++++++++-------------- 2 files changed, 99 insertions(+), 52 deletions(-) (limited to 'indra') diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp index adf72bf700..88527a3d31 100644 --- a/indra/llcommon/llsingleton.cpp +++ b/indra/llcommon/llsingleton.cpp @@ -150,7 +150,7 @@ void LLSingletonBase::pop_initializing() if (list.empty()) { logerrs("Underflow in stack of currently-initializing LLSingletons at ", - demangle(typeid(*this).name()).c_str(), "::getInstance()"); + classname(this).c_str(), "::getInstance()"); } // Now we know list.back() exists: capture it @@ -172,8 +172,8 @@ void LLSingletonBase::pop_initializing() if (back != this) { logerrs("Push/pop mismatch in stack of currently-initializing LLSingletons: ", - demangle(typeid(*this).name()).c_str(), "::getInstance() trying to pop ", - demangle(typeid(*back).name()).c_str()); + classname(this).c_str(), "::getInstance() trying to pop ", + classname(back).c_str()); } // log AFTER popping so logging singletons don't cry circularity @@ -216,7 +216,7 @@ void LLSingletonBase::log_initializing(const char* verb, const char* name) ri != rend; ++ri) { LLSingletonBase* sb(*ri); - LL_CONT << ' ' << demangle(typeid(*sb).name()); + LL_CONT << ' ' << classname(sb); } LL_ENDL; } @@ -250,7 +250,7 @@ void LLSingletonBase::capture_dependency(list_t& initializing, EInitState initSt // 'found' is an iterator; *found is an LLSingletonBase*; **found // is the actual LLSingletonBase instance. LLSingletonBase* foundp(*found); - out << demangle(typeid(*foundp).name()) << " -> "; + out << classname(foundp) << " -> "; } // We promise to capture dependencies from both the constructor // and the initSingleton() method, so an LLSingleton's instance @@ -264,7 +264,7 @@ void LLSingletonBase::capture_dependency(list_t& initializing, EInitState initSt if (initState == CONSTRUCTING) { logerrs("LLSingleton circularity in Constructor: ", out.str().c_str(), - demangle(typeid(*this).name()).c_str(), ""); + classname(this).c_str(), ""); } else if (it_next == initializing.end()) { @@ -275,14 +275,14 @@ void LLSingletonBase::capture_dependency(list_t& initializing, EInitState initSt // Example: LLNotifications singleton initializes default channels. // Channels register themselves with singleton once done. logdebugs("LLSingleton circularity: ", out.str().c_str(), - demangle(typeid(*this).name()).c_str(), ""); + classname(this).c_str(), ""); } else { // Actual circularity with other singleton (or single singleton is used extensively). // Dependency can be unclear. logwarns("LLSingleton circularity: ", out.str().c_str(), - demangle(typeid(*this).name()).c_str(), ""); + classname(this).c_str(), ""); } } else @@ -295,8 +295,8 @@ void LLSingletonBase::capture_dependency(list_t& initializing, EInitState initSt if (current->mDepends.insert(this).second) { // only log the FIRST time we hit this dependency! - logdebugs(demangle(typeid(*current).name()).c_str(), - " depends on ", demangle(typeid(*this).name()).c_str()); + logdebugs(classname(current).c_str(), + " depends on ", classname(this).c_str()); } } } @@ -355,19 +355,19 @@ void LLSingletonBase::cleanupAll() sp->mCleaned = true; logdebugs("calling ", - demangle(typeid(*sp).name()).c_str(), "::cleanupSingleton()"); + classname(sp).c_str(), "::cleanupSingleton()"); try { sp->cleanupSingleton(); } catch (const std::exception& e) { - logwarns("Exception in ", demangle(typeid(*sp).name()).c_str(), + logwarns("Exception in ", classname(sp).c_str(), "::cleanupSingleton(): ", e.what()); } catch (...) { - logwarns("Unknown exception in ", demangle(typeid(*sp).name()).c_str(), + logwarns("Unknown exception in ", classname(sp).c_str(), "::cleanupSingleton()"); } } @@ -382,7 +382,7 @@ void LLSingletonBase::deleteAll() { // Capture the class name first: in case of exception, don't count on // being able to extract it later. - const std::string name = demangle(typeid(*sp).name()); + const std::string name = classname(sp); try { // Call static method through instance function pointer. @@ -459,7 +459,17 @@ void LLSingletonBase::logerrs(const char* p1, const char* p2, const char* p3, co log(LLError::LEVEL_ERROR, p1, p2, p3, p4); // The other important side effect of LL_ERRS() is // https://www.youtube.com/watch?v=OMG7paGJqhQ (emphasis on OMG) - LLError::crashAndLoop(std::string()); + std::ostringstream out; + out << p1 << p2 << p3 << p4; + auto crash{ LLError::getFatalFunction() }; + if (crash) + { + crash(out.str()); + } + else + { + LLError::crashAndLoop(out.str()); + } } std::string LLSingletonBase::demangle(const char* mangled) diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index de6990efd4..0da6d548ab 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -68,10 +68,11 @@ protected: typedef enum e_init_state { UNINITIALIZED = 0, // must be default-initialized state - CONSTRUCTING, - INITIALIZING, - INITIALIZED, - DELETED + CONSTRUCTING, // within DERIVED_TYPE constructor + CONSTRUCTED, // finished DERIVED_TYPE constructor + INITIALIZING, // within DERIVED_TYPE::initSingleton() + INITIALIZED, // normal case + DELETED // deleteSingleton() or deleteAll() called } EInitState; // Define tag to pass to our template constructor. You can't explicitly @@ -129,6 +130,10 @@ protected: static void logwarns(const char* p1, const char* p2="", const char* p3="", const char* p4=""); static std::string demangle(const char* mangled); + template + static std::string classname() { return demangle(typeid(T).name()); } + template + static std::string classname(T* ptr) { return demangle(typeid(*ptr).name()); } // Default methods in case subclass doesn't declare them. virtual void initSingleton() {} @@ -317,25 +322,28 @@ private: template static void constructSingleton(Args&&... args) { - sData.mInitState = CONSTRUCTING; auto prev_size = LLSingleton_manage_master().get_initializing().size(); + // getInstance() calls are from within constructor + sData.mInitState = CONSTRUCTING; try { sData.mInstance = new DERIVED_TYPE(std::forward(args)...); - sData.mInitState = INITIALIZING; + // we have called constructor, have not yet called initSingleton() + sData.mInitState = CONSTRUCTED; } catch (const std::exception& err) { - logwarns("Error constructing ", demangle(typeid(DERIVED_TYPE).name()).c_str(), + // LLSingletonBase might -- or might not -- have pushed the new + // instance onto the init stack before the exception. Reset the + // init stack to its previous size BEFORE logging so log-machinery + // LLSingletons don't record a dependency on DERIVED_TYPE! + LLSingleton_manage_master().reset_initializing(prev_size); + logwarns("Error constructing ", classname().c_str(), ": ", err.what()); // There isn't a separate EInitState value meaning "we attempted // to construct this LLSingleton subclass but could not," so use // DELETED. That seems slightly more appropriate than UNINITIALIZED. sData.mInitState = DELETED; - // LLSingletonBase might -- or might not -- have pushed the new - // instance onto the init stack before the exception. Reset the - // init stack to its previous size. - LLSingleton_manage_master().reset_initializing(prev_size); // propagate the exception throw; } @@ -343,28 +351,27 @@ private: static void finishInitializing() { - // go ahead and flag ourselves as initialized so we can be - // reentrant during initialization - sData.mInitState = INITIALIZED; - // initialize singleton after constructing it so that it can - // reference other singletons which in turn depend on it, thus - // breaking cyclic dependencies + // getInstance() calls are from within initSingleton() + sData.mInitState = INITIALIZING; try { + // initialize singleton after constructing it so that it can + // reference other singletons which in turn depend on it, thus + // breaking cyclic dependencies sData.mInstance->initSingleton(); + sData.mInitState = INITIALIZED; // pop this off stack of initializing singletons - LLSingleton_manage_master().pop_initializing(sData.mInstance); - - // record the dependency, if any - capture_dependency(); + pop_initializing(); } catch (const std::exception& err) { - logwarns("Error in ", demangle(typeid(DERIVED_TYPE).name()).c_str(), + // pop this off stack of initializing singletons here, too -- + // BEFORE logging, so log-machinery LLSingletons don't record a + // dependency on DERIVED_TYPE! + pop_initializing(); + logwarns("Error in ", classname().c_str(), "::initSingleton(): ", err.what()); - // pop this off stack of initializing singletons here, too - LLSingleton_manage_master().pop_initializing(sData.mInstance); // and get rid of the instance entirely deleteSingleton(); // propagate the exception @@ -372,6 +379,13 @@ private: } } + static void pop_initializing() + { + // route through LLSingleton_manage_master so we Do The Right Thing + // (namely, nothing) for MasterList + LLSingleton_manage_master().pop_initializing(sData.mInstance); + } + // Without this 'using' declaration, the static method we're declaring // here would hide the base-class method we want it to call. using LLSingletonBase::capture_dependency; @@ -458,8 +472,7 @@ public: static void deleteSingleton() { delete sData.mInstance; - sData.mInstance = NULL; - sData.mInitState = DELETED; + // SingletonData state handled by destructor, above } static DERIVED_TYPE* getInstance() @@ -472,38 +485,46 @@ public: case UNINITIALIZED: // should never be uninitialized at this point logerrs("Uninitialized singleton ", - demangle(typeid(DERIVED_TYPE).name()).c_str()); + classname().c_str()); return NULL; case CONSTRUCTING: // here if DERIVED_TYPE's constructor (directly or indirectly) // calls DERIVED_TYPE::getInstance() logerrs("Tried to access singleton ", - demangle(typeid(DERIVED_TYPE).name()).c_str(), + classname().c_str(), " from singleton constructor!"); return NULL; - case INITIALIZING: - // first time through: set to INITIALIZING by - // constructSingleton(), called by sInitializer's constructor + case CONSTRUCTED: + // first time through: set to CONSTRUCTED by + // constructSingleton(), called by sInitializer's constructor; + // still have to call initSingleton() finishInitializing(); break; + case INITIALIZING: + // here if DERIVED_TYPE::initSingleton() (directly or indirectly) + // calls DERIVED_TYPE::getInstance(): go ahead and allow it case INITIALIZED: // normal subsequent calls - capture_dependency(); break; case DELETED: // called after deleteSingleton() logwarns("Trying to access deleted singleton ", - demangle(typeid(DERIVED_TYPE).name()).c_str(), + classname().c_str(), " -- creating new instance"); + // This recovery sequence is NOT thread-safe! We would need a + // recursive_mutex a la LLParamSingleton. constructSingleton(); finishInitializing(); break; } + // record the dependency, if any: check if we got here from another + // LLSingleton's constructor or initSingleton() method + capture_dependency(); return sData.mInstance; } @@ -589,7 +610,7 @@ public: if (super::sData.mInitState != super::UNINITIALIZED) { super::logerrs("Tried to initialize singleton ", - super::demangle(typeid(DERIVED_TYPE).name()).c_str(), + super::template classname().c_str(), " twice!"); } else @@ -609,22 +630,38 @@ public: { case super::UNINITIALIZED: super::logerrs("Uninitialized param singleton ", - super::demangle(typeid(DERIVED_TYPE).name()).c_str()); + super::template classname().c_str()); break; case super::CONSTRUCTING: super::logerrs("Tried to access param singleton ", - super::demangle(typeid(DERIVED_TYPE).name()).c_str(), + super::template classname().c_str(), " from singleton constructor!"); break; + case super::CONSTRUCTED: + // Should never happen!? The CONSTRUCTED state is specifically to + // navigate through LLSingleton::SingletonInitializer getting + // constructed (once) before LLSingleton::getInstance()'s switch + // on mInitState. But our initParamSingleton() method calls + // constructSingleton() and then calls finishInitializing(), which + // immediately sets INITIALIZING. Why are we here? + super::logerrs("Param singleton ", + super::template classname().c_str(), + "::initSingleton() not yet called"); + break; + case super::INITIALIZING: + // As with LLSingleton, explicitly permit circular calls from + // within initSingleton() case super::INITIALIZED: + // for any valid call, capture dependencies + super::capture_dependency(); return super::sData.mInstance; case super::DELETED: super::logerrs("Trying to access deleted param singleton ", - super::demangle(typeid(DERIVED_TYPE).name()).c_str()); + super::template classname().c_str()); break; } -- cgit v1.2.3 From 5b5eb55f0c999a55cd0df1dd983629967d447b6c Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 20 Aug 2019 12:48:21 -0400 Subject: DRTVWR-493: Clarify capturing LLError::getFatalFunction() in a var. VS 2013 thought we were storing an initialization-list. --- indra/llcommon/llsingleton.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra') diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp index 88527a3d31..c45c144570 100644 --- a/indra/llcommon/llsingleton.cpp +++ b/indra/llcommon/llsingleton.cpp @@ -461,7 +461,7 @@ void LLSingletonBase::logerrs(const char* p1, const char* p2, const char* p3, co // https://www.youtube.com/watch?v=OMG7paGJqhQ (emphasis on OMG) std::ostringstream out; out << p1 << p2 << p3 << p4; - auto crash{ LLError::getFatalFunction() }; + auto crash = LLError::getFatalFunction(); if (crash) { crash(out.str()); -- cgit v1.2.3