Repository navigation
Conversation
CI Test ResultsRun: #40080521876 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 76 | Passed: 76 | Failed: 0 Updated: 2026-10-24 09:27:29 UTC |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Writer serialization protects the prepare/commit state, and lookups copy stub fields before replaced arrays can be freed. Static review identified no reportable defect; runtime tests were not executed.
🤖 Bits Code Review · Commit cc87ec9
jbachorik
added
test:asan
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/147274446 |
rkennke
left a comment
rkennke
left a comment
There was a problem hiding this comment.
Thanks, the deadlock fix looks right to me. Nothing is allocated or freed under the exclusive lock, replaced arrays are freed only after the unlock, lookups copy what they need before releasing the shared lock, and the write mutex is recursive and never taken in a signal handler.
Two comments I'd like addressed (jitCodeCache.cpp L175 and hotspotSupport.cpp L1174). They are related: with the spin budget, contention among readers alone can make a stub lookup fail, and on aarch64 a failed lookup changes how the frame is unwound, not just what it's called. Using tryLockShared() with no argument should fix both.
The comments marked Optional are suggestions. Up to you.
| 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)) { |
There was a problem hiding this comment.
Readers competing with each other can now make a lookup fail
tryLockShared(DEFAULT_SHARED_SPIN_BUDGET) gives up after 256 failed CAS attempts even when no writer holds the lock. In a wall-clock burst, many sampled threads take the shared lock at the same moment, and CAS failures between those readers alone can use up the budget. A real stub is then reported as "not found". Before this PR, lockShared() always succeeded in that case. The same applies to findRuntimeStubInfo() at L199.
Suggestion: use the overload with no argument, tryLockShared() (spinLock.h:61). It returns false only when a writer holds the lock, which is exactly the deadlock case, and it keeps retrying while only readers compete. That retry loop ends, because readers hold the lock for a short, finite time. With commitAdd() now allocation-free, the writer's critical section is also short, so the spin budget no longer protects against anything here. This also means STUB_LOOKUP_LOCK_UNAVAILABLE would count only one cause, the writer holding the lock, so the metric becomes easier to interpret.
There was a problem hiding this comment.
Agreed. Both lookups now call the overload with no argument, tryLockShared(). It fails only while a writer holds the lock exclusively, so the self-deadlock case is still covered, and competing readers no longer cause a miss. STUB_LOOKUP_LOCK_UNAVAILABLE now counts only that one cause. Fixed in 0da83b4.
| if (stub != NULL) { | ||
| stub_start = stub->_start; | ||
| stub_name = stub->_name; | ||
| if (JitCodeCache::findRuntimeStub((const void *)frame.pc(), &stub_start, &stub_name)) { |
There was a problem hiding this comment.
A failed lookup also changes how the frame is unwound
When this returns false because the lock is busy, the code falls through to the CodeHeap::findNMethod() branch. That branch finds the stub blob and calls frame.unwindStub(NULL, nmethod->name()), with no stub start and no StubUnwindInfo. On aarch64 that means the old heuristics instead of the precomputed table, and jitCodeCache.cpp describes those heuristics as able to read a stale x30 slot. The walkVM path (L771) also continues with stub_info == nullptr.
So a busy lock can produce a wrong caller frame, not just a missing or less specific frame name. Inside the short registration window that is acceptable, but the reader-contention case in the other comment makes it happen more often. Fixing that comment is enough. It would still be worth saying in the PR description and the header comment that the fallback is a degraded unwind.
There was a problem hiding this comment.
Correct, the fallback degrades the unwind and not only the frame name. The findRuntimeStub() header comment now says this: on a false result, the walk unwinds without the stub start and, on aarch64, with the legacy heuristics instead of the precomputed StubUnwindInfo. The PR description says the same. With the reader-contention case fixed, the fallback happens only during the registration window.
| // 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); |
There was a problem hiding this comment.
Optional: up to you, not blocking.
The writer mutex is taken before the aarch64 analyzeStubUnwind() call, so every stub's instruction decode and allocation runs while _stubs_write_lock is held. The analysis only reads the stub's code and touches no shared state. Moving the MutexLocker below the #ifdef __aarch64__ analysis block would stop concurrent DynamicCodeGenerated calls (at startup, or when GenerateEvents replays them) from waiting on each other's scans. The duplicate check and prepareAdd() are the only parts that need the mutex.
There was a problem hiding this comment.
Done in 0da83b4. analyzeStubUnwind() now runs before _stubs_write_lock. It reads only the stub code, and its one shared write is an atomic counter.
| // 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. |
There was a problem hiding this comment.
Optional: up to you, not blocking.
This comment is slightly imprecise. "this thread" refers to two different threads, and "that lookup could never succeed" describes the old unbounded lockShared(). With the bounded readers in this PR, the lookup returns false instead of spinning. Maybe say it explicitly: the bounded reader is what prevents the deadlock, and allocating outside the lock keeps the window short in which lookups fail and fall back to the degraded unwind.
There was a problem hiding this comment.
Rewritten in 0da83b4. The comment now says that the bounded readers prevent the deadlock, and that allocating outside the lock keeps the window short, during which lookups fail and fall back.
| * 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") \ |
There was a problem hiding this comment.
Optional: up to you, not blocking.
This counter combines "a writer held the lock" (an expected, short registration window) with "the spin budget ran out" (reader contention). If you switch to the no-argument tryLockShared(), only the first cause remains and this goes away. Otherwise, two counters would make the telemetry possible to act on.
There was a problem hiding this comment.
The switch to tryLockShared() fixes this. Only the "writer holds the lock" cause remains, and the counter comment says so.
| int capacity; // _capacity that this preparation is valid for | ||
| }; | ||
|
|
||
| // Two-phase add() for a cache whose readers run in signal handlers under a |
There was a problem hiding this comment.
Optional: up to you, not blocking.
A design point to consider. The bounded readers alone already break the deadlock, so prepareAdd()/commitAdd() plus the second mutex are about keeping the exclusive window short. I think that's worth keeping: without it, a writer blocked in malloc holds the lock exclusively for an unbounded time, every stub lookup fails during that time, and runtimeStubsMemoryUsage() waits behind it.
The rules, though, are enforced only by debug asserts: exactly one commitAdd() per prepareAdd(), nothing changing the cache in between, the caller deletes the returned array after unlocking, and writers are serialized externally. To make misuse harder, PreparedAdd could be an RAII type that frees its name copy and grown array if it is never committed.
There was a problem hiding this comment.
Done in 8aba764. PreparedAdd now owns its allocations. The destructor frees the name copy and the grown array unless commitAdd() took them, and commitAdd() takes ownership by setting both fields to null. The type is non-copyable, and prepareAdd() asserts that it gets a fresh object. New tests cover an uncommitted prepare, including one that holds a grown array, and the transfer of ownership.
| } // 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 |
There was a problem hiding this comment.
Optional: up to you, not blocking.
Two small points:
- The tests cover the bounded-reader half of the fix, but nothing would catch a
malloc/new/delete[]moved back inside the_stubs_lock.lock()/unlock()block inDynamicCodeGeneratedoutside ASan. That half is hard to test directly. A debug-only "allocation forbidden" flag, or at least a clear comment at the critical section, would help. - "Before the fix the lookup spun in lockShared()" describes history. It would be better to describe what the test checks now.
There was a problem hiding this comment.
- Allocation guard: Add signal-reachability checker and source quality checks (qualityChecks task) #893 adds
checkSignalReachability, which reports every path from a signal handler or malloc/free hook to an async-signal-unsafe sink, including allocations. It found this defect, and it keeps PROF-16313 allowlisted until this PR merges. After the merge, the allowlist entry goes, so any allocation that moves back into a path reachable from a signal handler failsqualityChecks. I'd rather rely on that than add a runtime flag in debug builds only. - Test comment: rewritten in 0da83b4 to describe what the test checks.
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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f720327 to
b85dbe8
Compare
What does this PR do?:
Removes a JVM deadlock between
JitCodeCache::DynamicCodeGeneratedand the sampling signal handler.findRuntimeStub()andfindRuntimeStubInfo()take the lock withOptionalSharedLockGuard(&_stubs_lock, SpinLock::NO_SPIN_BUDGET). While a registration holds the lock exclusively, the lookup returns "not found", incrementsstub_lookup_lock_unavailable, and the stack walk uses its fallback unwind. Competing readers do not make the lookup fail.DynamicCodeGeneratedallocates and frees memory only outside_stubs_lock. Inside the lock, it only stores and swaps pointers.findRuntimeStub()copies the stub start and name while it holds the lock. The old version returned aCodeBlob*that a concurrent registration could free.DynamicCodeGeneratedfrees the unwind info of a stub when the registry cannot grow.CodeCache::PreparedAddowns its allocations: its destructor frees them unlesscommitAdd()takes them. Every_stubs_locksite uses a guard (OptionalSharedLockGuard,ExclusiveLockGuard,SharedLockGuard).NO_SPIN_BUDGETguard modeOptionalSharedLockGuardacceptsSpinLock::NO_SPIN_BUDGET. With it, the guard fails only while a writer holds the lock, and competing readers do not make it fail. Existing callers keep the spin budget.utils/check-signal-reachability.pyno longer allowlistsfindRuntimeStub/findRuntimeStubInfo -> SpinLock::lockShared. The allowlist is empty.AGENTS.mdRegistration path in detail
_stubs_write_lock(newMutex). It orders concurrent writers. Signal handlers never take it._stubs_lock:CodeCache::prepareAdd()copies the name and, when the blob array is full, allocates and fills a larger array.ExclusiveLockGuardblock on_stubs_lock:CodeCache::commitAdd()and the registry insert store and swap pointers only._stubs_lock, then free the replaced arrays.CodeCache::add()callsprepareAdd()andcommitAdd()and keeps the same results and memory accounting. APreparedAddthat is never committed frees its name copy and grown array in its destructor. This PR removesCodeCache::expand().Motivation:
Issue #843 reports this cycle (JDK 25, glibc):
DynamicCodeGenerated_stubs_lock(exclusive)mallocmalloc_stubs_lock, spinning inlockShared()_stubs_lock, spinning inlockShared()The JVM cannot reach a safepoint, and the sampled threads use 100% CPU.
The bounded lookup also prevents a wait on the same thread. The thread that holds
_stubs_lockcan take a sample of itself through a profiling signal or through the malloc hook. When its stack contains a runtime-stub frame, the walk calls the stub lookup.Additional Notes:
_stubs_write_lock, then_stubs_lock. Readers take only_stubs_lock.runtimeStubsMemoryUsage()runs on the dump thread, so it waits for the lock (SharedLockGuard).StubUnwindInfo, so the caller frame is less reliable for that sample.CodeHeap::findNMethod()whenVMStructs::hasMethodStructs()is true.checkSignalReachability(Add signal-reachability checker and source quality checks (qualityChecks task) #893) reports every call path from a signal handler or malloc/free hook to an async-signal-unsafe sink. It found this defect asJitCodeCache::findRuntimeStub -> SpinLock::lockShared(viaPerfEvents::signalHandler -> Profiler::recordSample -> ... -> HotspotSupport::walkVM). With the allowlist entry removed, a signal-reachable unbounded stub-lock wait failsqualityChecks.How to test the change?:
New tests:
FindRuntimeStubDoesNotWaitForExclusiveHolder_stubs_lockheld, the lookup returns "not found" within 5 s and increments the counter. On base commitaf819c666, the test fails.FindRuntimeStubInfoDoesNotWaitForExclusiveHolderRegistrationsAcrossGrowthStayFindableDuplicateRegistrationKeepsFirstInfoConcurrentRegistrationAndLookupTwoPhaseAddGrowsOnlyWhenFullprepareAdd()/commitAdd()grow only when full and keep the exact memory accounting.TwoPhaseAddReplacesNonPrintableNameprepareAdd()replaces non-printable name characters.UncommittedPrepareFreesAndLeavesCacheUnchangedPreparedAddthat holds a grown array and is never committed leaves the cache unchanged. LeakSanitizer reports its allocations if the destructor does not free them.CommitAddTakesOwnershipcommitAdd(), thePreparedAddno longer refers to the name or the grown array.OptionalGuard_NoSpinBudget_*(3 tests)NO_SPIN_BUDGET, the guard fails while a writer holds the lock, acquires and releases a free lock, and acquires 100,000 times out of 100,000 while four threads compete as readers. RoutingNO_SPIN_BUDGETthrough the budgeted overload makes the contention test fail.No automated test checks that the exclusive section makes no allocator calls. Code review covers that point.
Results at
b85dbe82a::ddprof-lib:gtestDebug,:ddprof-lib:gtestRelease:ddprof-lib:qualityCheckscheckSignalReachabilityskips on macOS (no ELF objects).:ddprof-lib:qualityChecks(fresh clone of this branch)checkSignalReachability: 0 findings with an empty allowlist, self-test passes.checkSignalReachabilityonmain(c9a40e7b0) with the entry removedfindRuntimeStub -> SpinLock::lockSharedspinLock_ut,codeCache_ut,jitCodeCache_ut:ddprof-test:testDebugOn the first commit, macOS arm64
testDebughad 3 failures (VtableReceiverFrameTest,NativeThreadTest,DynamicNativeThread). The same 3 fail on the unmodified baseaf819c666.For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.Closes #843
Unsure? Have a question? Request a review!
🤖 Generated with Claude Code