From 1a8c8df6862620de64f621363b025b0ffbef72fa Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 13 Mar 2017 14:09:14 -0400 Subject: DRTVWR-418: Ignore logging that requires resurrecting singletons. The logging subsystem depends on two different LLSingletons for some reason. It turns out to be very difficult to completely avoid executing any logging calls after the LLSingletonBase::deleteAll(), but we really don't want to resurrect those LLSingletons so late in the run for a couple stragglers. Introduce LLSingleton::wasDeleted() query method, and use it in logging subsystem to simply bypass last-millisecond logging requests. --- indra/llcommon/llerror.cpp | 97 ++++++++++++++++++++++++++++---------------- indra/llcommon/llsingleton.h | 8 ++++ 2 files changed, 70 insertions(+), 35 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llerror.cpp b/indra/llcommon/llerror.cpp index e6407ecf22..2ddb3edbdd 100644 --- a/indra/llcommon/llerror.cpp +++ b/indra/llcommon/llerror.cpp @@ -1067,7 +1067,15 @@ 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++; @@ -1106,7 +1114,10 @@ namespace LLError std::ostringstream* Log::out() { LogLock lock; - if (lock.ok()) + // 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.ok() && ! (Settings::wasDeleted() || Globals::wasDeleted())) { Globals* g = Globals::getInstance(); @@ -1116,41 +1127,49 @@ namespace LLError return &g->messageStream; } } - + return new std::ostringstream; } - + void Log::flush(std::ostringstream* out, char* message) - { - LogLock lock; - if (!lock.ok()) - { - return; - } - - if(strlen(out->str().c_str()) < 128) - { - strcpy(message, out->str().c_str()); - } - else - { - strncpy(message, out->str().c_str(), 127); - message[127] = '\0' ; - } - - Globals* g = Globals::getInstance(); - if (out == &g->messageStream) - { - g->messageStream.clear(); - g->messageStream.str(""); - g->messageStreamInUse = false; - } - else - { - delete out; - } - return ; - } + { + LogLock lock; + if (!lock.ok()) + { + 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()); + } + else + { + strncpy(message, out->str().c_str(), 127); + message[127] = '\0' ; + } + + Globals* g = Globals::getInstance(); + if (out == &g->messageStream) + { + g->messageStream.clear(); + g->messageStream.str(""); + g->messageStreamInUse = false; + } + else + { + delete out; + } + return ; + } void Log::flush(std::ostringstream* out, const CallSite& site) { @@ -1159,7 +1178,15 @@ 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(); diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 1b915dfd6e..0d4a1f34f8 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -452,6 +452,14 @@ public: return sData.mInitState == INITIALIZED; } + // Has this singleton been deleted? This can be useful during shutdown + // processing to avoid "resurrecting" a singleton we thought we'd already + // cleaned up. + static bool wasDeleted() + { + return sData.mInitState == DELETED; + } + private: struct SingletonData { -- cgit v1.2.3 From e6fc3528fdfd2a251571ef86f321e321865d4592 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 13 Mar 2017 14:22:19 -0400 Subject: DRTVWR-418: #include "llrefcount.h" : LLTombStone uses LLRefCount. Apparently we've been getting away so far without this essential #include only by "leakage" from other #includes in existing consumers. --- indra/llcommon/llhandle.h | 1 + 1 file changed, 1 insertion(+) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llhandle.h b/indra/llcommon/llhandle.h index feb5f41848..570cd330b8 100644 --- a/indra/llcommon/llhandle.h +++ b/indra/llcommon/llhandle.h @@ -28,6 +28,7 @@ #define LLHANDLE_H #include "llpointer.h" +#include "llrefcount.h" #include "llexception.h" #include #include -- cgit v1.2.3 From c1458713dea2ac8cec100628c0ca5238fcca93ba Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 13 Mar 2017 14:31:38 -0400 Subject: DRTVWR-418: Make LLEventPumps an LLHandleProvider for LLEventPump. LLEventPump's destructor was using LLEventPumps::instance() to unregister the LLEventPump instance from LLEventPumps. Evidently, though, there are lingering LLEventPump instances that persist even after the LLSingletonBase::deleteAll() call destroys the LLEventPumps LLSingleton instance. These were resurrecting LLEventPumps -- pointlessly, since a newly-resurrected LLEventPumps instance can have no knowledge of the LLEventPump instance! Unregistering is unnecessary! What we want is a reference we can bind into each LLEventPump instance that allows us to safely test whether the LLEventPumps instance still exists. LLHandle is exactly that. Make LLEventPumps an LLHandleProvider and bind its LLHandle in each LLEventPump's constructor; then the destructor can unregister only when LLEventPumps still exists. --- indra/llcommon/llevents.cpp | 12 +++++++++--- indra/llcommon/llevents.h | 18 +++++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llevents.cpp b/indra/llcommon/llevents.cpp index 97270e4931..a3856e4fc4 100644 --- a/indra/llcommon/llevents.cpp +++ b/indra/llcommon/llevents.cpp @@ -281,7 +281,8 @@ const std::string LLEventPump::ANONYMOUS = std::string(); LLEventPump::LLEventPump(const std::string& name, bool tweak): // Register every new instance with LLEventPumps - mName(LLEventPumps::instance().registerNew(*this, name, tweak)), + mRegistry(LLEventPumps::instance().getHandle()), + mName(mRegistry.get()->registerNew(*this, name, tweak)), mSignal(new LLStandardSignal()), mEnabled(true) {} @@ -292,8 +293,13 @@ LLEventPump::LLEventPump(const std::string& name, bool tweak): LLEventPump::~LLEventPump() { - // Unregister this doomed instance from LLEventPumps - LLEventPumps::instance().unregister(*this); + // Unregister this doomed instance from LLEventPumps -- but only if + // LLEventPumps is still around! + LLEventPumps* registry = mRegistry.get(); + if (registry) + { + registry->unregister(*this); + } } // static data member diff --git a/indra/llcommon/llevents.h b/indra/llcommon/llevents.h index 7cff7dfd45..1d51c660ed 100644 --- a/indra/llcommon/llevents.h +++ b/indra/llcommon/llevents.h @@ -62,6 +62,7 @@ #include "lldependencies.h" #include "llstl.h" #include "llexception.h" +#include "llhandle.h" /*==========================================================================*| // override this to allow binding free functions with more parameters @@ -227,7 +228,15 @@ class LLEventPump; * LLEventPumps is a Singleton manager through which one typically accesses * this subsystem. */ -class LL_COMMON_API LLEventPumps: public LLSingleton +// LLEventPumps isa LLHandleProvider only for (hopefully rare) long-lived +// class objects that must refer to this class late in their lifespan, say in +// the destructor. Specifically, the case that matters is a possible reference +// after LLEventPumps::deleteSingleton(). (Lingering LLEventPump instances are +// capable of this.) In that case, instead of calling LLEventPumps::instance() +// again -- resurrecting the deleted LLSingleton -- store an +// LLHandle and test it before use. +class LL_COMMON_API LLEventPumps: public LLSingleton, + public LLHandleProvider { LLSINGLETON(LLEventPumps); public: @@ -590,6 +599,9 @@ private: return this->listen_impl(name, listener, after, before); } + // must precede mName; see LLEventPump::LLEventPump() + LLHandle mRegistry; + std::string mName; protected: @@ -817,14 +829,14 @@ public: mConnection(new LLBoundListener) { } - + /// Copy constructor. Copy shared_ptrs to original instance data. LLListenerWrapperBase(const LLListenerWrapperBase& that): mName(that.mName), mConnection(that.mConnection) { } - virtual ~LLListenerWrapperBase() {} + virtual ~LLListenerWrapperBase() {} /// Ask LLEventPump::listen() for the listener name virtual void accept_name(const std::string& name) const -- cgit v1.2.3 From 64581fb8d001262d2e34b3e3b653e485555d9c9c Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 29 Mar 2017 16:07:58 -0400 Subject: DRTVWR-418: Instead of "Unknown", try be informative about platform. When a 'family' code isn't recognized, for instance, report the family code. That should at least clue us in to look up and add an entry for the relevant family code. --- indra/llcommon/llprocessor.cpp | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llprocessor.cpp b/indra/llcommon/llprocessor.cpp index 65b4507e2d..446c312ca9 100644 --- a/indra/llcommon/llprocessor.cpp +++ b/indra/llcommon/llprocessor.cpp @@ -26,9 +26,11 @@ #include "linden_common.h" #include "llprocessor.h" - +#include "llstring.h" +#include "stringize.h" #include "llerror.h" +#include //#include #if LL_WINDOWS @@ -188,7 +190,7 @@ namespace case 0xF: return "Intel Pentium 4"; case 0x10: return "Intel Itanium 2 (IA-64)"; } - return "Unknown"; + return STRINGIZE("Intel "); } std::string amd_CPUFamilyName(int composed_family) @@ -201,26 +203,26 @@ namespace case 0xF: return "AMD K8"; case 0x10: return "AMD K8L"; } - return "Unknown"; + return STRINGIZE("AMD "); } std::string compute_CPUFamilyName(const char* cpu_vendor, int family, int ext_family) { const char* intel_string = "GenuineIntel"; const char* amd_string = "AuthenticAMD"; - if(!strncmp(cpu_vendor, intel_string, strlen(intel_string))) + if (LLStringUtil::startsWith(cpu_vendor, intel_string)) { U32 composed_family = family + ext_family; return intel_CPUFamilyName(composed_family); } - else if(!strncmp(cpu_vendor, amd_string, strlen(amd_string))) + else if (LLStringUtil::startsWith(cpu_vendor, amd_string)) { U32 composed_family = (family == 0xF) ? family + ext_family : family; return amd_CPUFamilyName(composed_family); } - return "Unknown"; + return STRINGIZE("Unrecognized CPU vendor <" << cpu_vendor << ">"); } } // end unnamed namespace @@ -258,8 +260,8 @@ public: return hasExtension("Altivec"); } - std::string getCPUFamilyName() const { return getInfo(eFamilyName, "Unknown").asString(); } - std::string getCPUBrandName() const { return getInfo(eBrandName, "Unknown").asString(); } + std::string getCPUFamilyName() const { return getInfo(eFamilyName, "Unset family").asString(); } + std::string getCPUBrandName() const { return getInfo(eBrandName, "Unset brand").asString(); } // This is virtual to support a different linux format. // *NOTE:Mani - I didn't want to screw up server use of this data... @@ -271,7 +273,7 @@ public: out << "//////////////////////////" << std::endl; out << "Processor Name: " << getCPUBrandName() << std::endl; out << "Frequency: " << getCPUFrequency() << " MHz" << std::endl; - out << "Vendor: " << getInfo(eVendor, "Unknown").asString() << std::endl; + out << "Vendor: " << getInfo(eVendor, "Unset vendor").asString() << std::endl; out << "Family: " << getCPUFamilyName() << " (" << getInfo(eFamily, 0) << ")" << std::endl; out << "Extended family: " << getInfo(eExtendedFamily, 0) << std::endl; out << "Model: " << getInfo(eModel, 0) << std::endl; -- cgit v1.2.3 From e9fe0714ad422c2b9250c8ce13d3b2837dff5430 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 30 Mar 2017 15:39:47 -0400 Subject: DRTVWR-418: Xcode 8.3 complains about LLSafeHandle implementation. The previous LLSafeHandle implementation declares a static data member of the template class but provides no (generic) definition, relying on particular specializations to provide the definition. The data member is a function pointer, which is called in one of the methods to produce a pointer to a "null" T instance: that is, a dummy instance to be dereferenced in case the wrapped T* is null. Xcode 8.3's version of clang is bothered by the call, in a generic method, through this (usually) uninitialized pointer. It happens that the only specializations of LLSafeHandle do both provide definitions. I don't know whether that's formally valid C++03 or not; but I agree with the compiler: I don't like it. Instead of declaring a public static function pointer which each specialization is required to define, add a protected static method to the template class. This protected static method simply returns a pointer to a function-static T instance. This is functionally similar to a static LLPointer set on demand (as in the two specializations), including lazy instantiation. Unlike the previous implementation, this approach prohibits a given specialization from customizing the "null" instance function. Although there exist reasonable ways to support that (e.g. a related traits template), I decided not to complicate the LLSafeHandle implementation to make it more generally useful. I don't really approve of LLSafeHandle, and don't want to see it proliferate. It's not clear that unconditionally dereferencing LLSafeHandle is in any way better than conditionally dereferencing LLPointer. It doesn't even skip the runtime conditional test; it simply obscures it. (There exist hints in the code that at one time it might have immediately replaced any wrapped null pointer value with the pointer to the "null" instance, obviating the test at dereference time, but this is not the current functionality. Perhaps it was only ever wishful thinking.) Remove the corresponding functions and static LLPointers from the two classes that use LLSafeHandle. --- indra/llcommon/llsafehandle.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llsafehandle.h b/indra/llcommon/llsafehandle.h index 4226bf04f0..af1c26dd4f 100644 --- a/indra/llcommon/llsafehandle.h +++ b/indra/llcommon/llsafehandle.h @@ -112,10 +112,6 @@ public: return *this; } -public: - typedef Type* (*NullFunc)(); - static const NullFunc sNullFunc; - protected: void ref() { @@ -155,6 +151,12 @@ protected: return ptr == NULL ? sNullFunc() : ptr; } + static Type* sNullFunc() + { + static Type sInstance; + return &sInstance; + } + protected: Type* mPointer; }; -- cgit v1.2.3 From 9dd7c67012e305fa61d20135e6082389e622c88a Mon Sep 17 00:00:00 2001 From: Callum Prentice Date: Wed, 19 Apr 2017 16:50:56 -0700 Subject: Pull in improvements to LLProcess termination via a commit from Nat Linden here: https://bitbucket.org/rider_linden/doduo-viewer/commits/4f39500cb46e879dbb732e6547cc66f3ba39959e?at=default --- indra/llcommon/llprocess.cpp | 12 +++-- indra/llcommon/llprocess.h | 29 +++++++++-- indra/llcommon/tests/llprocess_test.cpp | 89 ++++++++++++++++++++++++++++----- 3 files changed, 110 insertions(+), 20 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 8c321d06b9..5753efdc59 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -517,6 +517,10 @@ LLProcessPtr LLProcess::create(const LLSDOrParams& params) LLProcess::LLProcess(const LLSDOrParams& params): mAutokill(params.autokill), + // Because 'autokill' originally meant both 'autokill' and 'attached', to + // preserve existing semantics, we promise that mAttached defaults to the + // same setting as mAutokill. + mAttached(params.attached.isProvided()? params.attached : params.autokill), mPipes(NSLOTS) { // Hmm, when you construct a ptr_vector with a size, it merely reserves @@ -625,9 +629,9 @@ LLProcess::LLProcess(const LLSDOrParams& params): // std handles and the like, and that's a bit more detachment than we // want. autokill=false just means not to implicitly kill the child when // the parent terminates! -// chkapr(apr_procattr_detach_set(procattr, params.autokill? 0 : 1)); +// chkapr(apr_procattr_detach_set(procattr, mAutokill? 0 : 1)); - if (params.autokill) + if (mAutokill) { #if ! defined(APR_HAS_PROCATTR_AUTOKILL_SET) // Our special preprocessor symbol isn't even defined -- wrong APR @@ -696,7 +700,7 @@ LLProcess::LLProcess(const LLSDOrParams& params): // take steps to terminate the child. This is all suspenders-and-belt: in // theory our destructor should kill an autokill child, but in practice // that doesn't always work (e.g. VWR-21538). - if (params.autokill) + if (mAutokill) { /*==========================================================================*| // NO: There may be an APR bug, not sure -- but at least on Mac, when @@ -799,7 +803,7 @@ LLProcess::~LLProcess() sProcessListener.dropPoll(*this); } - if (mAutokill) + if (mAttached) { kill("destructor"); } diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index bfac4567a5..e3386ad88e 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -167,6 +167,7 @@ public: args("args"), cwd("cwd"), autokill("autokill", true), + attached("attached", true), files("files"), postend("postend"), desc("desc") @@ -183,9 +184,31 @@ public: Multiple args; /// current working directory, if need it changed Optional cwd; - /// implicitly kill process on destruction of LLProcess object - /// (default true) + /// implicitly kill child process on termination of parent, whether + /// voluntary or crash (default true) Optional autokill; + /// implicitly kill process on destruction of LLProcess object + /// (default same as autokill) + /// + /// Originally, 'autokill' conflated two concepts: kill child process on + /// - destruction of its LLProcess object, and + /// - termination of parent process, voluntary or otherwise. + /// + /// It's useful to tease these apart. Some child processes are sent a + /// "clean up and terminate" message before the associated LLProcess + /// object is destroyed. A child process launched with attached=false + /// has an extra time window from the destruction of its LLProcess + /// until parent-process termination in which to perform its own + /// orderly shutdown, yet autokill=true still guarantees that we won't + /// accumulate orphan instances of such processes indefinitely. With + /// attached=true, if a child process cannot clean up between the + /// shutdown message and LLProcess destruction (presumably very soon + /// thereafter), it's forcibly killed anyway -- which can lead to + /// distressing user-visible crash indications. + /// + /// (The usefulness of attached=true with autokill=false is less + /// clear, but we don't prohibit that combination.) + Optional attached; /** * Up to three FileParam items: for child stdin, stdout, stderr. * Passing two FileParam entries means default treatment for stderr, @@ -540,7 +563,7 @@ private: std::string mDesc; std::string mPostend; apr_proc_t mProcess; - bool mAutokill; + bool mAutokill, mAttached; Status mStatus; // explicitly want this ptr_vector to be able to store NULLs typedef boost::ptr_vector< boost::nullable > PipeVector; diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 5ba343b183..b27e125d2e 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -788,6 +788,69 @@ namespace tut template<> template<> void object::test<10>() + { + set_test_name("attached=false"); + // almost just like autokill=false, except set autokill=true with + // attached=false. + NamedTempFile from("from", "not started"); + NamedTempFile to("to", ""); + LLProcess::handle phandle(0); + { + PythonProcessLauncher py(get_test_name(), + "from __future__ import with_statement\n" + "import sys, time\n" + "with open(sys.argv[1], 'w') as f:\n" + " f.write('ok')\n" + "# wait for 'go' from test program\n" + "for i in xrange(60):\n" + " time.sleep(1)\n" + " with open(sys.argv[2]) as f:\n" + " go = f.read()\n" + " if go == 'go':\n" + " break\n" + "else:\n" + " with open(sys.argv[1], 'w') as f:\n" + " f.write('never saw go')\n" + " sys.exit(1)\n" + "# okay, saw 'go', write 'ack'\n" + "with open(sys.argv[1], 'w') as f:\n" + " f.write('ack')\n"); + py.mParams.args.add(from.getName()); + py.mParams.args.add(to.getName()); + py.mParams.autokill = true; + py.mParams.attached = false; + py.launch(); + // Capture handle for later + phandle = py.mPy->getProcessHandle(); + // Wait for the script to wake up and do its first write + int i = 0, timeout = 60; + for ( ; i < timeout; ++i) + { + yield(); + if (readfile(from.getName(), "from autokill script") == "ok") + break; + } + // If we broke this loop because of the counter, something's wrong + ensure("script never started", i < timeout); + // Now destroy the LLProcess, which should NOT kill the child! + } + // If the destructor killed the child anyway, give it time to die + yield(2); + // How do we know it's not terminated? By making it respond to + // a specific stimulus in a specific way. + { + std::ofstream outf(to.getName().c_str()); + outf << "go"; + } // flush and close. + // now wait for the script to terminate... one way or another. + waitfor(phandle, "autokill script"); + // If the LLProcess destructor implicitly called kill(), the + // script could not have written 'ack' as we expect. + ensure_equals(get_test_name() + " script output", readfile(from.getName()), "ack"); + } + + template<> template<> + void object::test<11>() { set_test_name("'bogus' test"); CaptureLog recorder; @@ -801,7 +864,7 @@ namespace tut } template<> template<> - void object::test<11>() + void object::test<12>() { set_test_name("'file' test"); // Replace this test with one or more real 'file' tests when we @@ -815,7 +878,7 @@ namespace tut } template<> template<> - void object::test<12>() + void object::test<13>() { set_test_name("'tpipe' test"); // Replace this test with one or more real 'tpipe' tests when we @@ -832,7 +895,7 @@ namespace tut } template<> template<> - void object::test<13>() + void object::test<14>() { set_test_name("'npipe' test"); // Replace this test with one or more real 'npipe' tests when we @@ -850,7 +913,7 @@ namespace tut } template<> template<> - void object::test<14>() + void object::test<15>() { set_test_name("internal pipe name warning"); CaptureLog recorder; @@ -914,7 +977,7 @@ namespace tut } while (0) template<> template<> - void object::test<15>() + void object::test<16>() { set_test_name("get*Pipe() validation"); PythonProcessLauncher py(get_test_name(), @@ -934,7 +997,7 @@ namespace tut } template<> template<> - void object::test<16>() + void object::test<17>() { set_test_name("talk to stdin/stdout"); PythonProcessLauncher py(get_test_name(), @@ -992,7 +1055,7 @@ namespace tut } template<> template<> - void object::test<17>() + void object::test<18>() { set_test_name("listen for ReadPipe events"); PythonProcessLauncher py(get_test_name(), @@ -1052,7 +1115,7 @@ namespace tut } template<> template<> - void object::test<18>() + void object::test<19>() { set_test_name("ReadPipe \"eof\" event"); PythonProcessLauncher py(get_test_name(), @@ -1078,7 +1141,7 @@ namespace tut } template<> template<> - void object::test<19>() + void object::test<20>() { set_test_name("setLimit()"); PythonProcessLauncher py(get_test_name(), @@ -1107,7 +1170,7 @@ namespace tut } template<> template<> - void object::test<20>() + void object::test<21>() { set_test_name("peek() ReadPipe data"); PythonProcessLauncher py(get_test_name(), @@ -1160,7 +1223,7 @@ namespace tut } template<> template<> - void object::test<21>() + void object::test<22>() { set_test_name("bad postend"); std::string pumpname("postend"); @@ -1185,7 +1248,7 @@ namespace tut } template<> template<> - void object::test<22>() + void object::test<23>() { set_test_name("good postend"); PythonProcessLauncher py(get_test_name(), @@ -1241,7 +1304,7 @@ namespace tut }; template<> template<> - void object::test<23>() + void object::test<24>() { set_test_name("all data visible at postend"); PythonProcessLauncher py(get_test_name(), -- cgit v1.2.3 From d415e019a61cfd20bb1e254fbc9279f96047da85 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 24 Apr 2017 16:36:54 -0400 Subject: DRTVWR-418: Remove final shutdown cleanup as a cause of crashes. The recent LLSingleton work added a hook that would run during the C++ runtime's final destruction of static objects. When the LAST LLSingleton in any module was destroyed, its destructor would call LLSingletonBase::deleteAll(). That mechanism was intended to permit an application consuming LLSingletons to skip making an explicit deleteAll() call, knowing that all instantiated LLSingleton instances would eventually be cleaned up anyway. However -- experience proves that kicking off deleteAll() processing during the C++ runtime's final cleanup is too late. Too much has already been destroyed. That call tends to cause more shutdown crashes than it resolves. This commit deletes that whole mechanism. Going forward, if you want to clean up LLSingleton instances, you must explicitly call LLSingletonBase::deleteAll() during the application lifetime. If you don't, LLSingleton instances will simply be leaked -- which might be okay, considering the application is terminating anyway. --- indra/llcommon/llsingleton.cpp | 46 ++---------------------------------------- indra/llcommon/llsingleton.h | 30 ++++++++------------------- 2 files changed, 10 insertions(+), 66 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp index 9025e53bb2..18e1b96a5f 100644 --- a/indra/llcommon/llsingleton.cpp +++ b/indra/llcommon/llsingleton.cpp @@ -369,62 +369,20 @@ void LLSingletonBase::deleteAll() } } -/*------------------------ Final cleanup management ------------------------*/ -class LLSingletonBase::MasterRefcount -{ -public: - // store a POD int so it will be statically initialized to 0 - int refcount; -}; -static LLSingletonBase::MasterRefcount sMasterRefcount; - -LLSingletonBase::ref_ptr_t LLSingletonBase::get_master_refcount() -{ - // Calling this method constructs a new ref_ptr_t, which implicitly calls - // intrusive_ptr_add_ref(MasterRefcount*). - return &sMasterRefcount; -} - -void intrusive_ptr_add_ref(LLSingletonBase::MasterRefcount* mrc) -{ - // Count outstanding SingletonLifetimeManager instances. - ++mrc->refcount; -} - -void intrusive_ptr_release(LLSingletonBase::MasterRefcount* mrc) -{ - // Notice when each SingletonLifetimeManager instance is destroyed. - if (! --mrc->refcount) - { - // The last instance was destroyed. Time to kill any remaining - // LLSingletons -- but in dependency order. - LLSingletonBase::deleteAll(); - } -} - /*---------------------------- Logging helpers -----------------------------*/ namespace { bool oktolog() { // See comments in log() below. - return sMasterRefcount.refcount && LLError::is_available(); + return LLError::is_available(); } void log(LLError::ELevel level, const char* p1, const char* p2, const char* p3, const char* p4) { - // Check whether we're in the implicit final LLSingletonBase::deleteAll() - // call. We've carefully arranged for deleteAll() to be called when the - // last SingletonLifetimeManager instance is destroyed -- in other words, - // when the last translation unit containing an LLSingleton instance - // cleans up static data. That could happen after std::cerr is destroyed! // The is_available() test below ensures that we'll stop logging once // LLError has been cleaned up. If we had a similar portable test for - // std::cerr, this would be a good place to use it. As we do not, just - // don't log anything during implicit final deleteAll(). Detect that by - // the master refcount having gone to zero. - if (sMasterRefcount.refcount == 0) - return; + // std::cerr, this would be a good place to use it. // Check LLError::is_available() because some of LLError's infrastructure // is itself an LLSingleton. If that LLSingleton has not yet been diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 0d4a1f34f8..859e271e26 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -27,7 +27,6 @@ #include #include -#include #include #include #include @@ -36,8 +35,6 @@ class LLSingletonBase: private boost::noncopyable { public: class MasterList; - class MasterRefcount; - typedef boost::intrusive_ptr ref_ptr_t; private: // All existing LLSingleton instances are tracked in this master list. @@ -119,9 +116,6 @@ protected: const char* p3="", const char* p4=""); static std::string demangle(const char* mangled); - // obtain canonical ref_ptr_t - static ref_ptr_t get_master_refcount(); - // Default methods in case subclass doesn't declare them. virtual void initSingleton() {} virtual void cleanupSingleton() {} @@ -175,10 +169,6 @@ public: static void deleteAll(); }; -// support ref_ptr_t -void intrusive_ptr_add_ref(LLSingletonBase::MasterRefcount*); -void intrusive_ptr_release(LLSingletonBase::MasterRefcount*); - // Most of the time, we want LLSingleton_manage_master() to forward its // methods to real LLSingletonBase methods. template @@ -298,8 +288,7 @@ private: // stores pointer to singleton instance struct SingletonLifetimeManager { - SingletonLifetimeManager(): - mMasterRefcount(LLSingletonBase::get_master_refcount()) + SingletonLifetimeManager() { construct(); } @@ -317,17 +306,14 @@ private: // of static-object destruction, mean that we DO NOT WANT this // destructor to delete this LLSingleton. This destructor will run // without regard to any other LLSingleton whose cleanup might - // depend on its existence. What we really want is to count the - // runtime's attempts to cleanup LLSingleton static data -- and on - // the very last one, call LLSingletonBase::deleteAll(). That - // method will properly honor cross-LLSingleton dependencies. This - // is why we store an intrusive_ptr to a MasterRefcount: our - // ref_ptr_t member counts SingletonLifetimeManager instances. - // Once the runtime destroys the last of these, THEN we can delete - // every remaining LLSingleton. + // depend on its existence. If you want to clean up LLSingletons, + // call LLSingletonBase::deleteAll() sometime before static-object + // destruction begins. That method will properly honor cross- + // LLSingleton dependencies. Otherwise we simply leak LLSingleton + // instances at shutdown. Since the whole process is terminating + // anyway, that's not necessarily a bad thing; it depends on what + // resources your LLSingleton instances are managing. } - - LLSingletonBase::ref_ptr_t mMasterRefcount; }; protected: -- cgit v1.2.3 From 29dd5f0123a89f44688477d04a421b90d1efe635 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 27 Apr 2017 18:49:33 -0400 Subject: DRTVWR-418: Use (protected) LLSingleton to store "null instance" of LLSafeHandle's referenced type. Using LLSingleton gives us a well-defined time at which the "null instance" is deleted: LLSingletonBase::deleteAll(). --- indra/llcommon/llsafehandle.h | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llsafehandle.h b/indra/llcommon/llsafehandle.h index af1c26dd4f..74d31297a0 100644 --- a/indra/llcommon/llsafehandle.h +++ b/indra/llcommon/llsafehandle.h @@ -27,6 +27,7 @@ #define LLSAFEHANDLE_H #include "llerror.h" // *TODO: consider eliminating this +#include "llsingleton.h" // Expands LLPointer to return a pointer to a special instance of class Type instead of NULL. // This is useful in instances where operations on NULL pointers are semantically safe and/or @@ -146,15 +147,24 @@ protected: } } - static Type* nonNull(Type* ptr) + // Define an LLSingleton whose sole purpose is to hold a "null instance" + // of the subject Type: the canonical instance to dereference if this + // LLSafeHandle actually holds a null pointer. We use LLSingleton + // specifically so that the "null instance" can be cleaned up at a well- + // defined time, specifically LLSingletonBase::deleteAll(). + // Of course, as with any LLSingleton, the "null instance" is only + // instantiated on demand -- in this case, if you actually try to + // dereference an LLSafeHandle containing null. + class NullInstanceHolder: public LLSingleton { - return ptr == NULL ? sNullFunc() : ptr; - } + LLSINGLETON_EMPTY_CTOR(NullInstanceHolder); + public: + Type mNullInstance; + }; - static Type* sNullFunc() + static Type* nonNull(Type* ptr) { - static Type sInstance; - return &sInstance; + return ptr? ptr : &NullInstanceHolder::instance().mNullInstance; } protected: -- cgit v1.2.3 From 52899ed62a241d7875277b0f3412e2be78f7b3af Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 2 May 2017 10:51:18 -0400 Subject: DRTVWR-418, MAINT-6996: Rationalize LLMemory wrt 64-bit support. There were two distinct LLMemory methods getCurrentRSS() and getWorkingSetSize(). It was pointless to have both: on Windows they were completely redundant; on other platforms getWorkingSetSize() always returned 0. (Amusingly, though the Windows implementations both made exactly the same GetProcessMemoryInfo() call and used exactly the same logic, the code was different in the two -- as though the second was implemented without awareness of the first, even though they were adjacent in the source file.) One of the actual MAINT-6996 problems was due to the fact that getWorkingSetSize() returned U32, where getCurrentRSS() returns U64. In other words, getWorkingSetSize() was both useless *and* wrong. Remove it, and change its one call to getCurrentRSS() instead. The other culprit was that in several places, the 64-bit WorkingSetSize returned by the Windows GetProcessMemoryInfo() call (and by getCurrentRSS()) was explicitly cast to a 32-bit data type. That works only when explicitly or implicitly (using LLUnits type conversion) scaling the value to kilobytes or megabytes. When the size in bytes is desired, use 64-bit types instead. In addition to the symptoms, LLMemory was overdue for a bit of cleanup. There was a 16K block of memory called reserveMem, the comment on which read: "reserve 16K for out of memory error handling." Yet *nothing* was ever done with that block! If it were going to be useful, one would think someone would at some point explicitly free the block. In fact there was a method freeReserve(), apparently for just that purpose -- which was never called. As things stood, reserveMem served only to *prevent* the viewer from ever using that chunk of memory. Remove reserveMem and the unused freeReserve(). The only function of initClass() and cleanupClass() was to allocate and free reserveMem. Remove initClass(), cleanupClass() and the LLCommon calls to them. In a similar vein, there was an LLMemoryInfo::getPhysicalMemoryClamped() method that returned U32Bytes. Its job was simply to return a size in bytes that could fit into a U32 data type, returning U32_MAX if the 64-bit value exceeded 4GB. Eliminate that; change all its calls to getPhysicalMemoryKB() (which getPhysicalMemoryClamped() used internally anyway). We no longer care about any platform that cannot handle 64-bit data types. --- indra/llcommon/llcommon.cpp | 2 - indra/llcommon/llmemory.cpp | 89 ++++++++------------------------------------- indra/llcommon/llmemory.h | 5 --- indra/llcommon/llsys.cpp | 16 -------- indra/llcommon/llsys.h | 5 --- 5 files changed, 15 insertions(+), 102 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llcommon.cpp b/indra/llcommon/llcommon.cpp index 439ff4e628..2d665c611b 100644 --- a/indra/llcommon/llcommon.cpp +++ b/indra/llcommon/llcommon.cpp @@ -41,7 +41,6 @@ static LLTrace::ThreadRecorder* sMasterThreadRecorder = NULL; //static void LLCommon::initClass() { - LLMemory::initClass(); if (!sAprInitialized) { ll_init_apr(); @@ -70,5 +69,4 @@ void LLCommon::cleanupClass() ll_cleanup_apr(); sAprInitialized = FALSE; } - SUBSYSTEM_CLEANUP(LLMemory); } diff --git a/indra/llcommon/llmemory.cpp b/indra/llcommon/llmemory.cpp index 1e04044269..6a7e1362f0 100644 --- a/indra/llcommon/llmemory.cpp +++ b/indra/llcommon/llmemory.cpp @@ -44,10 +44,10 @@ #include "llsys.h" #include "llframetimer.h" #include "lltrace.h" +#include "llerror.h" //---------------------------------------------------------------------------- //static -char* LLMemory::reserveMem = 0; U32Kilobytes LLMemory::sAvailPhysicalMemInKB(U32_MAX); U32Kilobytes LLMemory::sMaxPhysicalMemInKB(0); static LLTrace::SampleStatHandle sAllocatedMem("allocated_mem", "active memory in use by application"); @@ -78,29 +78,6 @@ void ll_assert_aligned_func(uintptr_t ptr,U32 alignment) #endif } -//static -void LLMemory::initClass() -{ - if (!reserveMem) - { - reserveMem = new char[16*1024]; // reserve 16K for out of memory error handling - } -} - -//static -void LLMemory::cleanupClass() -{ - delete [] reserveMem; - reserveMem = NULL; -} - -//static -void LLMemory::freeReserve() -{ - delete [] reserveMem; - reserveMem = NULL; -} - //static void LLMemory::initMaxHeapSizeGB(F32Gigabytes max_heap_size, BOOL prevent_heap_failure) { @@ -111,19 +88,18 @@ void LLMemory::initMaxHeapSizeGB(F32Gigabytes max_heap_size, BOOL prevent_heap_f //static void LLMemory::updateMemoryInfo() { -#if LL_WINDOWS - HANDLE self = GetCurrentProcess(); +#if LL_WINDOWS PROCESS_MEMORY_COUNTERS counters; - - if (!GetProcessMemoryInfo(self, &counters, sizeof(counters))) + + if (!GetProcessMemoryInfo(GetCurrentProcess(), &counters, sizeof(counters))) { LL_WARNS() << "GetProcessMemoryInfo failed" << LL_ENDL; return ; } - sAllocatedMemInKB = (U32Bytes)(counters.WorkingSetSize) ; + sAllocatedMemInKB = U64Bytes(counters.WorkingSetSize) ; sample(sAllocatedMem, sAllocatedMemInKB); - sAllocatedPageSizeInKB = (U32Bytes)(counters.PagefileUsage) ; + sAllocatedPageSizeInKB = U64Bytes(counters.PagefileUsage) ; sample(sVirtualMem, sAllocatedPageSizeInKB); U32Kilobytes avail_phys, avail_virtual; @@ -140,9 +116,9 @@ void LLMemory::updateMemoryInfo() } #else //not valid for other systems for now. - sAllocatedMemInKB = (U32Bytes)LLMemory::getCurrentRSS(); - sMaxPhysicalMemInKB = (U32Bytes)U32_MAX ; - sAvailPhysicalMemInKB = (U32Bytes)U32_MAX ; + sAllocatedMemInKB = U64Bytes(LLMemory::getCurrentRSS()); + sMaxPhysicalMemInKB = U64Bytes(U32_MAX); + sAvailPhysicalMemInKB = U64Bytes(U32_MAX); #endif return ; @@ -169,7 +145,7 @@ void* LLMemory::tryToAlloc(void* address, U32 size) return address ; #else return (void*)0x01 ; //skip checking -#endif +#endif } //static @@ -183,7 +159,7 @@ void LLMemory::logMemoryInfo(BOOL update) LL_INFOS() << "Current allocated physical memory(KB): " << sAllocatedMemInKB << LL_ENDL ; LL_INFOS() << "Current allocated page size (KB): " << sAllocatedPageSizeInKB << LL_ENDL ; - LL_INFOS() << "Current availabe physical memory(KB): " << sAvailPhysicalMemInKB << LL_ENDL ; + LL_INFOS() << "Current available physical memory(KB): " << sAvailPhysicalMemInKB << LL_ENDL ; LL_INFOS() << "Current max usable memory(KB): " << sMaxPhysicalMemInKB << LL_ENDL ; LL_INFOS() << "--- private pool information -- " << LL_ENDL ; @@ -263,12 +239,12 @@ U32Kilobytes LLMemory::getAllocatedMemKB() #if defined(LL_WINDOWS) +//static U64 LLMemory::getCurrentRSS() { - HANDLE self = GetCurrentProcess(); PROCESS_MEMORY_COUNTERS counters; - - if (!GetProcessMemoryInfo(self, &counters, sizeof(counters))) + + if (!GetProcessMemoryInfo(GetCurrentProcess(), &counters, sizeof(counters))) { LL_WARNS() << "GetProcessMemoryInfo failed" << LL_ENDL; return 0; @@ -277,20 +253,6 @@ U64 LLMemory::getCurrentRSS() return counters.WorkingSetSize; } -//static -U32 LLMemory::getWorkingSetSize() -{ - PROCESS_MEMORY_COUNTERS pmc ; - U32 ret = 0 ; - - if (GetProcessMemoryInfo( GetCurrentProcess(), &pmc, sizeof(pmc)) ) - { - ret = pmc.WorkingSetSize ; - } - - return ret ; -} - #elif defined(LL_DARWIN) /* @@ -337,11 +299,6 @@ U64 LLMemory::getCurrentRSS() return residentSize; } -U32 LLMemory::getWorkingSetSize() -{ - return 0 ; -} - #elif defined(LL_LINUX) U64 LLMemory::getCurrentRSS() @@ -353,7 +310,7 @@ U64 LLMemory::getCurrentRSS() if (fp == NULL) { LL_WARNS() << "couldn't open " << statPath << LL_ENDL; - goto bail; + return 0; } // Eee-yew! See Documentation/filesystems/proc.txt in your @@ -372,15 +329,9 @@ U64 LLMemory::getCurrentRSS() fclose(fp); -bail: return rss; } -U32 LLMemory::getWorkingSetSize() -{ - return 0 ; -} - #elif LL_SOLARIS #include #include @@ -410,11 +361,6 @@ U64 LLMemory::getCurrentRSS() return((U64)proc_psinfo.pr_rssize * 1024); } -U32 LLMemory::getWorkingSetSize() -{ - return 0 ; -} - #else U64 LLMemory::getCurrentRSS() @@ -422,11 +368,6 @@ U64 LLMemory::getCurrentRSS() return 0; } -U32 LLMemory::getWorkingSetSize() -{ - return 0; -} - #endif //-------------------------------------------------------------------------------------------------- diff --git a/indra/llcommon/llmemory.h b/indra/llcommon/llmemory.h index 5a3c9bd762..c37967e10e 100644 --- a/indra/llcommon/llmemory.h +++ b/indra/llcommon/llmemory.h @@ -334,13 +334,9 @@ inline void ll_memcpy_nonaliased_aligned_16(char* __restrict dst, const char* __ class LL_COMMON_API LLMemory { public: - static void initClass(); - static void cleanupClass(); - static void freeReserve(); // Return the resident set size of the current process, in bytes. // Return value is zero if not known. static U64 getCurrentRSS(); - static U32 getWorkingSetSize(); static void* tryToAlloc(void* address, U32 size); static void initMaxHeapSizeGB(F32Gigabytes max_heap_size, BOOL prevent_heap_failure); static void updateMemoryInfo() ; @@ -351,7 +347,6 @@ public: static U32Kilobytes getMaxMemKB() ; static U32Kilobytes getAllocatedMemKB() ; private: - static char* reserveMem; static U32Kilobytes sAvailPhysicalMemInKB ; static U32Kilobytes sMaxPhysicalMemInKB ; static U32Kilobytes sAllocatedMemInKB; diff --git a/indra/llcommon/llsys.cpp b/indra/llcommon/llsys.cpp index 1a66612e87..265c637b69 100644 --- a/indra/llcommon/llsys.cpp +++ b/indra/llcommon/llsys.cpp @@ -914,22 +914,6 @@ U32Kilobytes LLMemoryInfo::getPhysicalMemoryKB() const #endif } -U32Bytes LLMemoryInfo::getPhysicalMemoryClamped() const -{ - // Return the total physical memory in bytes, but clamp it - // to no more than U32_MAX - - U32Kilobytes phys_kb = getPhysicalMemoryKB(); - if (phys_kb >= U32Gigabytes(4)) - { - return U32Bytes(U32_MAX); - } - else - { - return phys_kb; - } -} - //static void LLMemoryInfo::getAvailableMemoryKB(U32Kilobytes& avail_physical_mem_kb, U32Kilobytes& avail_virtual_mem_kb) { diff --git a/indra/llcommon/llsys.h b/indra/llcommon/llsys.h index 962367f69f..95ef4cf065 100644 --- a/indra/llcommon/llsys.h +++ b/indra/llcommon/llsys.h @@ -113,11 +113,6 @@ public: void stream(std::ostream& s) const; ///< output text info to s U32Kilobytes getPhysicalMemoryKB() const; - - /*! Memory size in bytes, if total memory is >= 4GB then U32_MAX will - ** be returned. - */ - U32Bytes getPhysicalMemoryClamped() const; ///< Memory size in clamped bytes //get the available memory infomation in KiloBytes. static void getAvailableMemoryKB(U32Kilobytes& avail_physical_mem_kb, U32Kilobytes& avail_virtual_mem_kb); -- cgit v1.2.3 From 2bb19aec989d5964b22f64cc01aa7cbe962e1e6b Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 2 May 2017 11:05:13 -0400 Subject: DRTVWR-418, MAINT-6996: Update Mac LLMemory::getCurrentRSS(). Evidently the Mac implementation of LLMemory::getCurrentRSS() goes back to OS X 10.3, because there was a helpful comment of the form: ------ The API used here is not capable of dealing with 64-bit memory sizes, but is available before 10.4. Once we start requiring 10.4, we can use the updated API, which looks like this: [new current implementation] Of course, this doesn't gain us anything unless we start building the viewer as a 64-bit executable, since that's the only way for our memory allocation to exceed 2^32. ------ Hey, guess what, we're building 64-bit viewers now! Thank you, whoever thoughtfully noted that, both for calling out the issue and sparing us the research. (The comment goes back to Subversion days, so hg blame shows only the merge-to-release changeset.) --- indra/llcommon/llmemory.cpp | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llmemory.cpp b/indra/llcommon/llmemory.cpp index 6a7e1362f0..9f9c3af892 100644 --- a/indra/llcommon/llmemory.cpp +++ b/indra/llcommon/llmemory.cpp @@ -255,19 +255,6 @@ U64 LLMemory::getCurrentRSS() #elif defined(LL_DARWIN) -/* - The API used here is not capable of dealing with 64-bit memory sizes, but is available before 10.4. - - Once we start requiring 10.4, we can use the updated API, which looks like this: - - task_basic_info_64_data_t basicInfo; - mach_msg_type_number_t basicInfoCount = TASK_BASIC_INFO_64_COUNT; - if (task_info(mach_task_self(), TASK_BASIC_INFO_64, (task_info_t)&basicInfo, &basicInfoCount) == KERN_SUCCESS) - - Of course, this doesn't gain us anything unless we start building the viewer as a 64-bit executable, since that's the only way - for our memory allocation to exceed 2^32. -*/ - // if (sysctl(ctl, 2, &page_size, &size, NULL, 0) == -1) // { // LL_WARNS() << "Couldn't get page size" << LL_ENDL; @@ -280,9 +267,9 @@ U64 LLMemory::getCurrentRSS() U64 LLMemory::getCurrentRSS() { U64 residentSize = 0; - task_basic_info_data_t basicInfo; - mach_msg_type_number_t basicInfoCount = TASK_BASIC_INFO_COUNT; - if (task_info(mach_task_self(), TASK_BASIC_INFO, (task_info_t)&basicInfo, &basicInfoCount) == KERN_SUCCESS) + task_basic_info_64_data_t basicInfo; + mach_msg_type_number_t basicInfoCount = TASK_BASIC_INFO_64_COUNT; + if (task_info(mach_task_self(), TASK_BASIC_INFO_64, (task_info_t)&basicInfo, &basicInfoCount) == KERN_SUCCESS) { residentSize = basicInfo.resident_size; -- cgit v1.2.3 From 9002e3cbc7a4f7ef2847ab6652de0f11eaff6ef2 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 3 May 2017 13:20:56 -0400 Subject: DRTVWR-418: Add big deprecation notice to llsafehandle.h. --- indra/llcommon/llsafehandle.h | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llsafehandle.h b/indra/llcommon/llsafehandle.h index 74d31297a0..968a5cfeb1 100644 --- a/indra/llcommon/llsafehandle.h +++ b/indra/llcommon/llsafehandle.h @@ -29,6 +29,29 @@ #include "llerror.h" // *TODO: consider eliminating this #include "llsingleton.h" +/*==========================================================================*| + ____ ___ _ _ ___ _____ _ _ ____ _____ _ +| _ \ / _ \ | \ | |/ _ \_ _| | | | / ___|| ____| | +| | | | | | | | \| | | | || | | | | \___ \| _| | | +| |_| | |_| | | |\ | |_| || | | |_| |___) | |___|_| +|____/ \___/ |_| \_|\___/ |_| \___/|____/|_____(_) + +This handle class is deprecated. Unfortunately it is already in widespread use +to reference the LLObjectSelection and LLParcelSelection classes, but do not +apply LLSafeHandle to other classes, or declare new instances. + +Instead, use LLPointer or other smart pointer types with appropriate checks +for NULL. If you're certain the reference cannot (or must not) be NULL, +consider storing a C++ reference instead -- or use (e.g.) LLCheckedHandle. + +When an LLSafeHandle containing NULL is dereferenced, it resolves to a +canonical "null" T instance. This raises issues about the lifespan of the +"null" instance. In addition to encouraging sloppy coding practices, it +potentially masks bugs when code that performs some mutating operation +inadvertently applies it to the "null" instance. That result might or might +not ever affect subsequent computations. +|*==========================================================================*/ + // Expands LLPointer to return a pointer to a special instance of class Type instead of NULL. // This is useful in instances where operations on NULL pointers are semantically safe and/or // when error checking occurs at a different granularity or in a different part of the code -- cgit v1.2.3 From d6b870c0cae04a9a2576c8849ebe03822eb78e6c Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 3 May 2017 22:53:34 -0400 Subject: DRTVWR-418: Add dtor to LLSafeHandle::NullInstanceHolder to suppress fatal warnings in Visual Studio. --- indra/llcommon/llsafehandle.h | 1 + 1 file changed, 1 insertion(+) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llsafehandle.h b/indra/llcommon/llsafehandle.h index 968a5cfeb1..9550e6253e 100644 --- a/indra/llcommon/llsafehandle.h +++ b/indra/llcommon/llsafehandle.h @@ -181,6 +181,7 @@ protected: class NullInstanceHolder: public LLSingleton { LLSINGLETON_EMPTY_CTOR(NullInstanceHolder); + ~NullInstanceHolder() {} public: Type mNullInstance; }; -- cgit v1.2.3