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/codeCache.cpp b/ddprof-lib/src/main/cpp/codeCache.cpp index 943e7481ff..4845f053f7 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,55 +191,76 @@ 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 // (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); - _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, + 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++; + // 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); } + 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..6ced816af8 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,47 @@ 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 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 = 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 + // 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. 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, PreparedAdd &prepared, + bool update_bounds = false); void updateBounds(const void *start, const void *end); void sort(); @@ -407,7 +442,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..ad58859eb3 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 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") \ 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..c314e5cc3c 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; @@ -61,10 +64,10 @@ void JNICALL JitCodeCache::DynamicCodeGenerated(jvmtiEnv *jvmti, const char *nam const void *address, jint length) { ProfiledThread::initCurrentThreadSignalSafe(); #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 - // 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 @@ -76,14 +79,23 @@ void JNICALL JitCodeCache::DynamicCodeGenerated(jvmtiEnv *jvmti, const char *nam : WALKVM_STUB_INFO_UNCLASSIFIED); } #endif - _stubs_lock.lock(); - _runtime_stubs.add(address, length, name, true); + // 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__ // 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,38 +105,62 @@ 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; - } - } - if (!duplicate && _stub_unwind_count < _stub_unwind_capacity) { - // 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); + 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 (!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 + + 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++; + } +#endif + } + + // 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 - _stubs_lock.unlock(); if (name[0] == 'I' && strcmp(name, "Interpreter") == 0) { CodeHeap::setInterpreterStart(address); @@ -138,22 +174,41 @@ 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(); - if (_runtime_stubs.contains(address)) { - stub = _runtime_stubs.findBlobByAddress(address); +bool JitCodeCache::findRuntimeStub(const void *address, const void **start, + const char **name) { + // 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. + OptionalSharedLockGuard guard(&_stubs_lock, SpinLock::NO_SPIN_BUDGET); + if (!guard.ownsLock()) { + Counters::increment(STUB_LOOKUP_LOCK_UNAVAILABLE); + return false; } - _stubs_lock.unlockShared(); - return stub; + if (!_runtime_stubs.contains(address)) { + return false; + } + 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__ bool JitCodeCache::findRuntimeStubInfo(const void* address, const void** start, const char** name, const StubUnwindInfo** info) { - bool found = false; *info = nullptr; - _stubs_lock.lockShared(); + // Same lock acquisition as findRuntimeStub(). + 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) { @@ -176,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/hotspot/jitCodeCache.h b/ddprof-lib/src/main/cpp/hotspot/jitCodeCache.h index c2e02b221b..fb9dd2ea9b 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,14 @@ struct StubUnwindEntry { // The class tracks JIT-compiled code and ranges class JitCodeCache { private: + friend class JitCodeCacheTestAccessor; + + // 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; static CodeCache _runtime_stubs; static std::atomic _call_stub_begin; static std::atomic _call_stub_end; @@ -58,24 +67,34 @@ 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 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); #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 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, 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/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/codeCache_ut.cpp b/ddprof-lib/src/test/cpp/codeCache_ut.cpp index 1183473316..dd6806be53 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,75 @@ 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")); +} + +// 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")); +} 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..079cc51031 --- /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 + +// 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"); + + 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; + } +} 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); +} 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")