[PATCH] D62258: [scudo][standalone] Introduce the thread specific data structures

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: lib/scudo/standalone/tsd_exclusive.h:28
+  void initLinkerInitialized() { Mutex.initLinkerInitialized(); }
+  void init() { memset(this, 0, sizeof(*this)); }
+
----------------
morehouse wrote:
> Shouldn't this also call `initLinkerInitialized`?
I think switching to `BlockingMutex` introduced a new bug.  `BlockingMutex` has a default constructor that will run at global ctor time.  So the following sequence could happen:

# `TSDRegistryExT::initLInkerInitialized()`
# `Mutex.lock()`
# `TSDRegistryExT()` implicit constructor runs, calling `Mutex`'s ctor.

So now `Mutex` is unlocked even though `unlock` was never called.


(also applies to the other registry)


================
Comment at: lib/scudo/standalone/tsd_exclusive.h:60
+
+  NOINLINE void initThread(Allocator *Instance, bool MinimalInit) {
+    if (UNLIKELY(!OnceDone))
----------------
cryptoad wrote:
> morehouse wrote:
> > Will threads have different allocators?
> Multiple allocators can coexist in a process, hence a need to know which allocator a registry belongs to.
Ok, but will a single registry ever have multiple allocators?  If not, we don't need to pass the allocator to the therad-specific `initThread` and should instead pass it to the global `init`.


================
Comment at: lib/scudo/standalone/tsd_exclusive.h:64
+    if (MinimalInit)
+      return;
+    CHECK_EQ(
----------------
cryptoad wrote:
> morehouse wrote:
> > What is the use-case for `MinimalInit` initialization?
> Right, this is documented in combined.h that hasn't landed yet, here is the comment:
> ```
>     // For a deallocation, we only ensure minimal initialization, meaning thread
>     // local data will be left uninitialized for now (when using ELF TLS). The
>     // fallback cache will be used instead. This is a workaround for a situation
>     // where the only heap operation performed in a thread would be a free past
>     // the TLS destructors, ending up in initialized thread specific data never
>     // being destroyed properly. Any other heap operation will do a full init.
> ```
> There is test case in the current Scudo for that situation that will be in as well.
Thanks for the explanation.  Would it make sense to put that comment here, or is the combined a better place?


Repository:
  rCRT Compiler Runtime

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62258/new/

https://reviews.llvm.org/D62258