Repository navigation
Conversation
Publish _request_stack_trace only after InitializeRequestStackTrace succeeds and store _init_request_stack_trace atomically (RELEASE/ACQUIRE), so canRequestStackTrace() reports 'initialized', not merely 'present'. Guard the extension params deallocation loop against a NULL params array.
Fix the synchronization-edge comment, drop the dead rollback store, pin exact Deallocate counts (with a null sentinel slot catching the off-by-one loop bound), and cover initializeRequestStackTrace()'s unset-init guard.
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-prod-us1-5[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Publishing the request function only after successful initialization is consistent with the reviewed callers and fallback paths. The null params guard also preserves extension cleanup without dereferencing a missing array.
🤖 Bits Code Review · Commit 17aa9d2
![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: 17aa9d2588
ℹ️ 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".
| @@ -419,28 +430,42 @@ void VM::probeJFRRequestStackTrace() { | |||
| } | |||
|
|
|||
| if (_agent_args._jvmtistacks) { | |||
There was a problem hiding this comment.
When the profiler is loaded through the normal JNI path, JavaProfiler_init0() calls initProfilerBridge() before any recording arguments are parsed, so _agent_args._jvmtistacks retains its default false value. This condition now prevents _request_stack_trace from ever being published, and the bridge is not probed again when a later start,jvmtistacks=true command reaches Profiler::selectCpuEngine(). Consequently, JNI-loaded users requesting JVMTI stacks at recording start always see canRequestStackTrace() == false and silently fall back to ASGCT; initialization/publication needs to be deferred to the recording option path or remain available after the initial probe.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the JNI load path (JavaProfiler_init0 → initProfilerBridge) probes before any recording options are parsed, so _agent_args._jvmtistacks was still false and a per-recording start,jvmtistacks=true could never publish the extension.
Fixed in f0085fe: the probe now records the discovered extension function pointers unconditionally, and a new VM::enableRequestStackTrace() (idempotent, quiet when the extension is absent) initializes and publishes lazily. Profiler::start() and Profiler::check() call it when args._jvmtistacks is set, so per-recording options enable the extension at recording time. Covered by new gtest cases (LazyEnableAfterJvmtistacksDisabledProbe, LazyEnableQuietWhenExtensionAbsent, LazyEnableIdempotentAfterPublication); full :ddprof-lib:gtestDebug 1024 passed / 0 failed.
CI Test ResultsRun: #40896903180 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Failed Jobs
Summary: Total: 32 | Passed: 31 | Failed: 1 Updated: 2026-10-29 15:20:43 UTC |
|
❌ 39 passed, 1 failed out of 40 expected configurations Test Matrix
Failure Detailsmusl-x64-openj9-jdk25Profiler-only: Links
|
The JNI load path probes before recording options are parsed, so jvmtistacks was never requested at probe time and per-recording start,jvmtistacks=true silently fell back to ASGCT. Record discovered extension pointers unconditionally and add VM::enableRequestStackTrace(), called from Profiler::start()/check() when jvmtistacks is requested.
|
Not sure if it should be part of this change, but one thing to note is that the upstream proposal has changed significantly since our side of it has been written:
See also: JEP draft: Native Profiler Hook for Unbiased Stack Traces (Experimental) |
|
@rkennke That's actually a good call. Should this be fixed now or better wait till the upstream is final? |
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 3 Pipeline jobs failed
Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: f0085fe | Docs | View more details | Give us feedback! |
Not sure. On one hand, the code as it currently is would not work/detect the JVMTI extension at all, with the current implementation. OTOH, now that the JEP is (hopefully) moving forward, I would expect increased activity on the implementation, which would be a chase. It's probably better to just wait it out until it's final. |
What does this PR do?:
Fixes three defects in
VM::probeJFRRequestStackTrace()(PROF-14757):_init_request_stack_tracewas stored with a plain store while_request_stack_tracewas published with__ATOMIC_RELEASE, so a cross-thread reader ofcanRequestStackTrace()had no happens-before guarantee for the init pointer (latent on arm64)._request_stack_tracewas published during the extension scan, even whenjvmtistackswas not requested and beforeInitializeRequestStackTraceran, socanRequestStackTrace()reported "ready" for an uninitialized extension.params[j]without guarding against a NULLparamsarray._request_stack_traceis now published only after a successfulInitializeRequestStackTraceunderjvmtistacks=true; the_initpointer is stored (RELEASE) before it, makingcanRequestStackTrace()mean initialized, not merely present.Motivation:
Review findings from PR #537
Behavior change (intended): with
jvmtistacks=false, or whenInitializeRequestStackTracefails,canRequestStackTrace()now returnsfalse. All consumers were audited: engine selection gates on_jvmtistacks && canRequestStackTrace();Profiler::recordSampleDelegated()is reachable only from the JVMTI engines; the dump-time diagnostic fires only when requests were counted. On init failure the JVMTI engines fall back to ASGCT.Additional Notes:
initializeRequestStackTrace()'s guard now checks only_init_request_stack_trace; the caller owns the presence check for the request function before publishing.How to test the change?:
New mock-JVMTI gtest
test_vmEntryRequestStackTrace.cpp(8 cases) drivesprobeJFRRequestStackTrace()against a fakejvmtiEnv:paramswithparam_count > 0→ no crash (issue 3)jvmtistacks=false→ nothing published, init never invoked (issue 2)GetExtensionFunctionsfailure; failure counter incrementsDeallocatecall counts pinned (with a null sentinel slot that catches the off-by-one loop-bound mutation);initializeRequestStackTrace()guard with the init pointer unsetVerified:
:ddprof-lib:gtestDebug— 1020 tests passed, 0 failed:ddprof-test:testDebug -Ptests=JvmtiBasedPrecheckTest— SKIPPED identically with and without the change (pre-existing: JDK 27-ea on macOS arm64 does not satisfy thejvmti_stacks_init_ok > 0assumption)/sphinx review+ fix round): 0 CRITICAL/HIGH; 4 findings fixed, 4 dismissed with recorded reasons; fix-delta review cleanFor 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.