[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)); }
+
----------------
cryptoad wrote:
> 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.
Is there a reason we need the init loop to begin with?  It would be nice to have a one-way initialization instead.


================
Comment at: lib/scudo/standalone/tsd_shared.h:23
+
+  ALWAYS_INLINE void initThreadMaybe(Allocator *Instance,
+                                     UNUSED bool MinimalInit) {
----------------
cryptoad wrote:
> morehouse wrote:
> > Should Allocator initialization just be done in `initLinkerInitialized`?
> This question made me realize that something is a bit sketchy.
> `initOnce` is initializing the allocator, which in turn is calling `initLinkerInitialized`of the registry: at this point we are in the spinmutex, and call the `initLinkerInitialized` method of that same mutex. It all works out because 1) everything is zeroinitialized 2) the `initLinkerInitialized` of the spinmutex is a no-op.
> But it is definitely logically skewed.
> The original version was using `pthread_once` which was alleviating the need for the mutex, but I needed the Instance parameter for `initOnce` (pthread_one doesn't allow parameters to the once function).
> I am going to have to rethink that.
Seems this is a consequence of the tight coupling between registry and allocator.

I think ideally we'd have a one-way dependency so either allocator-has-a-registry, or registry-has-an-allocator.  Then the initialization only ever makes sense one-way.

Loose coupling in general would also make it easier to understand each piece in isolation, which makes future code maintenance much easier.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D62258