Conversation
A null walking pc can reach attributionPC with pc_is_return_address=true when an optimistic unwind reads a zeroed return-address slot. Pointer arithmetic on nullptr is UB and UBSan (asan nightly config) aborts the test JVM on it. A null pc has no code to attribute either way, so pass it through unchanged.
9010c4c switched CallTraceSet to CountingAllocator; the harness lambda still declared std::unordered_set<CallTrace*> with the default allocator, which is not convertible to std::function<void(const CallTraceSet&)>
…d_count The counter delta spans the whole churn window (dumps and background JFR flushes) while the label assertion read only the last dump file; a stale trace can be evicted from the call-trace storage before the final dump, making the test flaky across JDKs/platforms. Snapshot the dump whose window observed the counter crossing and assert on it; if the counter only fired between dumps, take one more dump before stop.
CI Test ResultsRun: #36703717990 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 76 | Passed: 76 | Failed: 0 Updated: 2026-09-30 11:04:52 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. |
|
@rkennke Not sure if these changes are not redoing some things from your currently open PRs, let's wait until they are merged to see if this still makes sense. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbfa879569
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return pc != nullptr && pc_is_return_address | ||
| ? (const void*)((const char*)pc - 1) | ||
| : pc; |
There was a problem hiding this comment.
When an optimistic unwind reads a zeroed return-address slot, this new condition is the only behavior preventing the original UBSan failure, but no automated test under ddprof-lib/src/test or ddprof-test invokes attributionPC(nullptr, true) or exercises an equivalent WalkPc path. Add a focused test that fails against the parent implementation and verifies that a null PC passes through unchanged, so this sanitizer regression cannot return unnoticed.
AGENTS.md reference: AGENTS.md:L445-L446
Useful? React with 👍 / 👎.
![datadog-prod-us1-6[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The retained first counter-firing dump can contain the stale frame only in datadog.ObjectSample, but the assertion ignores that event type, so the nightly stress test can still fail even when the intended <unloaded> label was emitted.
🤖 Bits Code Review · Commit fbfa879 · @DataDog review to ask questions
| // With skippedDelta > 0, firedDumpFile is always set: either the dump whose window | ||
| // observed the counter crossing (label emitted in that same fillJavaMethodInfo call) | ||
| // or the extra post-churn dump taken above. | ||
| assertUnloadedFrameLabel(firedDumpFile); |
There was a problem hiding this comment.
When the first counter increase comes from an allocation trace, the retained dump may contain the stale frame only in datadog.ObjectSample. The assertion instead scans nonexistent datadog.AllocationSample, causing a false failure even though the expected <unloaded> label was emitted.
Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session
kaahos
left a comment
kaahos
left a comment
There was a problem hiding this comment.
looks good to me; thanks for the fix!
This branch has not been deployed
2 participants
What does this PR do?:
Fixes the three failure classes recurring in the Nightly Sanitized Run (e.g. https://github.com/DataDog/java-profiler/actions/runs/36663570752):
UBSan: applying non-zero offset to null pointer —
attributionPC()(stackWalker.inline.h) does(char*)pc - 1for return-address pcs. An optimistic unwind can read a zeroed return-address slot, sopc == nullptrwithpc_is_return_address == truereaches it, UBSan (asan config) reports the error and the test JVM exits 1, killing everyrun-slow-test-asanjob. Fix: pass a null pc through unchanged (behavior-identical to pre-PROF-15955 walkers —findLibraryByAddress(nullptr)fails either way).Fuzz harness no longer compiles —
9010c4c3aswitchedCallTraceSettoCountingAllocator, but thefuzz_callTraceStorage.cpplambda still declaredconst std::unordered_set<CallTrace*>&(default allocator), which is not convertible tostd::function<void(const CallTraceSet&)>.compileFuzz_callTraceStoragehas failed every nightly since. Fix: useconst CallTraceSet&.JMethodIDInvalidationStressTestflake (graal/musl/glibc, JDK 21/25) —jmethodid_skipped_countaccumulates over the whole churn window (dumps and background JFR flushes), but the<unloaded>label assertion read only the last dump file; the stale trace can be evicted from the call-trace storage before the final dump. Fix: snapshot the dump whose counter window fired (the increment and the<unloaded>label are emitted in the samefillJavaMethodInfocall, so that recording is guaranteed to carry the label); if the counter only fired between dumps, take one extra dump beforestop().The
cache-jdks / cache-amd64-muslfailure in the same run is an Alpine CDN TLS infra flake, untouched.Motivation:
Nightly Sanitized Run failing regularly for the last few weeks, masking real regressions. The fuzz failure also showed that a job failure can coexist with a run-level "success" conclusion, so per-job status is the reliable signal.
Additional Notes:
testSlowDebugJMethodID test passes; full debug gtest suite (530 tests) green; spotless clean.:ddprof-lib:fuzz:compileFuzz_callTraceStorageBUILD SUCCESSFUL; asan-config gtests (walkVmAttribution_ut,returnAddressAttribution_ut,stackWalker_ut,test_callTraceStorage) green; JMethodID test 1 + 4--rerunruns all green (test executes, not skipped).How to test the change?:
./gradlew :ddprof-lib:fuzz:compileFuzz_callTraceStorage(was failing on Linux CI)./gradlew :ddprof-lib:gtestAsan_walkVmAttribution_ut :ddprof-lib:gtestAsan_returnAddressAttribution_ut :ddprof-lib:gtestAsan_stackWalker_ut :ddprof-lib:gtestAsan_test_callTraceStorage./gradlew :ddprof-test:testSlowDebug -Ptests=JMethodIDInvalidationStressTest(repeat with--rerun)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.