Skip to content
Snippets Groups Projects
  1. Mar 25, 2020
    • Nat Goodspeed's avatar
      SL-11216: Add llsd::drill() function to drill into an LLSD blob. · a2379d68
      Nat Goodspeed authored
      We include both const and non-const overloads. The latter returns LLSD&, so
      you can assign to the located element.
      
      In fact we already implemented the non-const logic in a less public form as
      storeToLLSDPath() in lleventcoro.cpp. Reimplement the latter to use the new
      llsd::drill() function.
      a2379d68
    • Nat Goodspeed's avatar
      SL-11216: Convert LLVersionInfo to an LLSingleton. · 5a260e0c
      Nat Goodspeed authored
      This changeset is meant to exemplify how to convert a "namespace" class whose
      methods are static -- and whose data are module-static -- to an LLSingleton.
      LLVersionInfo has no initClass() or cleanupClass() methods, but the general
      idea is the same.
      
      * Derive the class from LLSingleton<T>:
        class LLSomeSingleton: public LLSingleton<LLSomeSingleton> { ... };
      * Add LLSINGLETON(LLSomeSingleton); in the private section of the class. This
        usage implies a separate LLSomeSingleton::LLSomeSingleton() definition, as
        described in indra/llcommon/llsingleton.h.
      * Move module-scope data in the .cpp file to non-static class members. Change
        any sVariableName to mVariableName to avoid being outright misleading.
      * Make static class methods non-static. Remove '//static' comments from method
        definitions as needed.
      * For LLVersionInfo specifically, the 'const std::string&' return type was
        replaced with 'std::string'. Returning a reference to a static or a member,
        const or otherwise, is an anti-pattern: the interface constrains the
        implementation, prohibiting possibly later returning a temporary (an
        expression).
      * For LLVersionInfo specifically, 'const S32' return type was replaced with
        simple 'S32'. 'const' is just noise in that usage.
      * Simple member initialization (e.g. the original initializer expressions for
        static variables) can be done with member{ value } initializers (no examples
        here though).
      * Delete initClass() method.
      * LLSingleton's forté is of course lazy initialization. It might work to
        simply delete any calls to initClass(). But if there are side effects that
        must happen at that moment, replace LLSomeSingleton::initClass() with
        (void)LLSomeSingleton::instance();
      * Most initClass() initialization can be done in the constructor, as would
        normally be the case.
      * Initialization that might cause a circular LLSingleton reference should be
        moved to initSingleton(). Override 'void initSingleton();' should be private.
      * For LLVersionInfo specifically, certain initialization that used to be
        lazily performed was made unconditional, due to its low cost.
      * For LLVersionInfo specifically, certain initialization involved calling
        methods that have become non-static. This was moved to initSingleton()
        because, in a constructor body, 'this' does not yet point to the enclosing
        class.
      * Delete cleanupClass() method.
      * There is already a generic LLSingletonBase::deleteAll() call in
        LLAppViewer::cleanup(). It might work to let this new LLSingleton be cleaned
        up with all the rest. But if there are side effects that must happen at that
        moment, replace LLSomeSingleton::cleanupClass() with
        LLSomeSingleton::deleteSingleton(). That said, much of the benefit of
        converting to LLSingleton is deleteAll()'s guarantee that cross-LLSingleton
        dependencies will be properly honored: we're trying to migrate the code base
        away from the present fragile manual cleanup sequence.
      * Most cleanupClass() cleanup can be done in the destructor, as would normally
        be the case.
      * Cleanup that might throw an exception should be moved to cleanupSingleton().
        Override 'void cleanupSingleton();' should be private.
      * Within LLSomeSingleton methods, remove any existing
        LLSomeSingleton::methodName() qualification: simple methodName() is better.
      * In the rest of the code base, convert most LLSomeSingleton::methodName()
        references to LLSomeSingleton::instance().methodName(). (Prefer instance() to
        getInstance() because a reference does not admit the possibility of NULL.)
      * Of course, LLSomeSingleton::ENUM_VALUE can remain unchanged.
      
      In general, for many successive references to an LLSingleton instance, it
      can be useful to capture the instance() as in:
      
      auto& versionInfo{LLVersionInfo::instance()};
      // ... versionInfo.getVersion() ...
      
      We did not do that here only to simplify the code review.
      
      The STRINGIZE(expression) macro encapsulates:
      std::ostringstream out;
      out << expression;
      return out.str();
      We used that in a couple places.
      
      For LLVersionInfo specifically, lllogininstance_test.cpp used to dummy out a
      couple specific static methods. It's harder to dummy out
      LLSingleton::instance() references, so we add the real class to that test.
      5a260e0c
    • Nat Goodspeed's avatar
      SL-11216: Remove LLSingletonBase::cleanupAll(). · 47ec6ab3
      Nat Goodspeed authored
      Remove call from LLAppViewer::cleanup().
      
      Instead, make each LLSingleton<T>::deleteSingleton() call cleanupSingleton()
      just before destroying the instance. Since deleteSingleton() is not a
      destructor, it's fine to call cleanupSingleton() from there; and since
      deleteAll() calls deleteSingleton() on every remaining instance, the former
      cleanupAll() functionality has been subsumed into deleteAll().
      
      Since cleanupSingleton() is now called at exactly one point in the instance's
      lifetime, we no longer need a bool indicating whether it has been called.
      
      The previous protocol of calling cleanupAll() before deleteAll() implemented a
      two-phase cleanup strategy for the application. That is no longer needed.
      Moreover, the cleanupAll() / deleteAll() sequence created a time window during
      which individual LLSingleton<T> instances weren't usable (to the extent that
      their cleanupSingleton() methods released essential resources) but still
      existed -- so a getInstance() call would return the crippled instance rather
      than recreating it.
      
      Remove cleanupAll() calls from tests; adjust to new order of expected side
      effects: instead of A::cleanupSingleton(), B::cleanupSingleton(), ~A(), ~B(),
      now we get A::cleanupSingleton(), ~A(), B::cleanupSingleton(), ~B().
      47ec6ab3
    • Nat Goodspeed's avatar
    • Nat Goodspeed's avatar
      DRTVWR-494: Get initialized LLMutexes for very early log calls. · 4c9e90de
      Nat Goodspeed authored
      Use function-static LLMutex instances instead of module-static instances,
      since some log calls are evidently issued before we get around to initializing
      llerror.cpp module-static variables.
      4c9e90de
    • Nat Goodspeed's avatar
      DRTVWR-494: Move most LLSingleton cleanup back to destructor · 31863d83
      Nat Goodspeed authored
      instead of deleteSingleton().
      
      Specifically, clear static SingletonData and remove the instance from the
      MasterList in the destructor.
      
      Empirically, some consumers are manually deleting LLSingleton instances,
      instead of calling deleteSingleton(). If deleteSingleton() handles cleanup
      rather than the destructor, we're left with dangling pointers in the Master
      List.
      
      We don't also call cleanupSingleton() from the destructor because only
      deleteSingleton() promises to call cleanupSingleton(). Hopefully whoever is
      directly deleting an LLSingleton subclass instance isn't relying on
      cleanupSingleton().
      31863d83
    • Nat Goodspeed's avatar
      DRTVWR-494: Fix Windows macro collisions in windows_volume_catcher · b1477e98
      Nat Goodspeed authored
      by tweaking #include order.
      b1477e98
    • Nat Goodspeed's avatar
    • Nat Goodspeed's avatar
      DRTVWR-494: LLParamSingleton::initParamSingleton() on main thread. · 7e9c5dd0
      Nat Goodspeed authored
      When calling LLParamSingleton::initParamSingleton() on a secondary thread, use
      LLMainThreadTask::dispatch() to construct the instance on the main thread --
      as with LLSingleton::getInstance().
      7e9c5dd0
    • Nat Goodspeed's avatar
      DRTVWR-494: LLParamSingleton<T>::initParamSingleton() now returns T*. · 68505edf
      Nat Goodspeed authored
      So does LLLockedSingleton<T>::construct().
      68505edf
    • Nat Goodspeed's avatar
      DRTVWR-494: Dispatch all LLSingleton construction to the main thread. · a6f5e55d
      Nat Goodspeed authored
      Given the viewer's mutually-dependent LLSingletons, given that different
      threads might simultaneously request different LLSingletons from such a chain
      of circular dependencies, the key to avoiding deadlock is to serialize all
      LLSingleton construction on one thread: the main thread. Add comments to
      LLSingleton::getInstance() explaining the problem and the solution.
      
      Recast LLSingleton's static SingletonData to use LockStatic. Instead of using
      Locker, and simply trusting that every reference to sData is within the
      dynamic scope of a Locker instance, LockStatic enforces that: you can only
      access SingletonData members via LockStatic.
      
      Reorganize the switch in getInstance() to group the CONSTRUCTING error, the
      INITIALIZING/INITIALIZED success case, and the DELETED/UNINITIALIZED
      construction case.
      
      When [re]constructing an instance, on the main thread, retain the lock and
      call constructSingleton() (and capture_dependency()) directly.
      
      On a secondary thread, unlock LockStatic and use LLMainThreadTask::dispatch()
      to call getInstance() on the main thread. Since we might end up enqueuing
      multiple such tasks, it's important to let getInstance() notice when the
      instance has already been constructed and simply return the existing pointer.
      
      Add loginfos() method, sibling to logerrs(), logwarns() and logdebugs().
      Produce loginfos() messages when dispatching to the main thread, when actually
      running on the main thread and when resuming the suspended requesting thread.
      
      Make deleteSingleton() manage all associated state, instead of delegating some
      of that work to ~LLSingleton(). Now, within LockStatic, extract the instance
      pointer and set state to DELETED; that lets subsequent code, which retains the
      only remaining pointer to the instance, remove the master-list entry, call the
      subclass cleanupSingleton() and destructor without needing to hold the lock.
      
      In fact, entirely remove ~LLSingleton().
      
      Import LLSingletonBase::cleanup_() method to wrap the call to subclass
      cleanupSingleton() in try/catch.
      
      Remove cleanupAll() calls from llsingleton_test.cpp, and reorder the success
      cases to reflect the fact that T::cleanupSingleton() is called immediately
      before ~T() for each distinct LLSingleton subclass T.
      
      When getInstance() on a secondary thread dispatches to the main thread, it
      necessarily unlocks its LockStatic lock. But an LLSingleton dependency chain
      strongly depends on the function stack on which getInstance() is invoked --
      the task dispatched to the main thread doesn't know the dependencies tracked
      on the requesting thread stack. So, once the main thread delivers the instance
      pointer, the requesting thread captures its own dependencies for that
      instance.
      
      Back in the requesting thread, obtaining the current EInitState to pass to
      capture_dependencies() would have required relocking LockStatic. Instead, I've
      convinced myself that (a) capture_dependencies() only wanted to know
      EInitState to produce an error for CONSTRUCTING, and (b) in CONSTRUCTING
      state, we never get as far as capture_dependencies() because getInstance()
      produces an error first.
      
      Eliminate the EInitState parameter from all capture_dependencies() methods.
      Remove the LLSingletonBase::capture_dependency() stanza that tested
      EInitState. Make the capture_dependencies() variants that accepted LockStatic
      instead accept LLSingletonBase*. That lets getInstance(), in the
      LLMainThreadTask case, pass the newly-returned instance pointer.
      
      For symmetry, make pop_initializing() accept LLSingletonBase* as well, instead
      of accepting LockStatic and extracting mInstance.
      a6f5e55d
    • Nat Goodspeed's avatar
      DRTVWR-494: Fix VS LLError::Log::demangle() vulnerability. · 1fc7c994
      Nat Goodspeed authored
      The Windows implementation of demangle() assumed that a "mangled" class name
      produced by typeid(class).name() always starts with the prefix "class ",
      checked for that and removed it. If the mangled name didn't start with that
      prefix, it would emit a debug message and return the full name.
      
      When the class in question is actually a struct, the prefix is "struct "
      instead. But when demangle() was being called before logging had been fully
      initialized, the debug message remarking that it didn't start with "class "
      crashed.
      
      Look for either "class " or "struct " prefix. Remove whichever is found and
      return the rest of the name. If neither is found, only log if logging is
      available.
      1fc7c994
    • Nat Goodspeed's avatar
      DRTVWR-494: Remove LLMainThreadTask::dispatch(LockStatic&, ...) · 6586918d
      Nat Goodspeed authored
      Monty's code review reveals that conflating dispatch() with [un]lock
      functionality is inconsistent and unnecessary.
      6586918d
    • Nat Goodspeed's avatar
      dd98717c
    • Nat Goodspeed's avatar
      DRTVWR-494: Add LLMainThreadTask to perform work on the main thread. · 960593fd
      Nat Goodspeed authored
      If already running on the main thread, LLMaintThreadTask simply runs the work
      inline. Otherwise it queues it for the main thread using LLEventTimer, using
      std::future to retrieve the result.
      960593fd
    • Nat Goodspeed's avatar
    • Nat Goodspeed's avatar
      DRTVWR-494: Use std::thread::id for LLThread::currentID(). · 5e7df752
      Nat Goodspeed authored
      LLThread::currentID() used to return a U32, a distinct unsigned value
      incremented by explicitly constructing LLThread or by calling LLThread::
      registerThreadID() early in a thread launched by other means. The latter
      imposed an unobvious requirement on new code based on std::thread. Using
      std::thread::id instead delegates to the compiler/library the problem of
      distinguishing threads launched by any means.
      
      Change lots of explicit U32 declarations. Introduce LLThread::id_t typedef to
      avoid having to run around fixing uses again if we later revisit this decision.
      
      LLMutex, which stores an LLThread::id_t, wants a distinguished value meaning
      NO_THREAD, and had an enum with that name. But as std::thread::id promises
      that the default-constructed value is distinct from every valid value,
      NO_THREAD becomes unnecessary and goes away.
      
      Because LLMutex now stores LLThread::id_t instead of U32, make llmutex.h
      #include "llthread.h" instead of the other way around. This makes LLMutex an
      incomplete type within llthread.h, so move LLThread::lockData() and
      unlockData() to the .cpp file. Similarly, remove llrefcount.h's #include
      "llmutex.h" to break circularity; instead forward-declare LLMutex.
      
      It turns out that a number of source files assumed that #include "llthread.h"
      would get the definition for LLMutex. Sprinkle #include "llmutex.h" as needed.
      
      In the SAFE_SSL code in llcorehttp/httpcommon.cpp, there's an ssl_thread_id()
      callback that returns an unsigned long to the SSL library. When LLThread::
      currentID() was U32, we could simply return that. But std::thread::id is very
      deliberately opaque, and can't be reinterpret_cast to unsigned long.
      Fortunately it can be hashed because std::hash is specialized with that type.
      5e7df752
    • Nat Goodspeed's avatar
      DRTVWR-494: Put streaming operator<<() for kdu_dims in kdu_core. · d6baa7a8
      Nat Goodspeed authored
      It seems the lookup now requires that the operator<<() function be defined in
      the same namespace as the argument.
      d6baa7a8
    • Nat Goodspeed's avatar
      DRTVWR-494: Move LL_ERRS out of llinstancetracker.h header file. · 2506fd78
      Nat Goodspeed authored
      Add a namespaced free function in .cpp file to report LL_ERRS as needed.
      
      Per code review, use a more indicative namespace name.
      2506fd78
    • Nat Goodspeed's avatar
    • Nat Goodspeed's avatar
    • Nat Goodspeed's avatar
      DRTVWR-494: Extract LockStatic as a standalone template class. · 1f7335fd
      Nat Goodspeed authored
      The pattern of requiring a lock to permit *any* access to a static instance of
      something seems generally useful. Break out lockstatic.h; recast
      LLInstanceTracker to use it.
      
      Moving LockStatic to an external template class instead of a nested class in
      LLInstanceTrackerBase leaves LLInstanceTrackerBase pretty empty. Get rid of it.
      
      And *that* means we can move the definition of the StaticData used by each
      LLInstanceTracker specialization into the class itself, rather than having to
      define it beforehand in namespace LLInstanceTrackerStuff.
      1f7335fd
    • Nat Goodspeed's avatar
      DRTVWR-494: Streamline LLSingleton state machine. · 794072c1
      Nat Goodspeed authored
      The CONSTRUCTED state was only briefly set between constructSingleton() and
      finishInitializing(). But as no consumer code is executed between setting
      CONSTRUCTED and setting INITIALIZING, it was impossible to reach the switch
      statement in either getInstance() method in state CONSTRUCTED. So there was no
      point in state CONSTRUCTED. Remove it.
      
      With CONSTRUCTED gone, we only ever call finishInitializing() right after
      constructSingleton(). Merge finishInitializing() into constructSingleton().
      794072c1
    • Nat Goodspeed's avatar
    • Nat Goodspeed's avatar
      DRTVWR-494: Streamline LLEventTimer::updateClass(). · 0c42f50d
      Nat Goodspeed authored
      No need to capture a separate list of completed LLEventTimer instances to
      delete after the primary loop, since at this point we're looping over a
      snapshot and can directly delete each completed timer.
      0c42f50d
    • Nat Goodspeed's avatar
    • Nat Goodspeed's avatar
      DRTVWR-494: Improve thread safety of LLSingleton machinery. · b22f89c9
      Nat Goodspeed authored
      Remove warnings about LLSingleton not being thread-safe because, at this point,
      we have devoted considerable effort to trying to make it thread-safe.
      
      Add LLSingleton<T>::Locker, a nested class which both provides a function-
      static mutex and a scoped lock that uses it. Instantiating Locker, which has a
      nullary constructor, replaces the somewhat cumbersome idiom of declaring a
      std::unique_lock<std::recursive_mutex> lk(getMutex);
      
      This eliminates (or rather, absorbs) the typedefs and getMutex() method from
      LLParamSingleton. Replace explicit std::unique_lock declarations in
      LLParamSingleton methods with Locker declarations.
      
      Remove LLSingleton<T>::SingletonInitializer nested struct. Instead of
      getInstance() relying on function-static initialization to protect (only)
      constructSingleton() calls, explicitly use a Locker instance to cover its
      whole scope, and make the UNINITIALIZED case call constructSingleton().
      Rearrange cases so that after constructSingleton(), control falls through to
      the CONSTRUCTED case and the finishInitializing() call.
      
      Use a Locker instance in other public-facing methods too: instanceExists(),
      wasDeleted(), ~LLSingleton(). Make destructor protected so it can only be called
      via deleteSingleton() (but must be accessible to subclasses for overrides).
      
      Remove LLSingletonBase::get_master() and get_initializing(), which permitted
      directly manipulating the master list and the initializing stack without any
      locking mechanism. Replace with get_initializing_size().
      
      Similarly, replace LLSingleton_manage_master::get_initializing() with
      get_initializing_size(). Use in constructSingleton() in place of
      get_initializing().size().
      
      Remove LLSingletonBase::capture_dependency()'s list_t parameter, which
      accepted the list returned by get_initializing(). Encapsulate that retrieval
      within the scope of the new lock in capture_dependency().
      
      Add LLSingleton_manage_master::capture_dependency(LLSingletonBase*, EInitState)
      to forward (or not) a call to LLSingletonBase::capture_dependency(). Nullary
      LLSingleton<T>::capture_dependency() calls new LLSingleton_manage_master method.
      
      Equip LLSingletonBase::MasterList with a mutex of its own, separate from the
      one donated by the LLSingleton machinery, to serialize use of MasterList data
      members. Introduce MasterList::Lock nested class to lock the MasterList mutex
      while providing a reference to the MasterList instance. Introduce subclasses
      LockedMaster, which provides a reference to the actual mMaster master list
      while holding the MasterList lock; and LockedInitializing, which does the same
      for the initializing list. Make mMaster and get_initializing_() private so
      that consuming code can *only* access those lists via LockedInitializing and
      LockedMaster.
      
      Make MasterList::cleanup_initializing_() private, with a LockedInitializing
      public forwarding method. This avoids another call to MasterList::instance(),
      and also mandates that the lock is currently held during every call.
      
      Similarly, move LLSingletonBase::log_initializing() to a LockedInitializing
      log() method.
      (transplanted from dca0f16266c7bddedb51ae7d7dca468ba87060d5)
      b22f89c9
    • Nat Goodspeed's avatar
    • Nat Goodspeed's avatar
      DRTVWR-494: Defend LLInstanceTracker against multi-thread usage. · 9d5b8976
      Nat Goodspeed authored
      The previous implementation went to some effort to crash if anyone attempted
      to create or destroy an LLInstanceTracker subclass instance during traversal.
      That restriction is manageable within a single thread, but becomes unworkable
      if it's possible that a given subclass might be used on more than one thread.
      
      Remove LLInstanceTracker::instance_iter, beginInstances(), endInstances(),
      also key_iter, beginKeys() and endKeys(). Instead, introduce key_snapshot()
      and instance_snapshot(), the only means of iterating over LLInstanceTracker
      instances. (These are intended to resemble functions, but in fact the current
      implementation simply presents the classes.) Iterating over a captured
      snapshot defends against container modifications during traversal. The term
      'snapshot' reminds the coder that a new instance created during traversal will
      not be considered. To defend against instance deletion during traversal, a
      snapshot stores std::weak_ptrs which it lazily dereferences, skipping on the
      fly any that have expired.
      
      Dereferencing instance_snapshot::iterator gets you a reference rather than a
      pointer. Because some use cases want to delete all existing instances, add an
      instance_snapshot::deleteAll() method that extracts the pointer. Those cases
      used to require explicitly copying instance pointers into a separate
      container; instance_snapshot() now takes care of that. It remains the caller's
      responsibility to ensure that all instances of that LLInstanceTracker subclass
      were allocated on the heap.
      
      Replace unkeyed static LLInstanceTracker::getInstance(T*) -- which returned
      nullptr if that instance had been destroyed -- with new getWeak() method
      returning std::weak_ptr<T>. Caller must detect expiration of that weak_ptr.
      
      Adjust tests accordingly.
      
      Use of std::weak_ptr to detect expired instances requires engaging
      std::shared_ptr in the constructor. We now store shared_ptrs in the static
      containers (std::map for keyed, std::set for unkeyed).
      
      Make LLInstanceTrackerBase a template parameterized on the type of the static
      data it manages. For that reason, hoist static data class declarations out of
      the class definitions to an LLInstanceTrackerStuff namespace.
      
      Remove the static atomic sIterationNestDepth and its methods incrementDepth(),
      decrementDepth() and getDepth(), since they were used only to forbid creation
      and destruction during traversal.
      
      Add a std::mutex to static data. Introduce an internal LockStatic class that
      locks the mutex while providing a pointer to static data, making that the only
      way to access the static data.
      
      The LLINSTANCETRACKER_DTOR_NOEXCEPT macro goes away because we no longer
      expect ~LLInstanceTracker() to throw an exception in test programs.
      That affects LLTrace::StatBase as well as LLInstanceTracker itself.
      
      Adapt consumers to the new LLInstanceTracker API.
      9d5b8976
    • Nat Goodspeed's avatar
      DRTVWR-494: Show copy-paste-friendly env vars and test command. · 70a0b520
      Nat Goodspeed authored
      Moderately often I want to copy the (long) integration test program path from
      build output and rerun the test program by hand. But typically we need
      environment variables set as well so it can find its dynamic libraries. This
      has resulted in my copying parts of several lines of build output, then
      pasting to a command prompt, then hand-tweaking the pasted text so it makes
      sense as a command.
      
      Streamline run_build_test.py output so less hand-tweaking is needed.
      70a0b520
  2. Mar 18, 2020
  3. Mar 10, 2020
  4. Feb 25, 2020
  5. Feb 21, 2020
  6. Feb 20, 2020
  7. Feb 04, 2020
  8. Jan 30, 2020
  9. Jan 29, 2020
Loading