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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked an inline comment as done.
cryptoad 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:
> 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)
Damn, I failed.
The other option was to implement a call_once type function, that would be specific to the registry, using an atomic cas (like llvm::call_once) and a little spin, and not have a mutex at all.
I can't use std::call_once and pthread_once, but if you have another idea, I am happy to oblige.


================
Comment at: lib/scudo/standalone/tsd_exclusive.h:60
+
+  NOINLINE void initThread(Allocator *Instance, bool MinimalInit) {
+    if (UNLIKELY(!OnceDone))
----------------
morehouse wrote:
> 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`.
Ah, I see what you mean. That is indeed the case, I'll reorganize that.


================
Comment at: lib/scudo/standalone/tsd_exclusive.h:64
+    if (MinimalInit)
+      return;
+    CHECK_EQ(
----------------
morehouse wrote:
> 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?
I'll add a short version of it here.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D62258