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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/scudo/standalone/tsd_exclusive.h:62
+                                       memory_order_seq_cst)) {
+      initLinkerInitialized(Instance); // Sets Once to Done.
+    } else {
----------------
morehouse wrote:
> With this change, we now expect initialization to only happen through `initThreadMaybe`, right?  Should we even keep `initLinkerInitialized` around then? 
If I did things correctly, the Registry can also now be initialized with `initLinkerInitialized`, which will carry out the "once" initialization. Then `initThreadMaybe` will do the thread initialization skipping the "once".
My current plan is to have everything lazy initialized the first time someone calls `malloc` (or whatever else), meaning the first `initThreadMaybe` will carry the "once" initialization, but the alternative of pre-initializing is now offered.


================
Comment at: lib/scudo/standalone/tsd_exclusive.h:67
+      while (atomic_load(&Once, memory_order_seq_cst) != Done)
+        atomic_thread_fence(memory_order_seq_cst);
+    }
----------------
morehouse wrote:
> Is the fence necessary?  Not an atomic expert, but doesn't the load achieve the same thing?
Not an atomic expert either, it's inspired from from `llvm::call_once` which does this.
I figured it had a purpose and that I should keep it.


================
Comment at: lib/scudo/standalone/tsd_exclusive.h:77
+      initOnce(Instance);
+      atomic_thread_fence(memory_order_seq_cst);
+    }
----------------
morehouse wrote:
> I think things would be much simpler if we can use a `StaticSpinMutex`.  We're assuming we're zero-initialized anyway, so the mutex will be initialized before use as well.
I agree. But the issue that would come up using `StaticSpinMutex`, is that we would land in `initLinkerInitialized`where we should call the Mutex's `initLinkerInitialized` (to be consistent), while holding it. It works because it's a noop, but doesn't make sense from a logical perspective.
Implementing it this way (with our wannabe `call_once`) allows us to not have such a loophole.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D62258