Fix stub-lock deadlock in DynamicCodeGenerated

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

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) {

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<bool> _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

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")                   \

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);