From 6f6c8fa5ff6619b123f368d9137c11ed679a5349 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 28 Jun 2016 18:27:39 -0400 Subject: DRTVWR-418: Double coroutine stack size for 64-bit builds on the advice of NickyD. --- indra/llcommon/llcoros.cpp | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'indra/llcommon/llcoros.cpp') diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index d16bf0160b..290abbf45c 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -100,7 +100,11 @@ LLCoros::LLCoros(): // Previously we used // boost::context::guarded_stack_allocator::default_stacksize(); // empirically this is 64KB on Windows and Linux. Try quadrupling. +#if WORD_SIZE == 64 + mStackSize(512*1024) +#else mStackSize(256*1024) +#endif { // Register our cleanup() method for "mainloop" ticks LLEventPumps::instance().obtain("mainloop").listen( -- cgit v1.2.3 From 4d10172d8b2c72fa809e322a3b4ff326b19ff340 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 18 Aug 2016 17:33:44 -0400 Subject: MAINT-5011: Catch unhandled exceptions in LLCoros coroutines. Wrap coroutine call in try/catch in top-level coroutine wrapper function LLCoros::toplevel(). Distinguish exception classes derived from LLContinueError (log and continue) from all others (crash with LL_ERRS). Enhance CRASH_ON_UNHANDLED_EXCEPTIONS() and LOG_UNHANDLED_EXCEPTIONS() macros to accept a context string to supplement the log message. This lets us replace many places that called boost::current_exception_diagnostic_information() with LOG_UNHANDLED_EXCEPTIONS() instead, since the explicit calls were mostly to log supplemental information. Provide supplemental information (coroutine name, function parameters) for some of the previous LOG_UNHANDLED_EXCEPTIONS() calls. This information duplicates LL_DEBUGS() information at the top of these functions, but in a typical log file we wouldn't see the LL_DEBUGS() message. Eliminate a few catch (std::exception e) clauses: the information we get from boost::current_exception_diagnostic_information() in a catch (...) clause makes it unnecessary to distinguish. In a few cases, add a final 'throw;' to a catch (...) clause: having logged the local context info, propagate the exception to be caught by higher-level try/catch. In a couple places, couldn't resist reconciling indentation within a particular function: tabs where the rest of the function uses tabs, spaces where the rest of the function uses spaces. In LLLogin::Impl::loginCoro(), eliminate some confusing comments about an array of rewritten URIs that date back to a long-deleted implementation. --- indra/llcommon/llcoros.cpp | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) (limited to 'indra/llcommon/llcoros.cpp') diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index d16bf0160b..4ee8e6d796 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -38,6 +38,7 @@ #include "llevents.h" #include "llerror.h" #include "stringize.h" +#include "llexception.h" // do nothing, when we need nothing done void LLCoros::no_cleanup(CoroData*) {} @@ -235,7 +236,18 @@ void LLCoros::toplevel(coro::self& self, CoroData* data, const callable_t& calla // capture the 'self' param in CoroData data->mSelf = &self; // run the code the caller actually wants in the coroutine - callable(); + try + { + callable(); + } + catch (const LLContinueError& e) + { + LOG_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << data->mName)); + } + catch (...) + { + CRASH_ON_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << data->mName)); + } // This cleanup isn't perfectly symmetrical with the way we initially set // data->mPrev, but this is our last chance to reset mCurrentCoro. sCurrentCoro.reset(data->mPrev); -- cgit v1.2.3 From 17382b22e0fff078703a667cfd199db7d079ddbd Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 25 Aug 2016 14:04:36 -0400 Subject: MAINT-5011: Remove unreferenced param name to avoid fatal warning --- indra/llcommon/llcoros.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/llcommon/llcoros.cpp') diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index 4ee8e6d796..7f4c1780b8 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -240,7 +240,7 @@ void LLCoros::toplevel(coro::self& self, CoroData* data, const callable_t& calla { callable(); } - catch (const LLContinueError& e) + catch (const LLContinueError&) { LOG_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << data->mName)); } -- cgit v1.2.3 From abfe05c1b3d24ed820cf084ece3f3610ec35fa21 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 26 Aug 2016 14:20:11 -0400 Subject: MAINT-5011: Add comments to LLCoros::toplevel() exception handlers. --- indra/llcommon/llcoros.cpp | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'indra/llcommon/llcoros.cpp') diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index 7f4c1780b8..4db63937aa 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -242,10 +242,15 @@ void LLCoros::toplevel(coro::self& self, CoroData* data, const callable_t& calla } catch (const LLContinueError&) { + // Any uncaught exception derived from LLContinueError will be caught + // here and logged. This coroutine will terminate but the rest of the + // viewer will carry on. LOG_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << data->mName)); } catch (...) { + // Any OTHER kind of uncaught exception will cause the viewer to + // crash, hopefully informatively. CRASH_ON_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << data->mName)); } // This cleanup isn't perfectly symmetrical with the way we initially set -- cgit v1.2.3 From f931f6ef521527a929ceddf1354c7e2d85b35b63 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sat, 3 Sep 2016 11:39:17 -0400 Subject: MAINT-5232: Add LLCoros::get_id() to identify the running coroutine. Change the module-static thread_specific_ptr to a function-static thread_specific_ptr so it will be initialized on demand -- since LLSingleton will need to rely on get_id(). Note that since LLCoros isa LLSingleton, we must take great care to avoid circularity. Introduce a private helper class LLCoros::Current to obtain and bind that thread_specific_ptr. Change all existing internal references from the static thread_specific_ptr to the new Current helper class. --- indra/llcommon/llcoros.cpp | 58 ++++++++++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 23 deletions(-) (limited to 'indra/llcommon/llcoros.cpp') diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index d16bf0160b..e84fa6aea0 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -44,16 +44,25 @@ void LLCoros::no_cleanup(CoroData*) {} // CoroData for the currently-running coroutine. Use a thread_specific_ptr // because each thread potentially has its own distinct pool of coroutines. -// This thread_specific_ptr does NOT own the CoroData object! That's owned by -// LLCoros::mCoros. It merely identifies it. For this reason we instantiate -// it with a no-op cleanup function. -boost::thread_specific_ptr -LLCoros::sCurrentCoro(LLCoros::no_cleanup); +LLCoros::Current::Current() +{ + // Use a function-static instance so this thread_specific_ptr is + // instantiated on demand. Since we happen to know it's consumed by + // LLSingleton, this is likely to happen before the runtime has finished + // initializing module-static data. For the same reason, we can't package + // this pointer in an LLSingleton. + + // This thread_specific_ptr does NOT own the CoroData object! That's owned + // by LLCoros::mCoros. It merely identifies it. For this reason we + // instantiate it with a no-op cleanup function. + static boost::thread_specific_ptr sCurrent(LLCoros::no_cleanup); + mCurrent = &sCurrent; +} //static LLCoros::CoroData& LLCoros::get_CoroData(const std::string& caller) { - CoroData* current = sCurrentCoro.get(); + CoroData* current = Current(); if (! current) { LL_ERRS("LLCoros") << "Calling " << caller << " from non-coroutine context!" << LL_ENDL; @@ -79,20 +88,23 @@ bool LLCoros::get_consuming() return get_CoroData("get_consuming()").mConsuming; } -llcoro::Suspending::Suspending(): - mSuspended(LLCoros::sCurrentCoro.get()) +llcoro::Suspending::Suspending() { - // Revert mCurrentCoro to the value it had at the moment we last switched + LLCoros::Current current; + // Remember currently-running coroutine: we're about to suspend it. + mSuspended = current; + // Revert Current to the value it had at the moment we last switched // into this coroutine. - LLCoros::sCurrentCoro.reset(mSuspended->mPrev); + current.reset(mSuspended->mPrev); } llcoro::Suspending::~Suspending() { + LLCoros::Current current; // Okay, we're back, update our mPrev - mSuspended->mPrev = LLCoros::sCurrentCoro.get(); - // and reinstate our sCurrentCoro. - LLCoros::sCurrentCoro.reset(mSuspended); + mSuspended->mPrev = current; + // and reinstate our Current. + current.reset(mSuspended); } LLCoros::LLCoros(): @@ -212,7 +224,7 @@ bool LLCoros::kill(const std::string& name) std::string LLCoros::getName() const { - CoroData* current = sCurrentCoro.get(); + CoroData* current = Current(); if (! current) { // not in a coroutine @@ -228,8 +240,8 @@ void LLCoros::setStackSize(S32 stacksize) } // Top-level wrapper around caller's coroutine callable. This function accepts -// the coroutine library's implicit coro::self& parameter and sets sCurrentSelf -// but does not pass it down to the caller's callable. +// the coroutine library's implicit coro::self& parameter and saves it, but +// does not pass it down to the caller's callable. void LLCoros::toplevel(coro::self& self, CoroData* data, const callable_t& callable) { // capture the 'self' param in CoroData @@ -237,8 +249,8 @@ void LLCoros::toplevel(coro::self& self, CoroData* data, const callable_t& calla // run the code the caller actually wants in the coroutine callable(); // This cleanup isn't perfectly symmetrical with the way we initially set - // data->mPrev, but this is our last chance to reset mCurrentCoro. - sCurrentCoro.reset(data->mPrev); + // data->mPrev, but this is our last chance to reset Current. + Current().reset(data->mPrev); } /***************************************************************************** @@ -261,7 +273,7 @@ LLCoros::CoroData::CoroData(CoroData* prev, const std::string& name, mPrev(prev), mName(name), // Wrap the caller's callable in our toplevel() function so we can manage - // sCurrentCoro appropriately at startup and shutdown of each coroutine. + // Current appropriately at startup and shutdown of each coroutine. mCoro(boost::bind(toplevel, _1, this, callable), stacksize), // don't consume events unless specifically directed mConsuming(false), @@ -272,13 +284,13 @@ LLCoros::CoroData::CoroData(CoroData* prev, const std::string& name, std::string LLCoros::launch(const std::string& prefix, const callable_t& callable) { std::string name(generateDistinctName(prefix)); - // pass the current value of sCurrentCoro as previous context - CoroData* newCoro = new CoroData(sCurrentCoro.get(), name, - callable, mStackSize); + Current current; + // pass the current value of Current as previous context + CoroData* newCoro = new CoroData(current, name, callable, mStackSize); // Store it in our pointer map mCoros.insert(name, newCoro); // also set it as current - sCurrentCoro.reset(newCoro); + current.reset(newCoro); /* Run the coroutine until its first wait, then return here */ (newCoro->mCoro)(std::nothrow); return name; -- cgit v1.2.3 From 72f4c72001789ac8de425c6d0a5f59b0ffda3726 Mon Sep 17 00:00:00 2001 From: Oz Linden Date: Tue, 6 Sep 2016 10:21:28 -0400 Subject: downgrade spammy LLCoros logging to DEBUG --- indra/llcommon/llcoros.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'indra/llcommon/llcoros.cpp') diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index d16bf0160b..5bbce4325b 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -131,9 +131,9 @@ bool LLCoros::cleanup(const LLSD&) if ((previousCount < 5) || !(previousCount % 50)) { if (previousCount < 5) - LL_INFOS("LLCoros") << "LLCoros: cleaning up coroutine " << mi->first << LL_ENDL; + LL_DEBUGS("LLCoros") << "LLCoros: cleaning up coroutine " << mi->first << LL_ENDL; else - LL_INFOS("LLCoros") << "LLCoros: cleaning up coroutine " << mi->first << "("<< previousCount << ")" << LL_ENDL; + LL_DEBUGS("LLCoros") << "LLCoros: cleaning up coroutine " << mi->first << "("<< previousCount << ")" << LL_ENDL; } // The erase() call will invalidate its passed iterator value -- @@ -185,9 +185,9 @@ std::string LLCoros::generateDistinctName(const std::string& prefix) const if ((previousCount < 5) || !(previousCount % 50)) { if (previousCount < 5) - LL_INFOS("LLCoros") << "LLCoros: launching coroutine " << name << LL_ENDL; + LL_DEBUGS("LLCoros") << "LLCoros: launching coroutine " << name << LL_ENDL; else - LL_INFOS("LLCoros") << "LLCoros: launching coroutine " << name << "(" << previousCount << ")" << LL_ENDL; + LL_DEBUGS("LLCoros") << "LLCoros: launching coroutine " << name << "(" << previousCount << ")" << LL_ENDL; } @@ -223,7 +223,7 @@ std::string LLCoros::getName() const void LLCoros::setStackSize(S32 stacksize) { - LL_INFOS("LLCoros") << "Setting coroutine stack size to " << stacksize << LL_ENDL; + LL_DEBUGS("LLCoros") << "Setting coroutine stack size to " << stacksize << LL_ENDL; mStackSize = stacksize; } -- cgit v1.2.3 From a601c559e87d4f2d8b7a2b419b7e7b22cff37121 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 6 Sep 2016 12:08:38 -0400 Subject: MAINT-5232: Ensure that llcoro::get_id() returns distinct values. Until now, the "main coroutine" (the initial context) of each thread left LLCoros::Current() NULL. The trouble with that is that llcoro::get_id() returns that CoroData* as an opaque token, and we want distinct values for every stack in the process. That would not be true if the "main coroutine" on thread A returned the same value (NULL) as the "main coroutine" on thread B, and so forth. Give each thread's "main coroutine" a dummy heap CoroData instance of its own. --- indra/llcommon/llcoros.cpp | 58 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 13 deletions(-) (limited to 'indra/llcommon/llcoros.cpp') diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index e84fa6aea0..7ff5feac68 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -39,7 +39,12 @@ #include "llerror.h" #include "stringize.h" -// do nothing, when we need nothing done +namespace { +void no_op() {} +} // anonymous namespace + +// Do nothing, when we need nothing done. This is a static member of LLCoros +// because CoroData is a private nested class. void LLCoros::no_cleanup(CoroData*) {} // CoroData for the currently-running coroutine. Use a thread_specific_ptr @@ -56,6 +61,35 @@ LLCoros::Current::Current() // by LLCoros::mCoros. It merely identifies it. For this reason we // instantiate it with a no-op cleanup function. static boost::thread_specific_ptr sCurrent(LLCoros::no_cleanup); + + // If this is the first time we're accessing sCurrent for the running + // thread, its get() will be NULL. This could be a problem, in that + // llcoro::get_id() would return the same (NULL) token value for the "main + // coroutine" in every thread, whereas what we really want is a distinct + // value for every distinct stack in the process. So if get() is NULL, + // give it a heap CoroData: this ensures that llcoro::get_id() will return + // distinct values. + // This tactic is "leaky": sCurrent explicitly does not destroy any + // CoroData to which it points, and we do NOT enter these "main coroutine" + // CoroData instances in the LLCoros::mCoros map. They are dummy entries, + // and they will leak at process shutdown: one CoroData per thread. + if (! sCurrent.get()) + { + // It's tempting to provide a distinct name for each thread's "main + // coroutine." But as getName() has always returned the empty string + // to mean "not in a coroutine," empty string should suffice here -- + // and truthfully the additional (thread-safe!) machinery to ensure + // uniqueness just doesn't feel worth the trouble. + // We use a no-op callable and a minimal stack size because, although + // CoroData's constructor in fact initializes its mCoro with a + // coroutine with that stack size, no one ever actually enters it by + // calling mCoro(). + sCurrent.reset(new CoroData(0, // no prev + "", // not a named coroutine + no_op, // no-op callable + 1024)); // stacksize moot + } + mCurrent = &sCurrent; } @@ -63,17 +97,21 @@ LLCoros::Current::Current() LLCoros::CoroData& LLCoros::get_CoroData(const std::string& caller) { CoroData* current = Current(); - if (! current) - { - LL_ERRS("LLCoros") << "Calling " << caller << " from non-coroutine context!" << LL_ENDL; - } + // With the dummy CoroData set in LLCoros::Current::Current(), this + // pointer should never be NULL. + llassert_always(current); return *current; } //static LLCoros::coro::self& LLCoros::get_self() { - return *get_CoroData("get_self()").mSelf; + CoroData& current = get_CoroData("get_self()"); + if (! current.mSelf) + { + LL_ERRS("LLCoros") << "Calling get_self() from non-coroutine context!" << LL_ENDL; + } + return *current.mSelf; } //static @@ -224,13 +262,7 @@ bool LLCoros::kill(const std::string& name) std::string LLCoros::getName() const { - CoroData* current = Current(); - if (! current) - { - // not in a coroutine - return ""; - } - return current->mName; + return Current()->mName; } void LLCoros::setStackSize(S32 stacksize) -- cgit v1.2.3 From 6c7a97286113b1d3335d585d0d7cbc047baae021 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 15 Nov 2016 15:53:24 -0500 Subject: DRTVWR-418: Fold windows64 into windows platform with new autobuild. autobuild 1.1 now supports expanding $variables within a config file -- support that was explicitly added to address this very problem. So now the windows platform in autobuild.xml uses $AUTOBUILD_ADDRSIZE, $AUTOBUILD_WIN_VSPLATFORM and $AUTOBUILD_WIN_CMAKE_GEN, which should handle most of the deltas between the windows platform and windows64. This permits removing the windows64 platform definition from autobuild.xml. The one remaining delta between the windows64 and windows platform definitions was -DLL_64BIT_BUILD=TRUE. But we can handle that instead by checking ADDRESS_SIZE. Change all existing references to WORD_SIZE to ADDRESS_SIZE instead, and set ADDRESS_SIZE to $AUTOBUILD_ADDRSIZE. Change the one existing LL_64BIT_BUILD reference to test (ADDRESS_SIZE EQUAL 64) instead. --- indra/llcommon/llcoros.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/llcommon/llcoros.cpp') diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index 0d9e19f672..bc72faca5d 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -101,7 +101,7 @@ LLCoros::LLCoros(): // Previously we used // boost::context::guarded_stack_allocator::default_stacksize(); // empirically this is 64KB on Windows and Linux. Try quadrupling. -#if WORD_SIZE == 64 +#if ADDRESS_SIZE == 64 mStackSize(512*1024) #else mStackSize(256*1024) -- cgit v1.2.3