Emit one addressing convention for every frame - #830
Merged
Merged
Conversation
A single trace carried two. walkFP and walkDwarf put attribution addresses into callchain[], so their pc_offset came out as the address the call was made from. walkVM derived its offset from the raw pc, and walkKernel passed the kernel's return addresses through untouched. Nothing marked which was which, and the packed field has no version bit to tell them apart. PROF-15934 left this open because the wire value looked like a cross-team contract worth not moving. It turns out nothing consumes it, so the split is worth closing now rather than preserving indefinitely. Attribution wins over raw because callchain[] feeds local symbolication too: going the other way would reintroduce the boundary misattribution PROF-15934 fixed, or need a parallel flag array to avoid it. walkVM's offset now comes from the address its lookups already used. walkKernel adjusts everything past the leaf, the kernel having handed back the interrupted pc first and return addresses after it. LBR entries stay as they are, which is the same rule rather than an exception: a branch endpoint already names the instruction to symbolize. StackContext::pc also stays exact -- it is a resume point handed to the JVM, not something anyone symbolizes. The contract is written down on StackWalker and in RemoteSymbolication.md, including the part a consumer needs: do not adjust these offsets again. PcOffsetStaysDerivedFromTheRawPc asserted the old behaviour and was mutation-checked that way, so it is inverted here rather than deleted, with a note saying why -- the constraint it encoded was deliberate and is now lifted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![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: 6e5532c254
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![datadog-prod-us1-4[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Contributor
CI Test ResultsRun: #36706309429 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-09-30 11:24:16 UTC |
Contributor
jbachorik
approved these changes
Sep 30, 2026
Collaborator
|
LGTM! |
walkVM/walkKernel now emit attribution addresses like walkFP/walkDwarf; drop the remaining comments describing them as raw-PC producers. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the
pc_offsetquestion PROF-15934 left open, now that we know nothing consumes the value.Addresses PROF-16096. Follows PROF-15934 / PROF-15955 / PROF-16059.
The problem
One trace carried two conventions, and nothing said which was which:
pc_offsetwalkFP/walkDwarf(RA - 1) - imageBase—callchain[]holds attribution addresses since #786walkVMRA - imageBase— lookups used the attribution address, the offset did notwalkKernel#786 flagged exactly this ("offsets are not uniform within a single stack") and deferred it, because the wire value looked like a cross-team contract worth not moving. #813 then deliberately froze walkVM on raw for the same reason.
That reason is gone: no service consumes the field today. So the split is worth closing now, while it costs nothing, rather than preserving it indefinitely behind a comment.
Which convention, and why
Attribution, not raw.
callchain[]is not only the wire payload — it feeds local symbolication throughconvertNativeTraceas well. #786 made those entries attribution addresses precisely to fix boundary misattribution. Converging on raw would reintroduce that bug, or require carrying a parallel flag array to avoid it — option (b) from #786's own list, with its stated cost. Attribution is also already the majority convention in-tree; walkVM and walkKernel were the outliers.Changes
profiler.cpp—resolveNativeFrameForWalkVMderivespc_offsetfromlookup_pc, the address its lookups already used. Two lines.perfEvents_linux.cpp—walkKerneladjusts everything past the leaf.perf_callchain_userhands back the interrupted pc first and return addresses after it, so the leaf is exact and the rest are not.stackWalker.h/RemoteSymbolication.md— the convention written down as one rule, including the part a consumer needs: do not apply your own return-address adjustment.Two things deliberately left unadjusted
LBR entries. Not an exception to the rule:
fromis the branch instruction andtois its target, so both already name the instruction to symbolize. Subtracting one would step off the front.StackContext::pc. A resume point handed to the JVM, not something anyone symbolizes. Now stated in the header so it does not read as an oversight.About the inverted test
PcOffsetStaysDerivedFromTheRawPcasserted the old behaviour and was mutation-checked in that direction. It is inverted here rather than deleted, renamedPcOffsetUsesTheAttributionAddress, with a comment recording that the constraint was deliberate and why it was lifted — otherwise the next reader finds a reversed assertion ingit logwith no explanation.I confirmed it failed before changing it (
2048vs2047), so the behaviour change is real and the test was genuinely gating it.Verification
gtestDebugandgtestRelease: 71 binaries, 0 failures each.walkKernelchange actually compiled here rather than assuming:perfEvents_linux.cpphas no enclosing platform guard and its.ois produced in both configs.Not verified locally: anything Linux-specific at runtime.
walkKernelneeds a live perf session, which no test exercises — the change there rests on the documentedperf_callchain_userordering plus review. CI's release and sanitizer gtests (#821) cover the build on both arches.Follow-up worth deciding separately
The packed field still has no version bit. With no consumer that costs nothing today, but if one appears later and guesses wrong there is no way to detect it. Recording the convention in JFR metadata rather than the packed field would be the cheap insurance. Not done here.
🤖 Generated with Claude Code