From 802b52f3044dfaaec3406d654c091d626955f569 Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev Date: Tue, 22 Sep 2020 23:38:23 +0300 Subject: SL-13979 Crash of logging system at LLError::Settings::getInstance() LLSingleton depends onto logging system, having logging system be based on LLSingleton causes crashes and deadlocks --- indra/llcommon/llerror.cpp | 84 +++++++++++++++++++++------------------------- 1 file changed, 38 insertions(+), 46 deletions(-) (limited to 'indra/llcommon/llerror.cpp') diff --git a/indra/llcommon/llerror.cpp b/indra/llcommon/llerror.cpp index 411412c883..f876b8ee4a 100644 --- a/indra/llcommon/llerror.cpp +++ b/indra/llcommon/llerror.cpp @@ -332,12 +332,9 @@ namespace LLError } // huh, that's odd, we should see one or the other prefix -- but don't // try to log unless logging is already initialized - if (is_available()) - { - // in Python, " or ".join(vector) -- but in C++, a PITB - LL_DEBUGS() << "Did not see 'class' or 'struct' prefix on '" - << name << "'" << LL_ENDL; - } + // in Python, " or ".join(vector) -- but in C++, a PITB + LL_DEBUGS() << "Did not see 'class' or 'struct' prefix on '" + << name << "'" << LL_ENDL; return name; #else // neither GCC nor Visual Studio @@ -438,9 +435,12 @@ namespace typedef std::vector Recorders; typedef std::vector CallSiteVector; - class Globals : public LLSingleton + class Globals { - LLSINGLETON(Globals); + public: + static Globals* getInstance(); + protected: + Globals(); public: std::ostringstream messageStream; bool messageStreamInUse; @@ -460,6 +460,16 @@ namespace { } + Globals* Globals::getInstance() + { + // According to C++11 Function-Local Initialization + // of static variables is supposed to be thread safe + // without risk of deadlocks. + static Globals inst; + + return &inst; + } + void Globals::addCallSite(LLError::CallSite& site) { callSites.push_back(&site); @@ -512,14 +522,17 @@ namespace LLError typedef LLPointer SettingsConfigPtr; - class Settings : public LLSingleton + class Settings { - LLSINGLETON(Settings); + public: + static Settings* getInstance(); + protected: + Settings(); public: SettingsConfigPtr getSettingsConfig(); void reset(); - SettingsStoragePtr saveAndReset(); + SettingsStoragePtr saveAndReset(); void restore(SettingsStoragePtr pSettingsStorage); private: @@ -553,6 +566,16 @@ namespace LLError { } + Settings* Settings::getInstance() + { + // According to C++11 Function-Local Initialization + // of static variables is supposed to be thread safe + // without risk of deadlocks. + static Settings inst; + + return &inst; + } + SettingsConfigPtr Settings::getSettingsConfig() { return mSettingsConfig; @@ -577,11 +600,6 @@ namespace LLError SettingsConfigPtr newSettingsConfig(dynamic_cast(pSettingsStorage.get())); mSettingsConfig = newSettingsConfig; } - - bool is_available() - { - return Settings::instanceExists() && Globals::instanceExists(); - } } namespace LLError @@ -1028,7 +1046,7 @@ namespace LLError std::pair, Recorders::iterator> findRecorderPos() { - SettingsConfigPtr s = Settings::instance().getSettingsConfig(); + SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); // Since we promise to return an iterator, use a classic iterator // loop. auto end{s->mRecorders.end()}; @@ -1071,7 +1089,7 @@ namespace LLError auto found = findRecorderPos(); if (found.first) { - SettingsConfigPtr s = Settings::instance().getSettingsConfig(); + SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); s->mRecorders.erase(found.second); } return bool(found.first); @@ -1307,14 +1325,6 @@ namespace LLError return false; } - // If we hit a logging request very late during shutdown processing, - // when either of the relevant LLSingletons has already been deleted, - // DO NOT resurrect them. - if (Settings::wasDeleted() || Globals::wasDeleted()) - { - return false; - } - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); s->mShouldLogCallCounter++; @@ -1353,10 +1363,8 @@ namespace LLError std::ostringstream* Log::out() { LLMutexTrylock lock(getMutex(),5); - // If we hit a logging request very late during shutdown processing, - // when either of the relevant LLSingletons has already been deleted, - // DO NOT resurrect them. - if (lock.isLocked() && ! (Settings::wasDeleted() || Globals::wasDeleted())) + + if (lock.isLocked()) { Globals* g = Globals::getInstance(); @@ -1378,14 +1386,6 @@ namespace LLError return; } - // If we hit a logging request very late during shutdown processing, - // when either of the relevant LLSingletons has already been deleted, - // DO NOT resurrect them. - if (Settings::wasDeleted() || Globals::wasDeleted()) - { - return; - } - if(strlen(out->str().c_str()) < 128) { strcpy(message, out->str().c_str()); @@ -1418,14 +1418,6 @@ namespace LLError return; } - // If we hit a logging request very late during shutdown processing, - // when either of the relevant LLSingletons has already been deleted, - // DO NOT resurrect them. - if (Settings::wasDeleted() || Globals::wasDeleted()) - { - return; - } - Globals* g = Globals::getInstance(); SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); -- cgit v1.2.3 From 95e724f3deb856b0b69a3850ceb938c81d1f0321 Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev Date: Mon, 26 Oct 2020 21:04:10 +0200 Subject: SL-14004 Coalesce viewer's LLError::Settings and Globals --- indra/llcommon/llerror.cpp | 265 +++++++++++++++++++++------------------------ 1 file changed, 122 insertions(+), 143 deletions(-) (limited to 'indra/llcommon/llerror.cpp') diff --git a/indra/llcommon/llerror.cpp b/indra/llcommon/llerror.cpp index f876b8ee4a..781c41f3de 100644 --- a/indra/llcommon/llerror.cpp +++ b/indra/llcommon/llerror.cpp @@ -435,6 +435,60 @@ namespace typedef std::vector Recorders; typedef std::vector CallSiteVector; + class SettingsConfig : public LLRefCount + { + friend class Globals; + + public: + virtual ~SettingsConfig(); + + LLError::ELevel mDefaultLevel; + + bool mLogAlwaysFlush; + + U32 mEnabledLogTypesMask; + + LevelMap mFunctionLevelMap; + LevelMap mClassLevelMap; + LevelMap mFileLevelMap; + LevelMap mTagLevelMap; + std::map mUniqueLogMessages; + + LLError::FatalFunction mCrashFunction; + LLError::TimeFunction mTimeFunction; + + Recorders mRecorders; + + int mShouldLogCallCounter; + + private: + SettingsConfig(); + }; + + typedef LLPointer SettingsConfigPtr; + + SettingsConfig::SettingsConfig() + : LLRefCount(), + mDefaultLevel(LLError::LEVEL_DEBUG), + mLogAlwaysFlush(true), + mEnabledLogTypesMask(255), + mFunctionLevelMap(), + mClassLevelMap(), + mFileLevelMap(), + mTagLevelMap(), + mUniqueLogMessages(), + mCrashFunction(NULL), + mTimeFunction(NULL), + mRecorders(), + mShouldLogCallCounter(0) + { + } + + SettingsConfig::~SettingsConfig() + { + mRecorders.clear(); + } + class Globals { public: @@ -449,14 +503,21 @@ namespace void addCallSite(LLError::CallSite&); void invalidateCallSites(); + SettingsConfigPtr getSettingsConfig(); + + void resetSettingsConfig(); + LLError::SettingsStoragePtr saveAndResetSettingsConfig(); + void restore(LLError::SettingsStoragePtr pSettingsStorage); private: CallSiteVector callSites; + SettingsConfigPtr mSettingsConfig; }; Globals::Globals() : messageStream(), messageStreamInUse(false), - callSites() + callSites(), + mSettingsConfig(new SettingsConfig()) { } @@ -486,120 +547,31 @@ namespace callSites.clear(); } -} - -namespace LLError -{ - class SettingsConfig : public LLRefCount - { - friend class Settings; - - public: - virtual ~SettingsConfig(); - - LLError::ELevel mDefaultLevel; - - bool mLogAlwaysFlush; - - U32 mEnabledLogTypesMask; - - LevelMap mFunctionLevelMap; - LevelMap mClassLevelMap; - LevelMap mFileLevelMap; - LevelMap mTagLevelMap; - std::map mUniqueLogMessages; - - LLError::FatalFunction mCrashFunction; - LLError::TimeFunction mTimeFunction; - - Recorders mRecorders; - - int mShouldLogCallCounter; - - private: - SettingsConfig(); - }; - - typedef LLPointer SettingsConfigPtr; - - class Settings - { - public: - static Settings* getInstance(); - protected: - Settings(); - public: - SettingsConfigPtr getSettingsConfig(); - - void reset(); - SettingsStoragePtr saveAndReset(); - void restore(SettingsStoragePtr pSettingsStorage); - - private: - SettingsConfigPtr mSettingsConfig; - }; - - SettingsConfig::SettingsConfig() - : LLRefCount(), - mDefaultLevel(LLError::LEVEL_DEBUG), - mLogAlwaysFlush(true), - mEnabledLogTypesMask(255), - mFunctionLevelMap(), - mClassLevelMap(), - mFileLevelMap(), - mTagLevelMap(), - mUniqueLogMessages(), - mCrashFunction(NULL), - mTimeFunction(NULL), - mRecorders(), - mShouldLogCallCounter(0) - { - } - SettingsConfig::~SettingsConfig() - { - mRecorders.clear(); - } - - Settings::Settings(): - mSettingsConfig(new SettingsConfig()) - { - } - - Settings* Settings::getInstance() + SettingsConfigPtr Globals::getSettingsConfig() { - // According to C++11 Function-Local Initialization - // of static variables is supposed to be thread safe - // without risk of deadlocks. - static Settings inst; - - return &inst; + return mSettingsConfig; } - SettingsConfigPtr Settings::getSettingsConfig() - { - return mSettingsConfig; - } - - void Settings::reset() - { - Globals::getInstance()->invalidateCallSites(); - mSettingsConfig = new SettingsConfig(); - } + void Globals::resetSettingsConfig() + { + invalidateCallSites(); + mSettingsConfig = new SettingsConfig(); + } - SettingsStoragePtr Settings::saveAndReset() - { - SettingsStoragePtr oldSettingsConfig(mSettingsConfig.get()); - reset(); - return oldSettingsConfig; - } + LLError::SettingsStoragePtr Globals::saveAndResetSettingsConfig() + { + LLError::SettingsStoragePtr oldSettingsConfig(mSettingsConfig.get()); + resetSettingsConfig(); + return oldSettingsConfig; + } - void Settings::restore(SettingsStoragePtr pSettingsStorage) - { - Globals::getInstance()->invalidateCallSites(); - SettingsConfigPtr newSettingsConfig(dynamic_cast(pSettingsStorage.get())); - mSettingsConfig = newSettingsConfig; - } + void Globals::restore(LLError::SettingsStoragePtr pSettingsStorage) + { + invalidateCallSites(); + SettingsConfigPtr newSettingsConfig(dynamic_cast(pSettingsStorage.get())); + mSettingsConfig = newSettingsConfig; + } } namespace LLError @@ -723,7 +695,7 @@ namespace void commonInit(const std::string& user_dir, const std::string& app_dir, bool log_to_stderr = true) { - LLError::Settings::getInstance()->reset(); + Globals::getInstance()->resetSettingsConfig(); LLError::setDefaultLevel(LLError::LEVEL_INFO); LLError::setAlwaysFlush(true); @@ -765,13 +737,13 @@ namespace LLError void setFatalFunction(const FatalFunction& f) { - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); s->mCrashFunction = f; } FatalFunction getFatalFunction() { - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); return s->mCrashFunction; } @@ -782,72 +754,77 @@ namespace LLError void setTimeFunction(TimeFunction f) { - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); s->mTimeFunction = f; } void setDefaultLevel(ELevel level) { - Globals::getInstance()->invalidateCallSites(); - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + Globals *g = Globals::getInstance(); + g->invalidateCallSites(); + SettingsConfigPtr s = g->getSettingsConfig(); s->mDefaultLevel = level; } ELevel getDefaultLevel() { - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); return s->mDefaultLevel; } void setAlwaysFlush(bool flush) { - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); s->mLogAlwaysFlush = flush; } bool getAlwaysFlush() { - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); return s->mLogAlwaysFlush; } void setEnabledLogTypesMask(U32 mask) { - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); s->mEnabledLogTypesMask = mask; } U32 getEnabledLogTypesMask() { - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); return s->mEnabledLogTypesMask; } void setFunctionLevel(const std::string& function_name, ELevel level) { - Globals::getInstance()->invalidateCallSites(); - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + Globals *g = Globals::getInstance(); + g->invalidateCallSites(); + SettingsConfigPtr s = g->getSettingsConfig(); s->mFunctionLevelMap[function_name] = level; } void setClassLevel(const std::string& class_name, ELevel level) { - Globals::getInstance()->invalidateCallSites(); - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + Globals *g = Globals::getInstance(); + g->invalidateCallSites(); + SettingsConfigPtr s = g->getSettingsConfig(); s->mClassLevelMap[class_name] = level; } void setFileLevel(const std::string& file_name, ELevel level) { - Globals::getInstance()->invalidateCallSites(); - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + Globals *g = Globals::getInstance(); + g->invalidateCallSites(); + SettingsConfigPtr s = g->getSettingsConfig(); s->mFileLevelMap[file_name] = level; } void setTagLevel(const std::string& tag_name, ELevel level) { - Globals::getInstance()->invalidateCallSites(); - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + Globals *g = Globals::getInstance(); + g->invalidateCallSites(); + SettingsConfigPtr s = g->getSettingsConfig(); s->mTagLevelMap[tag_name] = level; } @@ -892,8 +869,9 @@ namespace LLError { void configure(const LLSD& config) { - Globals::getInstance()->invalidateCallSites(); - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + Globals *g = Globals::getInstance(); + g->invalidateCallSites(); + SettingsConfigPtr s = g->getSettingsConfig(); s->mFunctionLevelMap.clear(); s->mClassLevelMap.clear(); @@ -1020,7 +998,7 @@ namespace LLError { return; } - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); s->mRecorders.push_back(recorder); } @@ -1030,7 +1008,7 @@ namespace LLError { return; } - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); s->mRecorders.erase(std::remove(s->mRecorders.begin(), s->mRecorders.end(), recorder), s->mRecorders.end()); } @@ -1046,7 +1024,7 @@ namespace LLError std::pair, Recorders::iterator> findRecorderPos() { - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); // Since we promise to return an iterator, use a classic iterator // loop. auto end{s->mRecorders.end()}; @@ -1089,7 +1067,7 @@ namespace LLError auto found = findRecorderPos(); if (found.first) { - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); s->mRecorders.erase(found.second); } return bool(found.first); @@ -1187,7 +1165,7 @@ namespace void writeToRecorders(const LLError::CallSite& site, const std::string& message) { LLError::ELevel level = site.mLevel; - LLError::SettingsConfigPtr s = LLError::Settings::getInstance()->getSettingsConfig(); + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); std::string escaped_message; @@ -1325,7 +1303,8 @@ namespace LLError return false; } - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + Globals *g = Globals::getInstance(); + SettingsConfigPtr s = g->getSettingsConfig(); s->mShouldLogCallCounter++; @@ -1355,7 +1334,7 @@ namespace LLError : false); site.mCached = true; - Globals::getInstance()->addCallSite(site); + g->addCallSite(site); return site.mShouldLog = site.mLevel >= compareLevel; } @@ -1419,7 +1398,7 @@ namespace LLError } Globals* g = Globals::getInstance(); - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + SettingsConfigPtr s = g->getSettingsConfig(); std::string message = out->str(); if (out == &g->messageStream) @@ -1478,12 +1457,12 @@ namespace LLError { SettingsStoragePtr saveAndResetSettings() { - return Settings::getInstance()->saveAndReset(); + return Globals::getInstance()->saveAndResetSettingsConfig(); } void restoreSettings(SettingsStoragePtr pSettingsStorage) { - return Settings::getInstance()->restore(pSettingsStorage); + return Globals::getInstance()->restore(pSettingsStorage); } std::string removePrefix(std::string& s, const std::string& p) @@ -1529,7 +1508,7 @@ namespace LLError int shouldLogCallCount() { - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); return s->mShouldLogCallCounter; } @@ -1707,8 +1686,8 @@ bool debugLoggingEnabled(const std::string& tag) { return false; } - - LLError::SettingsConfigPtr s = LLError::Settings::getInstance()->getSettingsConfig(); + + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); LLError::ELevel level = LLError::LEVEL_DEBUG; bool res = checkLevelMap(s->mTagLevelMap, tag, level); return res; -- cgit v1.2.3 From 52c0776afdcd75ba8ebadc6b45d8611564e3b833 Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev Date: Mon, 16 Nov 2020 22:11:19 +0200 Subject: SL-14340 Fix crash on llerror's recorders --- indra/llcommon/llerror.cpp | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) (limited to 'indra/llcommon/llerror.cpp') diff --git a/indra/llcommon/llerror.cpp b/indra/llcommon/llerror.cpp index 781c41f3de..012ec08f07 100644 --- a/indra/llcommon/llerror.cpp +++ b/indra/llcommon/llerror.cpp @@ -458,6 +458,7 @@ namespace LLError::TimeFunction mTimeFunction; Recorders mRecorders; + LLMutex mRecorderMutex; int mShouldLogCallCounter; @@ -480,6 +481,7 @@ namespace mCrashFunction(NULL), mTimeFunction(NULL), mRecorders(), + mRecorderMutex(), mShouldLogCallCounter(0) { } @@ -999,6 +1001,7 @@ namespace LLError return; } SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); + LLMutexLock lock(&s->mRecorderMutex); s->mRecorders.push_back(recorder); } @@ -1009,6 +1012,7 @@ namespace LLError return; } SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); + LLMutexLock lock(&s->mRecorderMutex); s->mRecorders.erase(std::remove(s->mRecorders.begin(), s->mRecorders.end(), recorder), s->mRecorders.end()); } @@ -1020,11 +1024,12 @@ namespace LLError // with a Recorders::iterator indicating the position of that entry in // mRecorders. The shared_ptr might be empty (operator!() returns true) if // there was no such RECORDER subclass instance in mRecorders. + // + // NOTE!!! Requires external mutex lock!!! template std::pair, Recorders::iterator> - findRecorderPos() + findRecorderPos(SettingsConfigPtr &s) { - SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); // Since we promise to return an iterator, use a classic iterator // loop. auto end{s->mRecorders.end()}; @@ -1055,7 +1060,9 @@ namespace LLError template boost::shared_ptr findRecorder() { - return findRecorderPos().first; + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); + LLMutexLock lock(&s->mRecorderMutex); + return findRecorderPos(s).first; } // Remove an entry from SettingsConfig::mRecorders whose RecorderPtr @@ -1064,10 +1071,11 @@ namespace LLError template bool removeRecorder() { - auto found = findRecorderPos(); + SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); + LLMutexLock lock(&s->mRecorderMutex); + auto found = findRecorderPos(s); if (found.first) { - SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); s->mRecorders.erase(found.second); } return bool(found.first); @@ -1168,7 +1176,8 @@ namespace SettingsConfigPtr s = Globals::getInstance()->getSettingsConfig(); std::string escaped_message; - + + LLMutexLock lock(&s->mRecorderMutex); for (Recorders::const_iterator i = s->mRecorders.begin(); i != s->mRecorders.end(); ++i) -- cgit v1.2.3