ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:64

• initialize() publishes the pool pointer unconditionally; if construction failed (e.g., _threads == nullptr), subsequent acquire()/release() calls can hit UB. Only publish the pool if it is usable; otherwise keep _pool null.

void ThreadLocalDataPool::initialize() {
    ThreadLocalDataPool* pool = new ThreadLocalDataPool();
    __atomic_store_n(&_pool, pool, __ATOMIC_RELEASE);
}

ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:18

• ThreadLocalDataPool doesn’t initialize _threads when malloc fails, leaving it indeterminate. That can lead to invalid free() in the destructor and crashes in claim()/contains(). Initialize _threads to nullptr in the ctor initializer list.

ThreadLocalDataPool::ThreadLocalDataPool(uint64_t capacity) : _capacity(capacity), _used(0) {
    size_t malloc_size = capacity * sizeof(ProfiledThread);
    void* p = malloc(malloc_size);
    if (p != nullptr) {
      _threads = reinterpret_cast<ProfiledThread*>(p);

ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:48

• On probe failure, claim() currently assert(false) after scanning the whole pool. Under races (or if _threads is unexpectedly null), this can abort the process from a signal handler. Prefer to roll back _used and return nullptr (dropping the sample) instead of asserting.

    do {
        if (_threads[index].claim_acquire(tid)) {
            return &_threads[index];
        }
        index = (index + 1) % _capacity;
    } while (index != start_pos);
    assert(false && "Should not reach here");
    return nullptr;

ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:56

• unclaim() has the same unsigned-decrement issue as claim() (using __atomic_fetch_add(..., -1, ...) on a uint16_t). Use __atomic_fetch_sub(..., 1, ...) so _used doesn’t wrap.

        new (t)ProfiledThread(0);
        uint16_t used = __atomic_fetch_add(&_used, -1, __ATOMIC_RELEASE);
        assert(used > 0);
        return true;

ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp:1226

• The previous code restored ThreadLocalData::_unwinding_Java after a recovered siglongjmp because siglongjmp bypasses AsyncSampleMutex destructors. That restore was removed, so a crash recovery can leave _unwinding_Java stuck true, preventing future Java stack walks on the thread.

  if (sigsetjmp(crash_protection_ctx, 1) != 0) {
    // checkFault() does a siglongjmp from inside segvHandler, bypassing
    // segvHandler's SignalHandlerScope destructor. Compensate.
    SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP();
    prof_thread->setJmpCtx(prev_jmp_buf);

ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:55

• unclaim() reconstructs ProfiledThread via placement-new while other threads may concurrently probe/claim the same slot. Because ProfiledThread uses atomic operations on _misc_flags, reinitializing it with non-atomic stores (constructor/placement-new) can race with those atomics (UB) and also makes it possible to observe a partially-reset object. Consider adding an explicit “reset for pool reuse” routine that keeps the slot in a claimed state while resetting fields, then clears FLAG_CLAIMED with a release store as the final step.

bool ThreadLocalDataPool::unclaim(ProfiledThread* t) {
    if (contains(t)) {
        new (t)ProfiledThread(0);
        uint16_t used = __atomic_fetch_add(&_used, -1, __ATOMIC_RELEASE);
        assert(used > 0);

ddprof-lib/src/main/cpp/threadLocalData.inline.h:18

• ProfiledThread::current() is defined here, but if it’s also defined inline in threadLocalData.h (to keep existing include sites compiling), this becomes a duplicate definition across TUs. Keep only one definition (e.g., define current() in threadLocalData.h and leave only acquire_current() here).

ProfiledThread* ProfiledThread::current() {
    if (!isThreadKeyValid()) {
      return nullptr;
    }
    return _current_thread.get();
}

ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:37

• __atomic_fetch_add(&_used, -1, ...) is performed on a uint16_t. The -1 is converted to uint16_t (65535), so this increments by 65535 (wraps) rather than decrementing. Use __atomic_fetch_sub(..., 1, ...) (and similarly in unclaim).

This issue also appears on line 53 of the same file.

    uint16_t used = __atomic_fetch_add(&_used, 1, __ATOMIC_RELAXED);
    if (used >= _capacity) {
        __atomic_fetch_add(&_used, -1, __ATOMIC_RELAXED);
        return nullptr;

ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:58

• unclaim() reconstructs the slot with placement-new (new (t) ProfiledThread(0)), which writes _misc_flags (and other fields) non-atomically while other threads may concurrently read _misc_flags via __atomic_* in claim_acquire(). This mixes atomic and non-atomic accesses to the same object and can also momentarily clear FLAG_CLAIMED before the slot reset is fully complete.

    return nullptr;
}

bool ThreadLocalDataPool::unclaim(ProfiledThread* t) {
    if (contains(t)) {

ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:15

• If malloc() fails in ThreadLocalDataPool's constructor, _threads is left uninitialized, but later code (including the destructor and contains()) assumes it is either a valid pointer or nullptr. This can lead to undefined behavior.

ThreadLocalDataPool::ThreadLocalDataPool(uint64_t capacity)

ddprof-lib/src/main/cpp/threadLocalData.h:125

• The assertion message in ProfiledThread::unclaim() is inverted: if the assert fires, the slot was not claimed, but the message says it "has been claimed".

    assert(isClaimed() && "Slot has been claimed");

ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:43

• ThreadLocalDataPool::claim() currently has a broken/missing capacity guard: the code unconditionally decrements _used and returns nullptr, and the braces are unbalanced, so this won’t compile and the pool can never hand out slots. Add the intended used >= _capacity check and close the block correctly.

    uint16_t used = __atomic_fetch_add(&_used, 1, __ATOMIC_RELAXED);
        __atomic_fetch_add(&_used, -1, __ATOMIC_RELAXED);
        return nullptr;
    }

ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:60

• unclaim() reconstructs the slot with placement-new (ProfiledThread(0)), which clears FLAG_CLAIMED via a non-atomic write to _misc_flags. That allows another thread to observe the slot as unclaimed and race in while the object is mid-reset. Prefer clearing the claimed bit atomically (the class already provides ProfiledThread::unclaim() for this) and let the next acquire() reinitialize the object.

    if (contains(t)) {
        new (t)ProfiledThread(0);
        uint16_t used = __atomic_fetch_add(&_used, -1, __ATOMIC_RELEASE);

ddprof-lib/src/main/cpp/threadLocalData.inline.h:20

• ProfiledThread::acquire_current() is defined in a header included by multiple translation units but is not marked inline, which can produce multiple-definition linker errors. Mark it inline.

ProfiledThread* ProfiledThread::acquire_current() {

ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:12

• This file uses placement-new (new (&_threads[index]) ...) but does not include <new>, which is required to declare placement-new in standard C++. Add the missing include to avoid build failures on stricter toolchains.

#include <cassert>
#include <stdlib.h>

ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:62

• _used was widened in the header, but this local variable is still uint16_t, which will truncate the atomic counter and can underflow/wrap incorrectly.

        uint16_t used = __atomic_fetch_add(&_used, -1, __ATOMIC_RELEASE);

ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:63

• unclaim() reconstructs the ProfiledThread in-place with placement-new while other threads may concurrently read/modify the slot (via claim_acquire() / _misc_flags). Re-ending/restarting an object’s lifetime and doing non-atomic writes to the same storage other threads touch is undefined behavior and can lead to double-claim or corrupted state. Consider keeping slot ownership state separate from the ProfiledThread object (e.g., a dedicated std::atomic<uint32_t> claim word per slot) and avoid placement-new on shared objects; reset per-thread state only after exclusive ownership is established.

bool ThreadLocalDataPool::unclaim(ProfiledThread* t) {
    if (contains(t)) {
        new (t)ProfiledThread(0);
        uint16_t used = __atomic_fetch_add(&_used, -1, __ATOMIC_RELEASE);
        assert(used > 0);

ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:83

• acquire() reconstructs a claimed ProfiledThread in-place. Even though the slot is "claimed", other threads may still probe its claim state concurrently (via _misc_flags), and reconstructing the object restarts its lifetime while concurrent reads are possible. This is undefined behavior in C++ and can manifest as intermittent races. Prefer a separate per-slot claim flag (outside the object being reconstructed), or avoid placement-new and instead reset fields under exclusive ownership without touching memory concurrently accessed by other threads.

        ProfiledThread* t = pool->claim(tid);
        if (t != nullptr) {
            new (t)ProfiledThread(tid, true /* claimed */);
        }
        return t;

ddprof-lib/src/main/cpp/threadLocalData.h:183

• ProfiledThread::current() is declared inline here but no longer defined in this header. Several existing translation units still include threadLocalData.h (not threadLocalData.inline.h) and call ProfiledThread::current(), which will fail to compile. Either keep current() defined here (as before) or make threadLocalData.h include the inline definitions.

  // Signal-handler friendly (no allocation): returns existing TLS or nullptr.
  static inline ProfiledThread *current();
  // signal-handler friendly with priming: return existing TLS or acquire and set
  // ProfiledThread from ThreadLocalDataPool.
  static inline ProfiledThread* acquire_current();

ddprof-lib/src/main/cpp/threadLocalData.inline.h:13

• With ProfiledThread::current() defined back in threadLocalData.h, this out-of-class definition becomes a duplicate definition when threadLocalData.inline.h is included (directly or indirectly). Remove it to avoid redefinition errors.

inline ProfiledThread* ProfiledThread::current() {

ddprof-lib/src/main/cpp/threadLocalDataPool.h:20

• _used is a 16-bit counter but _capacity is 64-bit; if the pool capacity is ever increased beyond 65535, _used will wrap and the full/empty checks become incorrect. Use a wider counter type that can represent _capacity.

    const uint64_t      _capacity;
    volatile uint16_t   _used;
    ProfiledThread*     _threads;

ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:41

• _used was widened in the header, but this local variable is still uint16_t, which will truncate the atomic counter and break capacity checks once _used exceeds 65535.

This issue also appears on line 62 of the same file.

    uint16_t used = __atomic_fetch_add(&_used, 1, __ATOMIC_RELAXED);

ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:72

• This PR introduces a new concurrent, signal-path critical allocation strategy (ThreadLocalDataPool + ProfiledThread::acquire_current()), but there are no accompanying C++ unit tests validating pool exhaustion behavior, claim/release correctness, or the interaction with TLS teardown (ProfiledThread::freeValue). The repo has an existing gtest suite under ddprof-lib/src/test/cpp/; please add targeted tests to lock in correctness.

void ThreadLocalDataPool::initialize() {
    ThreadLocalDataPool* pool = new ThreadLocalDataPool();
    __atomic_store_n(&_pool, pool, __ATOMIC_RELEASE);
}

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".