From 4b5cb1b193dd633c725cf31a41d555ff82f11e45 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Fri, 23 Oct 2026 17:50:09 +0200 Subject: [PATCH 1/5] Fix stub-lock deadlock in DynamicCodeGenerated Bound signal-handler stub lookups and move allocations out of the exclusive stubs lock. Closes #843 JIRA: PROF-16313 Co-Authored-By: Claude Opus 5.5 --- ddprof-lib/src/main/cpp/codeCache.cpp | 65 +++-- ddprof-lib/src/main/cpp/codeCache.h | 38 ++- ddprof-lib/src/main/cpp/counters.h | 5 + .../src/main/cpp/hotspot/hotspotSupport.cpp | 18 +- .../src/main/cpp/hotspot/jitCodeCache.cpp | 123 +++++++--- .../src/main/cpp/hotspot/jitCodeCache.h | 35 ++- ddprof-lib/src/test/cpp/codeCache_ut.cpp | 41 ++++ ddprof-lib/src/test/cpp/jitCodeCache_ut.cpp | 227 ++++++++++++++++++ 8 files changed, 461 insertions(+), 91 deletions(-) create mode 100644 ddprof-lib/src/test/cpp/jitCodeCache_ut.cpp diff --git a/ddprof-lib/src/main/cpp/codeCache.cpp b/ddprof-lib/src/main/cpp/codeCache.cpp index 943e7481ff..5af326a464 100644 --- a/ddprof-lib/src/main/cpp/codeCache.cpp +++ b/ddprof-lib/src/main/cpp/codeCache.cpp @@ -176,8 +176,8 @@ CodeCache::~CodeCache() { } long long CodeCache::memoryUsage() const { - // O(1): _memory_usage is updated incrementally by the constructor, add(), - // expand(), and setDwarfTable() -- the same pre-publication mutators that + // O(1): _memory_usage is updated incrementally by the constructor, + // add()/commitAdd(), and setDwarfTable() -- the same pre-publication mutators that // used to be re-summed here on every call. Safe to read lock-free once // published (Libraries::native_libs) because those mutators never run // afterwards (asserted via _published), same invariant the old per-call @@ -191,25 +191,7 @@ long long CodeCache::memoryUsage() const { return _memory_usage; } -void CodeCache::expand() { - // Must not run after publication: memoryUsage() reads _blobs lock-free from - // the dump thread and a concurrent realloc would free it underneath. - assert(!_published.load(std::memory_order_acquire) && - "expand() on a published CodeCache races memoryUsage()"); - CodeBlob *old_blobs = _blobs; - CodeBlob *new_blobs = new CodeBlob[_capacity * 2]; - - memcpy(new_blobs, old_blobs, _count * sizeof(CodeBlob)); - - // Delta is exactly the current (pre-doubling) capacity's worth of blobs. - _memory_usage += (long long)_capacity * sizeof(CodeBlob); - _capacity *= 2; - _blobs = new_blobs; - delete[] old_blobs; -} - -void CodeCache::add(const void *start, int length, const char *name, - bool update_bounds) { +void CodeCache::prepareAdd(const char *name, PreparedAdd &prepared) const { // Symbols are added while parsing a library, before it is registered into a // CodeCacheArray. Adding after publication would race memoryUsage() (which // reads _blobs/_count lock-free at dump time). Unpublished standalone caches @@ -217,29 +199,62 @@ void CodeCache::add(const void *start, int length, const char *name, assert(!_published.load(std::memory_order_acquire) && "add() on a published CodeCache races memoryUsage()"); char *name_copy = NativeFunc::create(name, _lib_index); - _memory_usage += (long long)NativeFunc::allocSize(name); + prepared.name = name_copy; + prepared.name_size = (long long)NativeFunc::allocSize(name); // Measured off the block just handed back, while the pointer is in hand: the // gauge is published as a total at add() time, with no pointer left to probe. - _name_overhead += (long long)NativeFunc::nameOverhead(name_copy); + prepared.name_overhead = (long long)NativeFunc::nameOverhead(name_copy); // Replace non-printable characters for (char *s = name_copy; *s != 0; s++) { if (*s < ' ') *s = '?'; } + prepared.capacity = _capacity; + prepared.grown = nullptr; if (_count >= _capacity) { - expand(); + prepared.grown = new CodeBlob[_capacity * 2]; + memcpy(prepared.grown, _blobs, _count * sizeof(CodeBlob)); + } +} + +CodeBlob *CodeCache::commitAdd(const void *start, int length, + const PreparedAdd &prepared, + bool update_bounds) { + assert(!_published.load(std::memory_order_acquire) && + "add() on a published CodeCache races memoryUsage()"); + // A mutation between prepareAdd() and commitAdd() would make the grown copy + // stale or leave no free slot. + assert(prepared.capacity == _capacity && + (prepared.grown != nullptr || _count < _capacity)); + CodeBlob *retired = nullptr; + if (prepared.grown != nullptr) { + // Delta is exactly the current (pre-doubling) capacity's worth of blobs. + _memory_usage += (long long)_capacity * sizeof(CodeBlob); + _capacity *= 2; + retired = _blobs; + _blobs = prepared.grown; } + _memory_usage += prepared.name_size; + _name_overhead += prepared.name_overhead; const void *end = (const char *)start + length; _blobs[_count]._start = start; _blobs[_count]._end = end; - _blobs[_count]._name = name_copy; + _blobs[_count]._name = prepared.name; _count++; if (update_bounds) { updateBounds(start, end); } + return retired; +} + +void CodeCache::add(const void *start, int length, const char *name, + bool update_bounds) { + PreparedAdd prepared; + prepareAdd(name, prepared); + delete[] commitAdd(start, length, prepared, update_bounds); } void CodeCache::updateBounds(const void *start, const void *end) { diff --git a/ddprof-lib/src/main/cpp/codeCache.h b/ddprof-lib/src/main/cpp/codeCache.h index 951cfc610a..8f44cdce60 100644 --- a/ddprof-lib/src/main/cpp/codeCache.h +++ b/ddprof-lib/src/main/cpp/codeCache.h @@ -181,7 +181,7 @@ class CodeCache { CodeBlob *_blobs; // Running total of memoryUsage()'s formula, updated incrementally by the - // same pre-publication mutators (constructor, add(), expand(), + // same pre-publication mutators (constructor, add()/commitAdd(), // setDwarfTable()) instead of recomputed by iterating _blobs. Safe because // every contributing field is fixed before publication (see _published // below) -- there is no point after construction-and-population where this @@ -204,17 +204,17 @@ class CodeCache { // Set once the cache is registered into a CodeCacheArray (see markPublished()). // After that, memoryUsage() may be read lock-free from another thread (dump), - // so the mutators that touch the fields it reads (add()/expand()/ - // setDwarfTable()) must not run — asserted in debug. Standalone caches that + // so the mutators that touch the fields it reads (add()/prepareAdd()/ + // commitAdd()/setDwarfTable()) must not run — asserted in debug. Standalone caches that // are never registered (e.g. JitCodeCache::_runtime_stubs, which is instead - // guarded by its own JitCodeCache::_stubs_lock) stay unpublished, so the + // guarded by its own JitCodeCache::_stubs_lock, with writers serialized by + // JitCodeCache::_stubs_write_lock) stay unpublished, so the // asserts never fire for them. Atomic with release/acquire so the flag is // observably true on any thread that reaches a published cache — the store is // sequenced before CodeCacheArray::add()'s RELEASE publish of the pointer, and // the assert loads it with acquire — without a formal data race. std::atomic _published; - void expand(); void makeImportsPatchable(); void saveImport(ImportId id, void** entry); void copyFrom(const CodeCache& other); @@ -282,12 +282,34 @@ class CodeCache { void setImageFingerprint(u64 fingerprint) { _image_fingerprint = fingerprint; } // Mark this cache as published into a CodeCacheArray. Call before the array - // makes the pointer visible to readers; afterwards add()/expand()/ - // setDwarfTable() must not be called (see _published). + // makes the pointer visible to readers; afterwards add()/prepareAdd()/ + // commitAdd()/setDwarfTable() must not be called (see _published). void markPublished() { _published.store(true, std::memory_order_release); } void add(const void *start, int length, const char *name, bool update_bounds = false); + + // Heap allocations that prepareAdd() makes for exactly one later commitAdd(). + struct PreparedAdd { + char *name; // owned NativeFunc name copy, non-printables replaced + long long name_size; // NativeFunc::allocSize() of the name + long long name_overhead; // NativeFunc::nameOverhead() of the copy + CodeBlob *grown; // filled copy of _blobs at twice the capacity, or + // nullptr when the current array has a free slot + int capacity; // _capacity that this preparation is valid for + }; + + // Two-phase add() for a cache whose readers run in signal handlers under a + // SpinLock (JitCodeCache::_runtime_stubs): prepareAdd() makes every heap + // allocation and can run outside the lock; commitAdd() only stores and + // copies pointers, so it is safe under the exclusive lock. commitAdd() + // returns the replaced blob array (or nullptr), which the caller must + // delete[] after it releases the lock. Every prepareAdd() must be followed by + // exactly one commitAdd(), with no other mutation of this cache in between; + // the caller serializes the pair against other writers. + void prepareAdd(const char *name, PreparedAdd &prepared) const; + CodeBlob *commitAdd(const void *start, int length, + const PreparedAdd &prepared, bool update_bounds = false); void updateBounds(const void *start, const void *end); void sort(); @@ -407,7 +429,7 @@ class CodeCacheArray { // so it stays consistent with the logical bytes above. NativeMem::recordOverhead(NM_NATIVE_SYMBOLS, lib->nameOverhead()); // Mark published before the RELEASE store makes the pointer visible, so any - // later add()/expand()/setDwarfTable() on this cache trips the assert (its + // later add()/prepareAdd()/commitAdd()/setDwarfTable() on this cache trips the assert (its // _blobs would then be read lock-free by memoryUsage() at dump time). lib->markPublished(); // Store pointer before publishing count. The RELEASE here pairs with diff --git a/ddprof-lib/src/main/cpp/counters.h b/ddprof-lib/src/main/cpp/counters.h index e602a5da4d..d6113f4f82 100644 --- a/ddprof-lib/src/main/cpp/counters.h +++ b/ddprof-lib/src/main/cpp/counters.h @@ -136,6 +136,11 @@ X(WALKVM_STUB_INFO_HIT, "walkvm_stub_info_hit") \ X(WALKVM_STUB_INFO_FALLBACK, "walkvm_stub_info_fallback") \ X(WALKVM_STUB_INFO_REGISTRATION_FAILED, "walkvm_stub_info_registration_failed") \ + /* Runtime-stub lookups (JitCodeCache::findRuntimeStub/findRuntimeStubInfo) \ + * that returned "not found" without searching because the shared stubs \ + * lock was held exclusively by a registration or the shared spin budget \ + * ran out; the stack walk continued with its stub-not-found fallback. */ \ + X(STUB_LOOKUP_LOCK_UNAVAILABLE, "stub_lookup_lock_unavailable") \ X(WALKVM_FP_CHAIN_ATTEMPT, "walkvm_fp_chain_attempt") \ X(WALKVM_FP_CHAIN_REACHED_CODEHEAP, "walkvm_fp_chain_reached_codeheap") \ X(WALKVM_ANCHOR_NOT_IN_JAVA, "walkvm_anchor_not_in_java") \ diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index 52bf894354..92598afdc9 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -761,9 +761,9 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } } - // One shared-lock acquisition returns the stub view plus the - // precomputed aarch64 unwind info (null on x64): the signal - // handler still takes _stubs_lock exactly once. + // At most one bounded shared-lock attempt returns the stub view + // plus the precomputed aarch64 unwind info (null on x64). When + // the lookup fails, the nmethod's own start and name are used. const void* start; const char* name; const StubUnwindInfo* stub_info = nullptr; @@ -773,9 +773,10 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex name = nm->name(); } #else - CodeBlob* stub = JitCodeCache::findRuntimeStub(walk_pc.raw()); - start = stub != NULL ? stub->_start : nm->code(); - name = stub != NULL ? stub->_name : nm->name(); + if (!JitCodeCache::findRuntimeStub(walk_pc.raw(), &start, &name)) { + start = nm->code(); + name = nm->name(); + } #endif if (details) { @@ -1170,10 +1171,7 @@ int HotspotSupport::getJavaTraceAsync(void *ucontext, ASGCT_CallFrame *frames, #ifdef __aarch64__ if (JitCodeCache::findRuntimeStubInfo((const void *)frame.pc(), &stub_start, &stub_name, &stub_info)) { #else - CodeBlob *stub = JitCodeCache::findRuntimeStub((const void *)frame.pc()); - if (stub != NULL) { - stub_start = stub->_start; - stub_name = stub->_name; + if (JitCodeCache::findRuntimeStub((const void *)frame.pc(), &stub_start, &stub_name)) { #endif if (cstack != CSTACK_NO) { max_depth -= makeFrame(trace.frames++, BCI_NATIVE_FRAME, stub_name); diff --git a/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.cpp b/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.cpp index df75e6cf0b..6c1e7e8c71 100644 --- a/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.cpp @@ -8,6 +8,7 @@ #include #include +#include "counters.h" #include "hotspot/vmStructs.h" #include "threadLocalData.h" #ifdef __aarch64__ @@ -15,6 +16,7 @@ #endif SpinLock JitCodeCache::_stubs_lock; +Mutex JitCodeCache::_stubs_write_lock; CodeCache JitCodeCache::_runtime_stubs("[stubs]"); std::atomic JitCodeCache::_call_stub_begin = { nullptr }; std::atomic JitCodeCache::_call_stub_end = { nullptr }; @@ -24,11 +26,12 @@ std::atomic JitCodeCache::_call_stub_end = { nullptr }; // signal-handler lookup is a binary search. Inserts happen under the // exclusive _stubs_lock and each entry's info pointer is fixed at // registration time, so a reader holding the shared lock sees a stable -// snapshot: the same lock serializes the growing realloc with the scan, and -// the full-barrier CAS in SpinLock publishes the new contents. Info objects -// are malloc'ed once at DynamicCodeGenerated time and never freed -- a signal -// handler may still be reading one -- matching the lifetime of the stub name -// strings. +// snapshot. A full array is replaced by a larger copy, allocated and filled +// before the exclusive lock and swapped in under it; the old array is freed +// only after the unlock, when no reader can still hold it. The full-barrier +// CAS in SpinLock publishes the new contents. Published info objects are +// never freed -- a signal handler may still be reading one -- matching the +// lifetime of the stub name strings. StubUnwindEntry* JitCodeCache::_stub_unwind = nullptr; int JitCodeCache::_stub_unwind_count = 0; int JitCodeCache::_stub_unwind_capacity = 0; @@ -60,10 +63,13 @@ void JNICALL JitCodeCache::CompiledMethodLoad(jvmtiEnv *jvmti, jmethodID method, void JNICALL JitCodeCache::DynamicCodeGenerated(jvmtiEnv *jvmti, const char *name, const void *address, jint length) { ProfiledThread::initCurrentThreadSignalSafe(); + // Registrations can arrive concurrently from several threads. The writer + // mutex orders them, so the state read while preparing below stays valid + // until the commit. Signal handlers never take it. + MutexLocker writer(_stubs_write_lock); #ifdef __aarch64__ // Analyze before taking the lock: the classifier decodes every instruction - // and allocates, which must not happen while sampling threads spin on the - // shared lock. Publication is ordered by the exclusive lock below -- a + // and allocates. Publication is ordered by the exclusive lock below -- a // reader that finds the stub always finds its info. StubUnwindInfo* stub_info = nullptr; if (shouldAnalyzeStub(name)) { @@ -76,14 +82,17 @@ void JNICALL JitCodeCache::DynamicCodeGenerated(jvmtiEnv *jvmti, const char *nam : WALKVM_STUB_INFO_UNCLASSIFIED); } #endif - _stubs_lock.lock(); - _runtime_stubs.add(address, length, name, true); + // Every heap allocation and free happens outside the exclusive lock. A + // thread that holds the allocator lock can be interrupted by a profiling + // signal whose handler looks up a stub; if this thread then blocked in the + // allocator while holding _stubs_lock, that lookup could never succeed. + CodeCache::PreparedAdd prepared; + _runtime_stubs.prepareAdd(name, prepared); #ifdef __aarch64__ // A re-registration of the same range (never observed for HotSpot runtime // stubs, but DynamicCodeGenerated makes no promises) keeps the previously // registered info: the old object cannot be freed because a signal handler // may still hold a pointer to it. - bool duplicate = false; int lo = 0, hi = _stub_unwind_count; while (lo < hi) { int mid = (lo + hi) / 2; @@ -93,39 +102,60 @@ void JNICALL JitCodeCache::DynamicCodeGenerated(jvmtiEnv *jvmti, const char *nam hi = mid; } } - if (lo < _stub_unwind_count && _stub_unwind[lo]._start == address) { - duplicate = true; - } - if (!duplicate && _stub_unwind_count == _stub_unwind_capacity) { - int new_capacity = _stub_unwind_capacity == 0 ? 64 : _stub_unwind_capacity * 2; - StubUnwindEntry* grown = - (StubUnwindEntry*)realloc(_stub_unwind, new_capacity * sizeof(StubUnwindEntry)); - if (grown != nullptr) { - _stub_unwind = grown; - _stub_unwind_capacity = new_capacity; + bool insert = !(lo < _stub_unwind_count && _stub_unwind[lo]._start == address); + StubUnwindEntry* grown_unwind = nullptr; + int grown_capacity = 0; + if (insert && _stub_unwind_count == _stub_unwind_capacity) { + grown_capacity = _stub_unwind_capacity == 0 ? 64 : _stub_unwind_capacity * 2; + grown_unwind = (StubUnwindEntry*)malloc(grown_capacity * sizeof(StubUnwindEntry)); + if (grown_unwind != nullptr) { + // _stub_unwind is null with zero count on the first growth; memcpy + // requires a valid source even for zero bytes. + if (_stub_unwind_count > 0) { + memcpy(grown_unwind, _stub_unwind, _stub_unwind_count * sizeof(StubUnwindEntry)); + } + } else { + // Capacity could not be grown: this stub (and every later one) loses + // precomputed unwinding silently unless the outcome is counted. + Counters::increment(WALKVM_STUB_INFO_REGISTRATION_FAILED); + insert = false; } } - if (!duplicate && _stub_unwind_count < _stub_unwind_capacity) { + if (!insert) { + // Either a duplicate (the registered info stays) or a failed growth. The + // info was never published, so no reader can hold it. + free(stub_info); + stub_info = nullptr; + } + StubUnwindEntry* retired_unwind = nullptr; +#endif + + _stubs_lock.lock(); + CodeBlob* retired_blobs = _runtime_stubs.commitAdd(address, length, prepared, true); +#ifdef __aarch64__ + if (grown_unwind != nullptr) { + retired_unwind = _stub_unwind; + _stub_unwind = grown_unwind; + _stub_unwind_capacity = grown_capacity; + } + if (insert) { // Sorted insert: shift the tail right to keep the lookup binary-searchable. memmove(&_stub_unwind[lo + 1], &_stub_unwind[lo], (_stub_unwind_count - lo) * sizeof(StubUnwindEntry)); _stub_unwind[lo]._start = address; _stub_unwind[lo]._info = stub_info; _stub_unwind_count++; - } else { - if (duplicate) { - // The freshly analyzed info is redundant; the registered one stays. - free(stub_info); - } else { - // Capacity could not be grown: this stub (and every later one) loses - // precomputed unwinding silently unless the outcome is counted. - Counters::increment(WALKVM_STUB_INFO_REGISTRATION_FAILED); - } - stub_info = nullptr; } #endif _stubs_lock.unlock(); + // Readers take the shared lock for every access, so after the unlock no + // reader can still use the replaced arrays. + delete[] retired_blobs; +#ifdef __aarch64__ + free(retired_unwind); +#endif + if (name[0] == 'I' && strcmp(name, "Interpreter") == 0) { CodeHeap::setInterpreterStart(address); } else if (strcmp(name, "call_stub") == 0) { @@ -138,22 +168,39 @@ void JNICALL JitCodeCache::DynamicCodeGenerated(jvmtiEnv *jvmti, const char *nam CodeHeap::updateBounds(address, (const char *)address + length); } -CodeBlob* JitCodeCache::findRuntimeStub(const void *address) { - CodeBlob *stub = nullptr; - _stubs_lock.lockShared(); +bool JitCodeCache::findRuntimeStub(const void *address, const void **start, + const char **name) { + // Bounded: the caller is a signal handler, and the exclusive holder may be + // the interrupted thread itself. + if (!_stubs_lock.tryLockShared(SpinLock::DEFAULT_SHARED_SPIN_BUDGET)) { + Counters::increment(STUB_LOOKUP_LOCK_UNAVAILABLE); + return false; + } + bool found = false; if (_runtime_stubs.contains(address)) { - stub = _runtime_stubs.findBlobByAddress(address); + CodeBlob *blob = _runtime_stubs.findBlobByAddress(address); + if (blob != nullptr) { + // Copy out under the lock: a registration may replace the blob array + // as soon as the lock is released. + *start = blob->_start; + *name = blob->_name; + found = true; + } } _stubs_lock.unlockShared(); - return stub; + return found; } #ifdef __aarch64__ bool JitCodeCache::findRuntimeStubInfo(const void* address, const void** start, const char** name, const StubUnwindInfo** info) { - bool found = false; *info = nullptr; - _stubs_lock.lockShared(); + // Bounded for the same reason as findRuntimeStub(). + if (!_stubs_lock.tryLockShared(SpinLock::DEFAULT_SHARED_SPIN_BUDGET)) { + Counters::increment(STUB_LOOKUP_LOCK_UNAVAILABLE); + return false; + } + bool found = false; if (_runtime_stubs.contains(address)) { CodeBlob* blob = _runtime_stubs.findBlobByAddress(address); if (blob != nullptr) { diff --git a/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.h b/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.h index c2e02b221b..28961f123b 100644 --- a/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.h +++ b/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.h @@ -9,6 +9,7 @@ #include #include "codeCache.h" +#include "mutex.h" #include "spinLock.h" #include "hotspot/vmStructs.h" @@ -19,10 +20,11 @@ // hotspot/stubUnwindInfo.h). struct StubUnwindInfo; -// Registry entry: one per registered runtime stub, appended under the exclusive -// _stubs_lock, never removed or reordered (CodeCache never removes blobs -// either). _info is fixed at registration time and the pointed-to object is -// never freed, so readers see a stable snapshot under the shared lock. +// Registry entry: one per registered runtime stub, inserted in start-address +// order under the exclusive _stubs_lock and never removed (CodeCache never +// removes blobs either). _info is fixed at registration time and the +// pointed-to object is never freed, so readers see a stable snapshot under the +// shared lock. The array holding the entries is replaced when it grows. struct StubUnwindEntry { const void* _start; const StubUnwindInfo* _info; @@ -32,7 +34,13 @@ struct StubUnwindEntry { // The class tracks JIT-compiled code and ranges class JitCodeCache { private: + friend class JitCodeCacheTestAccessor; + + // Readers (signal handlers) take it shared with a bounded spin; writers + // take it exclusively only to publish what they prepared outside it. static SpinLock _stubs_lock; + // Orders DynamicCodeGenerated() writers; never taken in a signal handler. + static Mutex _stubs_write_lock; static CodeCache _runtime_stubs; static std::atomic _call_stub_begin; static std::atomic _call_stub_end; @@ -58,24 +66,31 @@ class JitCodeCache { address < stub_end; } - static CodeBlob* findRuntimeStub(const void *address); + // Finds the runtime stub containing address and copies its start and name + // out under the shared _stubs_lock (the names are never freed). Returns + // false when address is not inside any stub, or without searching when + // the lock is held exclusively or the shared spin budget runs out (counted + // as STUB_LOOKUP_LOCK_UNAVAILABLE). Safe to call from a signal handler. + static bool findRuntimeStub(const void *address, const void **start, + const char **name); #ifdef __aarch64__ // Finds the runtime stub containing address and returns a consistent view // of its start/name plus the precomputed StubUnwindInfo (NULL when the stub // was not classified -- including excluded names such as call_stub -- or // when address is not inside any stub; returns false in that case). - // Takes _stubs_lock shared exactly like findRuntimeStub(), which the - // calling signal-handler paths already do, so no new lock acquisitions are - // introduced. Copies everything out before releasing the lock; the info - // objects themselves are never freed (same lifetime as the stub names). + // Takes _stubs_lock shared with the same bounded spin as + // findRuntimeStub() and also returns false when the lock is unavailable. + // Copies everything out before releasing the lock; the info objects + // themselves are never freed (same lifetime as the stub names). static bool findRuntimeStubInfo(const void* address, const void** start, const char** name, const StubUnwindInfo** info); #endif // Heap usage of the runtime-stubs code cache, via CodeCache::memoryUsage(). // Read under the shared stubs lock because DynamicCodeGenerated() mutates - // _runtime_stubs (add()/expand()) concurrently. + // _runtime_stubs (commitAdd()) concurrently. Not for signal handlers: it + // waits without bound. static long long runtimeStubsMemoryUsage(); static bool isJitCode(const void* pc) { return CodeHeap::contains(pc); diff --git a/ddprof-lib/src/test/cpp/codeCache_ut.cpp b/ddprof-lib/src/test/cpp/codeCache_ut.cpp index 1183473316..5f3184776b 100644 --- a/ddprof-lib/src/test/cpp/codeCache_ut.cpp +++ b/ddprof-lib/src/test/cpp/codeCache_ut.cpp @@ -6,6 +6,8 @@ #include #include "codeCache.h" +#include + // memoryUsage() must reflect the actual per-symbol name length, not a fixed // per-symbol constant: adding one symbol grows the reported usage by exactly // the allocation size of its name string. @@ -99,3 +101,42 @@ TEST(CodeCacheTest, ArrayMemoryUsageSumsLibraries) { delete a; delete b; } + +// The two-phase add used under JitCodeCache's stubs lock: prepareAdd() hands +// over a grown array only when the cache is full, commitAdd() returns the +// replaced array exactly then, and the result matches add()'s documented +// accounting (blob array at the grown capacity plus each name allocation). +TEST(CodeCacheTest, TwoPhaseAddGrowsOnlyWhenFull) { + CodeCache cc("lib"); + long long names = 0; + char name[32]; + const int ADDS = INITIAL_CODE_CACHE_CAPACITY + 1; + for (int i = 0; i < ADDS; i++) { + snprintf(name, sizeof(name), "sym_%d", i); + CodeCache::PreparedAdd prepared; + cc.prepareAdd(name, prepared); + bool full = i == INITIAL_CODE_CACHE_CAPACITY; + EXPECT_EQ(full, prepared.grown != nullptr) << i; + CodeBlob *retired = cc.commitAdd(reinterpret_cast(0x1000 + i * 16), + 16, prepared); + EXPECT_EQ(full, retired != nullptr) << i; + delete[] retired; + names += (long long)NativeFunc::allocSize(name); + } + EXPECT_EQ((long long)INITIAL_CODE_CACHE_CAPACITY * 2 * sizeof(CodeBlob) + + (long long)NativeFunc::allocSize("lib") + names, + cc.memoryUsage()); + for (int i = 0; i < ADDS; i++) { + snprintf(name, sizeof(name), "sym_%d", i); + EXPECT_EQ(reinterpret_cast(0x1000 + i * 16), cc.findSymbol(name)) << i; + } +} + +// prepareAdd() keeps add()'s replacement of non-printable name characters. +TEST(CodeCacheTest, TwoPhaseAddReplacesNonPrintableName) { + CodeCache cc("lib"); + CodeCache::PreparedAdd prepared; + cc.prepareAdd("a\x01" "b", prepared); + delete[] cc.commitAdd(reinterpret_cast(0x1000), 16, prepared); + EXPECT_EQ(reinterpret_cast(0x1000), cc.findSymbol("a?b")); +} diff --git a/ddprof-lib/src/test/cpp/jitCodeCache_ut.cpp b/ddprof-lib/src/test/cpp/jitCodeCache_ut.cpp new file mode 100644 index 0000000000..f175e56e0a --- /dev/null +++ b/ddprof-lib/src/test/cpp/jitCodeCache_ut.cpp @@ -0,0 +1,227 @@ +/* + * Copyright 2026, Datadog, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +#include + +#include +#include +#include +#include +#include +#include +#include + +#include "counters.h" +#include "hotspot/jitCodeCache.h" +#ifdef __aarch64__ +#include "hotspot/stubUnwindInfo.h" +#endif + +class JitCodeCacheTestAccessor { +public: + static SpinLock& stubsLock() { return JitCodeCache::_stubs_lock; } +}; + +namespace { + +// JitCodeCache state is process-wide and stubs are never removed, so every +// test registers into its own disjoint slice of this buffer. The buffer is +// static so registered addresses stay valid; on aarch64 the registration +// decodes it, so it is filled with NOPs. Stub names avoid "Interpreter" and +// "call_stub", which change other global state. +const int CODE_WORDS = 16384; +alignas(64) uint32_t g_code[CODE_WORDS]; + +const uint32_t* codeAt(int word) { + static bool filled = [] { + for (int i = 0; i < CODE_WORDS; i++) { + g_code[i] = 0xd503201f; // aarch64 NOP + } + return true; + }(); + (void)filled; + return &g_code[word]; +} + +void registerStub(const void* start, int length, const char* name) { + JitCodeCache::DynamicCodeGenerated(nullptr, name, start, length); +} + +// Runs body on a helper thread and reports whether it finished within the +// timeout. Joins in every case, so a body that waits on a lock held by the +// caller must be released by the caller before the join (see release). +template +bool finishesWithin(std::chrono::milliseconds timeout, Body body, Release release) { + std::atomic done{false}; + std::thread t([&] { + body(); + done.store(true, std::memory_order_release); + }); + auto deadline = std::chrono::steady_clock::now() + timeout; + while (!done.load(std::memory_order_acquire) && + std::chrono::steady_clock::now() < deadline) { + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } + bool finished = done.load(std::memory_order_acquire); + release(); + t.join(); + return finished; +} + +} // namespace + +// Regression for the stub-lock deadlock: a signal-handler lookup must not wait +// for a registration that holds _stubs_lock exclusively. Before the fix the +// lookup spun in lockShared() until the lock was released. +TEST(JitCodeCacheTest, FindRuntimeStubDoesNotWaitForExclusiveHolder) { + const void* code = codeAt(0); + registerStub(code, 64, "jcc_test_busy"); + + const void* start = nullptr; + const char* name = nullptr; + ASSERT_TRUE(JitCodeCache::findRuntimeStub(code, &start, &name)); + EXPECT_EQ(code, start); + EXPECT_STREQ("jcc_test_busy", name); + + SpinLock& lock = JitCodeCacheTestAccessor::stubsLock(); + long long before = Counters::getCounter(STUB_LOOKUP_LOCK_UNAVAILABLE); + bool found = true; + lock.lock(); + bool finished = finishesWithin( + std::chrono::seconds(5), + [&] { found = JitCodeCache::findRuntimeStub(code, &start, &name); }, + [&] { lock.unlock(); }); + ASSERT_TRUE(finished) << "findRuntimeStub waited for the exclusive holder"; + EXPECT_FALSE(found); + EXPECT_EQ(1, Counters::getCounter(STUB_LOOKUP_LOCK_UNAVAILABLE) - before); + + EXPECT_TRUE(JitCodeCache::findRuntimeStub(code, &start, &name)); +} + +#ifdef __aarch64__ +TEST(JitCodeCacheTest, FindRuntimeStubInfoDoesNotWaitForExclusiveHolder) { + const void* code = codeAt(64); + registerStub(code, 64, "jcc_test_busy_info"); + + const void* start = nullptr; + const char* name = nullptr; + const StubUnwindInfo* info = nullptr; + ASSERT_TRUE(JitCodeCache::findRuntimeStubInfo(code, &start, &name, &info)); + EXPECT_EQ(code, start); + EXPECT_STREQ("jcc_test_busy_info", name); + + SpinLock& lock = JitCodeCacheTestAccessor::stubsLock(); + long long before = Counters::getCounter(STUB_LOOKUP_LOCK_UNAVAILABLE); + bool found = true; + lock.lock(); + bool finished = finishesWithin( + std::chrono::seconds(5), + [&] { found = JitCodeCache::findRuntimeStubInfo(code, &start, &name, &info); }, + [&] { lock.unlock(); }); + ASSERT_TRUE(finished) << "findRuntimeStubInfo waited for the exclusive holder"; + EXPECT_FALSE(found); + EXPECT_EQ(nullptr, info); + EXPECT_EQ(1, Counters::getCounter(STUB_LOOKUP_LOCK_UNAVAILABLE) - before); + + EXPECT_TRUE(JitCodeCache::findRuntimeStubInfo(code, &start, &name, &info)); +} + +// A second registration of the same start keeps the first unwind info. +TEST(JitCodeCacheTest, DuplicateRegistrationKeepsFirstInfo) { + const void* code = codeAt(128); + registerStub(code, 64, "jcc_test_dup"); + const void* start = nullptr; + const char* name = nullptr; + const StubUnwindInfo* first = nullptr; + ASSERT_TRUE(JitCodeCache::findRuntimeStubInfo(code, &start, &name, &first)); + ASSERT_NE(nullptr, first); + + registerStub(code, 64, "jcc_test_dup_again"); + const StubUnwindInfo* second = nullptr; + ASSERT_TRUE(JitCodeCache::findRuntimeStubInfo(code, &start, &name, &second)); + EXPECT_EQ(first, second); +} +#endif + +// More registrations than the initial blob capacity (and, on aarch64, the +// initial unwind-registry capacity) force both arrays to be replaced; every +// stub must stay findable with its own start, name and unwind info. +TEST(JitCodeCacheTest, RegistrationsAcrossGrowthStayFindable) { + const int STUBS = INITIAL_CODE_CACHE_CAPACITY + 100; + const int WORDS_PER_STUB = 2; + const int BASE = 256; + ASSERT_LE(BASE + STUBS * WORDS_PER_STUB, CODE_WORDS); + char name[32]; + for (int i = 0; i < STUBS; i++) { + snprintf(name, sizeof(name), "jcc_grow_%d", i); + registerStub(codeAt(BASE + i * WORDS_PER_STUB), WORDS_PER_STUB * 4, name); + } + for (int i = 0; i < STUBS; i++) { + const void* code = codeAt(BASE + i * WORDS_PER_STUB); + snprintf(name, sizeof(name), "jcc_grow_%d", i); + const void* start = nullptr; + const char* found_name = nullptr; + ASSERT_TRUE(JitCodeCache::findRuntimeStub(code, &start, &found_name)) << i; + EXPECT_EQ(code, start) << i; + EXPECT_STREQ(name, found_name) << i; +#ifdef __aarch64__ + const StubUnwindInfo* info = nullptr; + ASSERT_TRUE(JitCodeCache::findRuntimeStubInfo(code, &start, &found_name, &info)) << i; + EXPECT_NE(nullptr, info) << i; +#endif + } +} + +// Lookups racing registrations that replace the blob array. Readers copy the +// stub fields out under the lock, so none can read a replaced array; under +// ASan a read of a freed array fails the test. Readers pause between lookups: +// SpinLock::lock() only succeeds when no reader holds the lock, so readers +// that only yield can delay the writers for seconds under ASan. +TEST(JitCodeCacheTest, ConcurrentRegistrationAndLookup) { + const int WRITERS = 2; + const int STUBS_PER_WRITER = 600; + const int WORDS_PER_STUB = 2; + const int BASE = 4096; + ASSERT_LE(BASE + WRITERS * STUBS_PER_WRITER * WORDS_PER_STUB, CODE_WORDS); + + std::atomic writers_left{WRITERS}; + std::vector threads; + for (int w = 0; w < WRITERS; w++) { + threads.emplace_back([&, w] { + char name[32]; + for (int i = 0; i < STUBS_PER_WRITER; i++) { + int slot = w * STUBS_PER_WRITER + i; + snprintf(name, sizeof(name), "jcc_conc_%d", slot); + registerStub(codeAt(BASE + slot * WORDS_PER_STUB), WORDS_PER_STUB * 4, name); + } + writers_left.fetch_sub(1); + }); + } + for (int r = 0; r < 4; r++) { + threads.emplace_back([&, r] { + int slot = r; + while (writers_left.load() > 0) { + const void* code = codeAt(BASE + slot * WORDS_PER_STUB); + const void* start = nullptr; + const char* name = nullptr; + if (JitCodeCache::findRuntimeStub(code, &start, &name)) { + EXPECT_EQ(code, start); + EXPECT_EQ(0, strncmp(name, "jcc_conc_", 9)); + } + slot = (slot + 7) % (WRITERS * STUBS_PER_WRITER); + std::this_thread::sleep_for(std::chrono::microseconds(50)); + } + }); + } + for (auto& t : threads) { + t.join(); + } + for (int slot = 0; slot < WRITERS * STUBS_PER_WRITER; slot++) { + const void* start = nullptr; + const char* name = nullptr; + EXPECT_TRUE(JitCodeCache::findRuntimeStub(codeAt(BASE + slot * WORDS_PER_STUB), + &start, &name)) << slot; + } +} From e178820e356f77d8b2eb788607e3fe62f535d327 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Sat, 24 Oct 2026 10:17:42 +0200 Subject: [PATCH 2/5] Address review: unbudgeted stub lookup, narrower writer lock Co-Authored-By: Claude Opus 5.5 --- ddprof-lib/src/main/cpp/counters.h | 6 ++-- .../src/main/cpp/hotspot/jitCodeCache.cpp | 36 ++++++++++--------- .../src/main/cpp/hotspot/jitCodeCache.h | 18 ++++++---- ddprof-lib/src/test/cpp/jitCodeCache_ut.cpp | 6 ++-- 4 files changed, 37 insertions(+), 29 deletions(-) diff --git a/ddprof-lib/src/main/cpp/counters.h b/ddprof-lib/src/main/cpp/counters.h index d6113f4f82..ad58859eb3 100644 --- a/ddprof-lib/src/main/cpp/counters.h +++ b/ddprof-lib/src/main/cpp/counters.h @@ -137,9 +137,9 @@ X(WALKVM_STUB_INFO_FALLBACK, "walkvm_stub_info_fallback") \ X(WALKVM_STUB_INFO_REGISTRATION_FAILED, "walkvm_stub_info_registration_failed") \ /* Runtime-stub lookups (JitCodeCache::findRuntimeStub/findRuntimeStubInfo) \ - * that returned "not found" without searching because the shared stubs \ - * lock was held exclusively by a registration or the shared spin budget \ - * ran out; the stack walk continued with its stub-not-found fallback. */ \ + * that returned "not found" without searching because a registration held \ + * the stubs lock exclusively; the stack walk continued with its \ + * stub-not-found fallback. */ \ X(STUB_LOOKUP_LOCK_UNAVAILABLE, "stub_lookup_lock_unavailable") \ X(WALKVM_FP_CHAIN_ATTEMPT, "walkvm_fp_chain_attempt") \ X(WALKVM_FP_CHAIN_REACHED_CODEHEAP, "walkvm_fp_chain_reached_codeheap") \ diff --git a/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.cpp b/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.cpp index 6c1e7e8c71..6b8752fee0 100644 --- a/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.cpp @@ -63,14 +63,11 @@ void JNICALL JitCodeCache::CompiledMethodLoad(jvmtiEnv *jvmti, jmethodID method, void JNICALL JitCodeCache::DynamicCodeGenerated(jvmtiEnv *jvmti, const char *name, const void *address, jint length) { ProfiledThread::initCurrentThreadSignalSafe(); - // Registrations can arrive concurrently from several threads. The writer - // mutex orders them, so the state read while preparing below stays valid - // until the commit. Signal handlers never take it. - MutexLocker writer(_stubs_write_lock); #ifdef __aarch64__ - // Analyze before taking the lock: the classifier decodes every instruction - // and allocates. Publication is ordered by the exclusive lock below -- a - // reader that finds the stub always finds its info. + // Analyze before taking either lock: the classifier decodes every + // instruction and allocates, but reads only the stub code. Publication is + // ordered by the exclusive lock below -- a reader that finds the stub always + // finds its info. StubUnwindInfo* stub_info = nullptr; if (shouldAnalyzeStub(name)) { // Only the adapters blob is known to pack several entry points behind @@ -82,10 +79,16 @@ void JNICALL JitCodeCache::DynamicCodeGenerated(jvmtiEnv *jvmti, const char *nam : WALKVM_STUB_INFO_UNCLASSIFIED); } #endif - // Every heap allocation and free happens outside the exclusive lock. A - // thread that holds the allocator lock can be interrupted by a profiling - // signal whose handler looks up a stub; if this thread then blocked in the - // allocator while holding _stubs_lock, that lookup could never succeed. + // Registrations can arrive concurrently from several threads. The writer + // mutex orders them, so the state read while preparing below stays valid + // until the commit. Signal handlers never take it. + MutexLocker writer(_stubs_write_lock); + // Every heap allocation and free happens outside the exclusive lock. The + // bounded readers already prevent a deadlock: while the lock is held + // exclusively, a stub lookup returns false and the stack walk uses its + // fallback unwind. Keeping the allocator out of the exclusive section keeps + // that window short: the registering thread cannot block in the allocator + // while it holds _stubs_lock. CodeCache::PreparedAdd prepared; _runtime_stubs.prepareAdd(name, prepared); #ifdef __aarch64__ @@ -170,9 +173,10 @@ void JNICALL JitCodeCache::DynamicCodeGenerated(jvmtiEnv *jvmti, const char *nam bool JitCodeCache::findRuntimeStub(const void *address, const void **start, const char **name) { - // Bounded: the caller is a signal handler, and the exclusive holder may be - // the interrupted thread itself. - if (!_stubs_lock.tryLockShared(SpinLock::DEFAULT_SHARED_SPIN_BUDGET)) { + // Fails only while a writer holds the lock: the caller is a signal handler, + // and the writer may be the interrupted thread itself. Competing readers do + // not make it fail, because each failed CAS means another reader got in. + if (!_stubs_lock.tryLockShared()) { Counters::increment(STUB_LOOKUP_LOCK_UNAVAILABLE); return false; } @@ -195,8 +199,8 @@ bool JitCodeCache::findRuntimeStub(const void *address, const void **start, bool JitCodeCache::findRuntimeStubInfo(const void* address, const void** start, const char** name, const StubUnwindInfo** info) { *info = nullptr; - // Bounded for the same reason as findRuntimeStub(). - if (!_stubs_lock.tryLockShared(SpinLock::DEFAULT_SHARED_SPIN_BUDGET)) { + // Same lock acquisition as findRuntimeStub(). + if (!_stubs_lock.tryLockShared()) { Counters::increment(STUB_LOOKUP_LOCK_UNAVAILABLE); return false; } diff --git a/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.h b/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.h index 28961f123b..fb9dd2ea9b 100644 --- a/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.h +++ b/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.h @@ -36,8 +36,9 @@ class JitCodeCache { private: friend class JitCodeCacheTestAccessor; - // Readers (signal handlers) take it shared with a bounded spin; writers - // take it exclusively only to publish what they prepared outside it. + // Readers (signal handlers) take it shared and give up while a writer + // holds it; writers take it exclusively only to publish what they + // prepared outside it. static SpinLock _stubs_lock; // Orders DynamicCodeGenerated() writers; never taken in a signal handler. static Mutex _stubs_write_lock; @@ -68,9 +69,12 @@ class JitCodeCache { // Finds the runtime stub containing address and copies its start and name // out under the shared _stubs_lock (the names are never freed). Returns - // false when address is not inside any stub, or without searching when - // the lock is held exclusively or the shared spin budget runs out (counted - // as STUB_LOOKUP_LOCK_UNAVAILABLE). Safe to call from a signal handler. + // false when address is not inside any stub, or without searching while a + // registration holds the lock exclusively (counted as + // STUB_LOOKUP_LOCK_UNAVAILABLE). Safe to call from a signal handler. On a + // false result the stack walk unwinds the frame without the stub start + // and, on aarch64, with the legacy heuristics instead of the precomputed + // StubUnwindInfo. static bool findRuntimeStub(const void *address, const void **start, const char **name); @@ -79,8 +83,8 @@ class JitCodeCache { // of its start/name plus the precomputed StubUnwindInfo (NULL when the stub // was not classified -- including excluded names such as call_stub -- or // when address is not inside any stub; returns false in that case). - // Takes _stubs_lock shared with the same bounded spin as - // findRuntimeStub() and also returns false when the lock is unavailable. + // Takes _stubs_lock shared like findRuntimeStub() and also returns false + // while a registration holds the lock exclusively. // Copies everything out before releasing the lock; the info objects // themselves are never freed (same lifetime as the stub names). static bool findRuntimeStubInfo(const void* address, const void** start, diff --git a/ddprof-lib/src/test/cpp/jitCodeCache_ut.cpp b/ddprof-lib/src/test/cpp/jitCodeCache_ut.cpp index f175e56e0a..079cc51031 100644 --- a/ddprof-lib/src/test/cpp/jitCodeCache_ut.cpp +++ b/ddprof-lib/src/test/cpp/jitCodeCache_ut.cpp @@ -72,9 +72,9 @@ bool finishesWithin(std::chrono::milliseconds timeout, Body body, Release releas } // namespace -// Regression for the stub-lock deadlock: a signal-handler lookup must not wait -// for a registration that holds _stubs_lock exclusively. Before the fix the -// lookup spun in lockShared() until the lock was released. +// A signal-handler lookup must not wait for a registration that holds +// _stubs_lock exclusively: the lookup returns "not found" at once, counts the +// miss, and finds the stub again after the unlock. TEST(JitCodeCacheTest, FindRuntimeStubDoesNotWaitForExclusiveHolder) { const void* code = codeAt(0); registerStub(code, 64, "jcc_test_busy"); From 9bf9d7a0aca961ef55bdc0c3a860fde47d9a8ff5 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Sat, 24 Oct 2026 10:34:30 +0200 Subject: [PATCH 3/5] Make PreparedAdd own its allocations Co-Authored-By: Claude Opus 5.5 --- ddprof-lib/src/main/cpp/codeCache.cpp | 8 ++++- ddprof-lib/src/main/cpp/codeCache.h | 37 ++++++++++++++++-------- ddprof-lib/src/test/cpp/codeCache_ut.cpp | 33 +++++++++++++++++++++ 3 files changed, 65 insertions(+), 13 deletions(-) diff --git a/ddprof-lib/src/main/cpp/codeCache.cpp b/ddprof-lib/src/main/cpp/codeCache.cpp index 5af326a464..4845f053f7 100644 --- a/ddprof-lib/src/main/cpp/codeCache.cpp +++ b/ddprof-lib/src/main/cpp/codeCache.cpp @@ -198,6 +198,8 @@ void CodeCache::prepareAdd(const char *name, PreparedAdd &prepared) const { // (e.g. _runtime_stubs) are exempt — they are never published. assert(!_published.load(std::memory_order_acquire) && "add() on a published CodeCache races memoryUsage()"); + // A reused PreparedAdd would leak the allocations it still owns. + assert(prepared.name == nullptr && prepared.grown == nullptr); char *name_copy = NativeFunc::create(name, _lib_index); prepared.name = name_copy; prepared.name_size = (long long)NativeFunc::allocSize(name); @@ -219,7 +221,7 @@ void CodeCache::prepareAdd(const char *name, PreparedAdd &prepared) const { } CodeBlob *CodeCache::commitAdd(const void *start, int length, - const PreparedAdd &prepared, + PreparedAdd &prepared, bool update_bounds) { assert(!_published.load(std::memory_order_acquire) && "add() on a published CodeCache races memoryUsage()"); @@ -243,6 +245,10 @@ CodeBlob *CodeCache::commitAdd(const void *start, int length, _blobs[_count]._end = end; _blobs[_count]._name = prepared.name; _count++; + // The cache owns both allocations now; the PreparedAdd destructor must not + // free them. + prepared.name = nullptr; + prepared.grown = nullptr; if (update_bounds) { updateBounds(start, end); diff --git a/ddprof-lib/src/main/cpp/codeCache.h b/ddprof-lib/src/main/cpp/codeCache.h index 8f44cdce60..6ced816af8 100644 --- a/ddprof-lib/src/main/cpp/codeCache.h +++ b/ddprof-lib/src/main/cpp/codeCache.h @@ -289,14 +289,27 @@ class CodeCache { void add(const void *start, int length, const char *name, bool update_bounds = false); - // Heap allocations that prepareAdd() makes for exactly one later commitAdd(). + // Heap allocations that prepareAdd() makes for one later commitAdd(). + // commitAdd() takes ownership of name and grown and sets them to nullptr. + // If no commitAdd() happens, the destructor frees them. struct PreparedAdd { - char *name; // owned NativeFunc name copy, non-printables replaced - long long name_size; // NativeFunc::allocSize() of the name - long long name_overhead; // NativeFunc::nameOverhead() of the copy - CodeBlob *grown; // filled copy of _blobs at twice the capacity, or - // nullptr when the current array has a free slot - int capacity; // _capacity that this preparation is valid for + char *name = nullptr; // NativeFunc name copy, non-printables replaced + long long name_size = 0; // NativeFunc::allocSize() of the name + long long name_overhead = 0; // NativeFunc::nameOverhead() of the copy + CodeBlob *grown = nullptr; // filled copy of _blobs at twice the capacity, or + // nullptr when the current array has a free slot + int capacity = 0; // _capacity that this preparation is valid for + + PreparedAdd() = default; + ~PreparedAdd() { + if (name != nullptr) { + NativeFunc::destroy(name); + } + delete[] grown; + } + // Non-copyable: a copy would free the same allocations twice. + PreparedAdd(const PreparedAdd &) = delete; + PreparedAdd &operator=(const PreparedAdd &) = delete; }; // Two-phase add() for a cache whose readers run in signal handlers under a @@ -304,12 +317,12 @@ class CodeCache { // allocation and can run outside the lock; commitAdd() only stores and // copies pointers, so it is safe under the exclusive lock. commitAdd() // returns the replaced blob array (or nullptr), which the caller must - // delete[] after it releases the lock. Every prepareAdd() must be followed by - // exactly one commitAdd(), with no other mutation of this cache in between; - // the caller serializes the pair against other writers. + // delete[] after it releases the lock. Between prepareAdd() and commitAdd() + // nothing else may mutate this cache; the caller serializes the pair + // against other writers. Pass a freshly constructed PreparedAdd. void prepareAdd(const char *name, PreparedAdd &prepared) const; - CodeBlob *commitAdd(const void *start, int length, - const PreparedAdd &prepared, bool update_bounds = false); + CodeBlob *commitAdd(const void *start, int length, PreparedAdd &prepared, + bool update_bounds = false); void updateBounds(const void *start, const void *end); void sort(); diff --git a/ddprof-lib/src/test/cpp/codeCache_ut.cpp b/ddprof-lib/src/test/cpp/codeCache_ut.cpp index 5f3184776b..dd6806be53 100644 --- a/ddprof-lib/src/test/cpp/codeCache_ut.cpp +++ b/ddprof-lib/src/test/cpp/codeCache_ut.cpp @@ -140,3 +140,36 @@ TEST(CodeCacheTest, TwoPhaseAddReplacesNonPrintableName) { delete[] cc.commitAdd(reinterpret_cast(0x1000), 16, prepared); EXPECT_EQ(reinterpret_cast(0x1000), cc.findSymbol("a?b")); } + +// A PreparedAdd that is never committed frees its allocations and leaves the +// cache unchanged, including when it holds a grown blob array. LeakSanitizer +// (ASan builds) reports the allocations if the destructor does not free them. +TEST(CodeCacheTest, UncommittedPrepareFreesAndLeavesCacheUnchanged) { + CodeCache cc("lib"); + char name[32]; + for (int i = 0; i < INITIAL_CODE_CACHE_CAPACITY; i++) { + snprintf(name, sizeof(name), "sym_%d", i); + cc.add(reinterpret_cast(0x1000 + i * 16), 16, name); + } + long long usage = cc.memoryUsage(); + { + CodeCache::PreparedAdd prepared; + cc.prepareAdd("never_committed", prepared); + EXPECT_NE(nullptr, prepared.name); + EXPECT_NE(nullptr, prepared.grown); + } + EXPECT_EQ(usage, cc.memoryUsage()); + EXPECT_EQ(nullptr, cc.findSymbol("never_committed")); +} + +// commitAdd() takes ownership: the PreparedAdd no longer refers to the name or +// the grown array, so its destructor cannot free storage the cache uses. +TEST(CodeCacheTest, CommitAddTakesOwnership) { + CodeCache cc("lib"); + CodeCache::PreparedAdd prepared; + cc.prepareAdd("owned", prepared); + delete[] cc.commitAdd(reinterpret_cast(0x1000), 16, prepared); + EXPECT_EQ(nullptr, prepared.name); + EXPECT_EQ(nullptr, prepared.grown); + EXPECT_EQ(reinterpret_cast(0x1000), cc.findSymbol("owned")); +} From 23849b963349a57873b984e3c82535ec6f787a95 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Sat, 24 Oct 2026 10:55:34 +0200 Subject: [PATCH 4/5] Use RAII lock guards for stubs lock; add RAII rule Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 8 +++ .../src/main/cpp/hotspot/jitCodeCache.cpp | 71 ++++++++++--------- ddprof-lib/src/main/cpp/spinLock.h | 15 +++- ddprof-lib/src/test/cpp/spinLock_ut.cpp | 49 +++++++++++++ 4 files changed, 105 insertions(+), 38 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 03931677da..a574e785fa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -343,6 +343,14 @@ exception list. helpers unless reused or genuinely hard to follow; "why" comments must reference concrete mechanisms (e.g. "ASAN's allocator lock is reentrant"), never speculative claims about HotSpot/glibc/ASAN internals — if unsure, describe the observable symptom. +- **RAII for every acquire/release pair** (C/C++). A lock, a heap allocation, or a flag + that must be restored is released by a guard or owner destructor, not by a manual call + at each exit. Reuse `SharedLockGuard`, `ExclusiveLockGuard`, `OptionalSharedLockGuard`, + `MutexLocker`, `CriticalSection` before you add a type. A new owner type is non-copyable + and hands over ownership explicitly (the receiver sets the owner's pointer to `nullptr`). + A manual pair needs a comment that says why a guard scope cannot express it. In code that + shares a lock with signal handlers, end an owner that frees memory outside the exclusive + critical section, because its destructor frees at scope end. - Always challenge proposals with deep analysis when you have hard evidence. ## Work Tracking diff --git a/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.cpp b/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.cpp index 6b8752fee0..c314e5cc3c 100644 --- a/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.cpp @@ -133,24 +133,27 @@ void JNICALL JitCodeCache::DynamicCodeGenerated(jvmtiEnv *jvmti, const char *nam StubUnwindEntry* retired_unwind = nullptr; #endif - _stubs_lock.lock(); - CodeBlob* retired_blobs = _runtime_stubs.commitAdd(address, length, prepared, true); + CodeBlob* retired_blobs; + { + // The scope ends before the frees below: nothing in it allocates or frees. + ExclusiveLockGuard publish(&_stubs_lock); + retired_blobs = _runtime_stubs.commitAdd(address, length, prepared, true); #ifdef __aarch64__ - if (grown_unwind != nullptr) { - retired_unwind = _stub_unwind; - _stub_unwind = grown_unwind; - _stub_unwind_capacity = grown_capacity; - } - if (insert) { - // Sorted insert: shift the tail right to keep the lookup binary-searchable. - memmove(&_stub_unwind[lo + 1], &_stub_unwind[lo], - (_stub_unwind_count - lo) * sizeof(StubUnwindEntry)); - _stub_unwind[lo]._start = address; - _stub_unwind[lo]._info = stub_info; - _stub_unwind_count++; - } + if (grown_unwind != nullptr) { + retired_unwind = _stub_unwind; + _stub_unwind = grown_unwind; + _stub_unwind_capacity = grown_capacity; + } + if (insert) { + // Sorted insert: shift the tail right to keep the lookup binary-searchable. + memmove(&_stub_unwind[lo + 1], &_stub_unwind[lo], + (_stub_unwind_count - lo) * sizeof(StubUnwindEntry)); + _stub_unwind[lo]._start = address; + _stub_unwind[lo]._info = stub_info; + _stub_unwind_count++; + } #endif - _stubs_lock.unlock(); + } // Readers take the shared lock for every access, so after the unlock no // reader can still use the replaced arrays. @@ -176,23 +179,23 @@ bool JitCodeCache::findRuntimeStub(const void *address, const void **start, // Fails only while a writer holds the lock: the caller is a signal handler, // and the writer may be the interrupted thread itself. Competing readers do // not make it fail, because each failed CAS means another reader got in. - if (!_stubs_lock.tryLockShared()) { + OptionalSharedLockGuard guard(&_stubs_lock, SpinLock::NO_SPIN_BUDGET); + if (!guard.ownsLock()) { Counters::increment(STUB_LOOKUP_LOCK_UNAVAILABLE); return false; } - bool found = false; - if (_runtime_stubs.contains(address)) { - CodeBlob *blob = _runtime_stubs.findBlobByAddress(address); - if (blob != nullptr) { - // Copy out under the lock: a registration may replace the blob array - // as soon as the lock is released. - *start = blob->_start; - *name = blob->_name; - found = true; - } + if (!_runtime_stubs.contains(address)) { + return false; } - _stubs_lock.unlockShared(); - return found; + CodeBlob *blob = _runtime_stubs.findBlobByAddress(address); + if (blob == nullptr) { + return false; + } + // Copy out under the lock: a registration may replace the blob array as + // soon as the lock is released. + *start = blob->_start; + *name = blob->_name; + return true; } #ifdef __aarch64__ @@ -200,7 +203,8 @@ bool JitCodeCache::findRuntimeStubInfo(const void* address, const void** start, const char** name, const StubUnwindInfo** info) { *info = nullptr; // Same lock acquisition as findRuntimeStub(). - if (!_stubs_lock.tryLockShared()) { + OptionalSharedLockGuard guard(&_stubs_lock, SpinLock::NO_SPIN_BUDGET); + if (!guard.ownsLock()) { Counters::increment(STUB_LOOKUP_LOCK_UNAVAILABLE); return false; } @@ -227,14 +231,11 @@ bool JitCodeCache::findRuntimeStubInfo(const void* address, const void** start, } } } - _stubs_lock.unlockShared(); return found; } #endif long long JitCodeCache::runtimeStubsMemoryUsage() { - _stubs_lock.lockShared(); - long long usage = _runtime_stubs.memoryUsage(); - _stubs_lock.unlockShared(); - return usage; + SharedLockGuard guard(&_stubs_lock); + return _runtime_stubs.memoryUsage(); } diff --git a/ddprof-lib/src/main/cpp/spinLock.h b/ddprof-lib/src/main/cpp/spinLock.h index d35e40e45b..a62117431c 100644 --- a/ddprof-lib/src/main/cpp/spinLock.h +++ b/ddprof-lib/src/main/cpp/spinLock.h @@ -1,5 +1,6 @@ /* * Copyright 2017 Andrei Pangin + * Copyright 2026, Datadog, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -57,6 +58,10 @@ class alignas(DEFAULT_CACHE_LINE_SIZE) SpinLock { // Bounds the number of CAS-retry iterations under reader contention; // does NOT bound wall-clock latency (CAS stall time is hardware-dependent). static constexpr int DEFAULT_SHARED_SPIN_BUDGET = 256; + // Passed as max_spins to OptionalSharedLockGuard: fail only while an + // exclusive lock is held, and keep retrying under reader contention + // (tryLockShared() without a budget). + static constexpr int NO_SPIN_BUDGET = -1; bool tryLockShared() { // Spins while no exclusive lock is held and the CAS to acquire a shared @@ -123,15 +128,19 @@ class SharedLockGuard { // Acquires a shared lock with a bounded CAS-retry budget. Returns without // acquiring (ownsLock() == false) when an exclusive lock is observed or -// the spin budget is exhausted under reader contention. Safe to use in -// signal-handler paths. +// the spin budget is exhausted under reader contention. With +// SpinLock::NO_SPIN_BUDGET it fails only when an exclusive lock is observed. +// Safe to use in signal-handler paths. class OptionalSharedLockGuard { SpinLock* _lock; public: explicit OptionalSharedLockGuard( SpinLock* lock, int max_spins = SpinLock::DEFAULT_SHARED_SPIN_BUDGET) : _lock(lock) { - if (!_lock->tryLockShared(max_spins)) { + bool acquired = max_spins == SpinLock::NO_SPIN_BUDGET + ? _lock->tryLockShared() + : _lock->tryLockShared(max_spins); + if (!acquired) { _lock = nullptr; } } diff --git a/ddprof-lib/src/test/cpp/spinLock_ut.cpp b/ddprof-lib/src/test/cpp/spinLock_ut.cpp index 5b7ebceeb7..d9cb6a436c 100644 --- a/ddprof-lib/src/test/cpp/spinLock_ut.cpp +++ b/ddprof-lib/src/test/cpp/spinLock_ut.cpp @@ -19,6 +19,7 @@ #include "../../main/cpp/gtest_crash_handler.h" #include #include +#include static constexpr char SPINLOCK_TEST_NAME[] = "SpinLockTest"; @@ -114,3 +115,51 @@ TEST_F(SpinLockTest, TryLockShared_Free_ReturnsTrue) { EXPECT_TRUE(lock.tryLockShared()); lock.unlockShared(); } + +// NO_SPIN_BUDGET: an exclusive holder still makes the guard fail at once. +TEST_F(SpinLockTest, OptionalGuard_NoSpinBudget_ExclusiveHeld_NotOwned) { + lock.lock(); + { + OptionalSharedLockGuard g(&lock, SpinLock::NO_SPIN_BUDGET); + EXPECT_FALSE(g.ownsLock()); + } + lock.unlock(); +} + +// NO_SPIN_BUDGET: on a free lock the guard acquires and releases on destruction. +TEST_F(SpinLockTest, OptionalGuard_NoSpinBudget_Free_OwnedAndReleased) { + { + OptionalSharedLockGuard g(&lock, SpinLock::NO_SPIN_BUDGET); + EXPECT_TRUE(g.ownsLock()); + EXPECT_FALSE(lock.tryLock()); + } + EXPECT_TRUE(lock.tryLock()); + lock.unlock(); +} + +// NO_SPIN_BUDGET: reader contention alone never makes the guard fail. Readers +// on other threads race the CAS continuously; every acquisition must succeed. +TEST_F(SpinLockTest, OptionalGuard_NoSpinBudget_ReaderContention_AlwaysOwned) { + std::atomic stop{false}; + std::vector contenders; + for (int t = 0; t < 4; t++) { + contenders.emplace_back([&] { + while (!stop.load(std::memory_order_relaxed)) { + lock.lockShared(); + lock.unlockShared(); + } + }); + } + int failures = 0; + for (int i = 0; i < 100000; ++i) { + OptionalSharedLockGuard g(&lock, SpinLock::NO_SPIN_BUDGET); + if (!g.ownsLock()) { + failures++; + } + } + stop.store(true, std::memory_order_relaxed); + for (auto& t : contenders) { + t.join(); + } + EXPECT_EQ(0, failures); +} From b85dbe82ae839d76b4aaaaccd098015a16549e15 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Sat, 24 Oct 2026 11:04:31 +0200 Subject: [PATCH 5/5] Remove fixed stub-lock entry from signal-reachability allowlist Co-Authored-By: Claude Opus 5.5 --- utils/check-signal-reachability.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/utils/check-signal-reachability.py b/utils/check-signal-reachability.py index 580d7d1f4a..855f2c0a6c 100755 --- a/utils/check-signal-reachability.py +++ b/utils/check-signal-reachability.py @@ -130,10 +130,7 @@ # line in the same shape: # [|...] -> # [note] # --------------------------------------------------------------------------- -ALLOWLIST = [ - (("JitCodeCache::findRuntimeStub", "JitCodeCache::findRuntimeStubInfo"), - "SpinLock::lockShared", "PROF-16313"), -] +ALLOWLIST = [] SELF_TEST_FIXTURE = os.path.join(os.path.dirname(os.path.abspath(__file__)), "tests", "signal-reachability-fixture.cpp")