fix: Remove race condition in memory allocator initialization

[yurekami]

Summary

• Fixes Potential Race Condition in Memory Allocator Initialization #315: Potential Race Condition in Memory Allocator Initialization
• Removes the unsynchronized gAllocatorInited flag that caused a race condition
• Relies solely on std::call_once which provides proper thread-safety guarantees

Problem

The original code had a race condition in GlobalMemoryAllocator.cc:

static bool gAllocatorInited = false;  // Plain bool, not atomic

void *allocate(size_t size) {
  if (!gAllocatorInited) {  // Multiple threads can pass this check
    std::call_once(gInitOnce, loadMemoryAllocatorLib);
    gAllocatorInited = true;  // Can be reordered with gAllocator write
  }

Issues:

1. gAllocatorInited is a plain bool, not atomic - no synchronization
2. Instruction reordering: gAllocatorInited = true can execute before std::call_once fully completes
3. Memory visibility: other threads may see gAllocatorInited = true while gAllocator is still uninitialized

Solution

Remove the redundant flag and rely solely on std::call_once:

void *allocate(size_t size) {
  std::call_once(gInitOnce, loadMemoryAllocatorLib);

Why this works:

• std::call_once provides full happens-before guarantees (C++11 standard)
• After first execution, call_once becomes a fast single atomic load (efficient fast-path)
• No performance degradation - simpler code with same or better performance

Test plan

• Verify existing unit tests pass
• Code review for thread-safety correctness
• The fix is minimal and follows the principle of relying on well-tested standard library primitives

🤖 Generated with Claude Code

[Spacefish]

But wait, it doesn´t really remove a race condition.. It already calls std::call_once(gInitOnce, loadMemoryAllocatorLib);
the boolean flag is just an optimization to skip the std::call_once call.

The std::call_once takes 2-5 instructions (depending on arch and compiler) and the boolean flag check takes 1-3 instructions (depending on where the compiler puts the jump).

So the boolean flag is actually a perf optimization!

If you run into the "race condition" it will just fall back to the std::call_once and burn up 2-5 cycles more as long as the flag isn´t visible on the other threads.

[yurekami]

@Spacefish The flag is intended as a fast path, agreed, but it is not a benign one.

The harmless direction is the one you describe: a thread seeing a stale false falls into std::call_once and loses a few cycles. The problem is the other direction. gAllocatorInited is a plain bool, so the unsynchronized read is a data race in the formal sense (UB per [intro.races], and TSan reports it). Concretely, a reader that observes true skips call_once and therefore gets no acquire ordering, so nothing orders its later load of gAllocator after the writer's store. On x86 this is hard to hit, but on ARM the reader's two loads can be reordered: it can observe gAllocatorInited == true and still load gAllocator == nullptr.

That stale-null read is not a slow path, it is a correctness bug: allocate falls back to std::malloc, and once gAllocator becomes visible to that thread, deallocate frees the same pointer via gAllocator->deallocate. Allocating and freeing across two different allocators corrupts the heap.

On the perf side the flag buys less than it appears to: the call_once fast path in glibc/libstdc++ is itself roughly one atomic acquire load of the once flag. If a fast path is still wanted, the correct form is an std::atomic<bool> checked with an acquire load and set with a release store after call_once, which keeps the optimization without the race. Happy to switch the PR to that form if the maintainers prefer it.