From 6e5532c25455660de7a7b2da019e6aaafc7e2609 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Tue, 29 Sep 2026 12:38:25 +0200 Subject: [PATCH 1/2] fix(profiling): emit one addressing convention for every frame 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) --- ddprof-lib/src/main/cpp/perfEvents_linux.cpp | 15 +++++++- ddprof-lib/src/main/cpp/profiler.cpp | 9 ++--- ddprof-lib/src/main/cpp/stackWalker.h | 35 ++++++++++++------- .../src/test/cpp/walkVmAttribution_ut.cpp | 24 ++++++++----- doc/reference/RemoteSymbolication.md | 23 ++++++++++-- 5 files changed, 78 insertions(+), 28 deletions(-) diff --git a/ddprof-lib/src/main/cpp/perfEvents_linux.cpp b/ddprof-lib/src/main/cpp/perfEvents_linux.cpp index a168b2ade9..d075c3663a 100644 --- a/ddprof-lib/src/main/cpp/perfEvents_linux.cpp +++ b/ddprof-lib/src/main/cpp/perfEvents_linux.cpp @@ -36,6 +36,7 @@ #include "spinLock.h" #include "stackFrame.h" #include "stackWalker.h" +#include "stackWalker.inline.h" #include "symbols.h" #include "threadLocalData.inline.h" #include "threadState.inline.h" @@ -1072,6 +1073,13 @@ int PerfEvents::walkKernel(int tid, const void **callchain, int max_depth, struct perf_event_header *hdr = ring.seek(tail); if (hdr->type == PERF_RECORD_SAMPLE) { u64 nr = ring.next(); + // perf_callchain_user hands back the interrupted pc first and return + // addresses after it, so everything past the leaf is recorded as the + // address the call was made from -- the convention walkFP, walkDwarf + // and walkVM already put into callchain[]. java_ctx->pc keeps the exact + // address in every case: it is a resume point handed to the JVM, not + // something anyone symbolizes. + bool is_leaf = true; while (nr-- > 0) { u64 ip = ring.next(); if (ip < PERF_CONTEXT_MAX) { @@ -1081,10 +1089,15 @@ int PerfEvents::walkKernel(int tid, const void **callchain, int max_depth, java_ctx->pc = iptr; goto stack_complete; } - callchain[depth++] = iptr; + callchain[depth++] = attributionPC(iptr, !is_leaf); + is_leaf = false; } } + // Deliberately unadjusted below: LBR records branch endpoints, not + // return addresses. `from` is the branch instruction itself and `to` + // is its target, so both already name the instruction to symbolize; + // subtracting one would step off the front of it. if (_cstack == CSTACK_LBR) { u64 bnr = ring.next(); diff --git a/ddprof-lib/src/main/cpp/profiler.cpp b/ddprof-lib/src/main/cpp/profiler.cpp index 2c1b3271c3..bfd8d09945 100644 --- a/ddprof-lib/src/main/cpp/profiler.cpp +++ b/ddprof-lib/src/main/cpp/profiler.cpp @@ -393,8 +393,9 @@ void Profiler::populateRemoteFrame(ASGCT_CallFrame* frame, uintptr_t pc, CodeCac * Range-based lookups (findLibraryByAddress, binarySearch) key off the * attribution address, so a call that is the last instruction of its caller * still selects the caller rather than whatever follows it. The emitted - * pc_offset keeps using the raw pc: it is the remote-symbolication wire value - * and its meaning is a cross-team contract, not a local lookup detail. + * pc_offset is derived from that same address, which is what makes every + * frame in a trace mean the same thing: see the convention note on + * StackWalker in stackWalker.h. */ Profiler::NativeFrameResolution Profiler::resolveNativeFrameForWalkVM(uintptr_t pc, bool pc_is_return_address, int lock_index) { const void* lookup_pc = attributionPC((const void*)pc, pc_is_return_address); @@ -411,7 +412,7 @@ Profiler::NativeFrameResolution Profiler::resolveNativeFrameForWalkVM(uintptr_t } // Pack remote symbolication data using utility struct - uintptr_t pc_offset = pc - (uintptr_t)lib->imageBase(); + uintptr_t pc_offset = (uintptr_t)lookup_pc - (uintptr_t)lib->imageBase(); uint32_t lib_index = (uint32_t)lib->libIndex(); unsigned long packed = RemoteFramePacker::pack(pc_offset, mark, lib_index); @@ -434,7 +435,7 @@ Profiler::NativeFrameResolution Profiler::resolveNativeFrameForWalkVM(uintptr_t // Reuses BCI_NATIVE_FRAME_REMOTE encoding; resolveMethod() in flightRecorder.cpp // distinguishes remote vs local rendering via hasBuildId() && isRemoteSymbolication(). if (method_name == nullptr && lib != nullptr) { - uintptr_t pc_offset = pc - (uintptr_t)lib->imageBase(); + uintptr_t pc_offset = (uintptr_t)lookup_pc - (uintptr_t)lib->imageBase(); uint32_t lib_index = (uint32_t)lib->libIndex(); unsigned long packed = RemoteFramePacker::pack(pc_offset, 0, lib_index); return NativeFrameResolution(packed, BCI_NATIVE_FRAME_REMOTE); diff --git a/ddprof-lib/src/main/cpp/stackWalker.h b/ddprof-lib/src/main/cpp/stackWalker.h index 6e3a2da76c..5bd8ab9e1c 100644 --- a/ddprof-lib/src/main/cpp/stackWalker.h +++ b/ddprof-lib/src/main/cpp/stackWalker.h @@ -94,20 +94,29 @@ class StackWalker { // exactly max_depth is overflowed by one entry on any stack deep enough to // reach the limit. Without a truncation flag, max_depth entries suffice. // - // callchain[] entries from walkFP/walkDwarf are attribution addresses: - // pc - 1 for any frame whose pc was loaded from a return-address slot - // (see attributionPC in stackWalker.inline.h), the exact pc otherwise. - // Frames produced by walkVM/walkKernel are not adjusted this way and - // still carry raw addresses; callers merging chains from different - // walkers must not assume a single addressing convention across all - // entries. + // ADDRESSING CONVENTION -- one rule, every walker, in-process and on the + // wire. // - // This reaches the wire: Profiler::populateRemoteFrame derives the - // remote-symbolication pc_offset straight from a callchain[] entry, so - // for these two walkers the emitted offset already points inside the - // call instruction and must not be adjusted again off-process, while - // walkVM/walkKernel frames in the same trace still need that - // adjustment. The packed field carries no bit distinguishing the two. + // A recorded frame address is an *attribution* address: pc - 1 wherever + // the pc came out of a return-address slot, the exact pc otherwise (see + // attributionPC in stackWalker.inline.h). It names the instruction that + // transferred control, not the one execution would resume at, so a call + // that is the last instruction of its caller still resolves to the caller. + // + // This holds for walkFP, walkDwarf, walkVM and walkKernel alike. LBR + // entries are the one thing left unadjusted, and are not an exception to + // the rule: branch endpoints already name the instruction to symbolize. + // + // StackContext::pc is deliberately *not* adjusted -- it is a resume point + // handed to the JVM, not something to symbolize. + // + // It reaches the wire unchanged: the remote-symbolication pc_offset is + // this address minus the image base, whether it is packed by + // populateRemoteFrame or by resolveNativeFrameForWalkVM. An off-process + // symbolizer must therefore NOT apply its own return-address adjustment; + // doing so would step off the front of the call instruction. The packed + // field carries no version bit, so this comment and + // doc/reference/RemoteSymbolication.md are the contract. static int walkFP(void* ucontext, const void** callchain, int max_depth, StackContext* java_ctx, bool* truncated = nullptr); static int walkDwarf(void* ucontext, const void** callchain, int max_depth, StackContext* java_ctx, bool* truncated = nullptr); }; diff --git a/ddprof-lib/src/test/cpp/walkVmAttribution_ut.cpp b/ddprof-lib/src/test/cpp/walkVmAttribution_ut.cpp index d4a7389e28..0dd6b3a154 100644 --- a/ddprof-lib/src/test/cpp/walkVmAttribution_ut.cpp +++ b/ddprof-lib/src/test/cpp/walkVmAttribution_ut.cpp @@ -137,10 +137,17 @@ TEST_F(WalkVmAttributionTest, AddressInsideAFunctionResolvesTheSameEitherWay) { EXPECT_STREQ("walkvm_attr_first", as_ra.method_name); } -// The wire contract: the lookup may move, the emitted offset may not. This is -// what keeps walkVM out of the unresolved cross-team question about what -// pc_offset means to the backend symbolizer. -TEST_F(WalkVmAttributionTest, PcOffsetStaysDerivedFromTheRawPc) { +// The wire contract: the emitted offset is the attribution address minus the +// image base, the same address the lookups used, so an off-process symbolizer +// can resolve it without knowing how the frame was recovered -- and must not +// adjust it again. +// +// This assertion was inverted once. It previously pinned the offset to the raw +// pc, because walkFP/walkDwarf had already moved to attribution addresses and +// changing walkVM too would have made the emitted value differ from what a +// consumer might be relying on. No service consumed it, so the three walkers +// were converged on the one convention instead of preserving the split. +TEST_F(WalkVmAttributionTest, PcOffsetUsesTheAttributionAddress) { const char* pc = kBareLibBase + kUnnamedOff; Profiler::NativeFrameResolution exact = resolve(pc, /*pc_is_ra=*/false); @@ -152,10 +159,11 @@ TEST_F(WalkVmAttributionTest, PcOffsetStaysDerivedFromTheRawPc) { uintptr_t exact_off = Profiler::RemoteFramePacker::unpackPcOffset(exact.packed_remote_frame); uintptr_t as_ra_off = Profiler::RemoteFramePacker::unpackPcOffset(as_ra.packed_remote_frame); - EXPECT_EQ((uintptr_t)kUnnamedOff, exact_off); - EXPECT_EQ((uintptr_t)kUnnamedOff, as_ra_off) - << "flagging the pc as a return address must not shift the emitted offset -- " - << "the attribution address is a lookup detail and must not reach the wire"; + EXPECT_EQ((uintptr_t)kUnnamedOff, exact_off) + << "an exact pc is already the instruction to symbolize, so it is emitted as-is"; + EXPECT_EQ((uintptr_t)kUnnamedOff - 1, as_ra_off) + << "a return address is emitted as the address the call was made from, " + << "so the symbolizer resolves the caller without adjusting anything itself"; } // =========================================================================== diff --git a/doc/reference/RemoteSymbolication.md b/doc/reference/RemoteSymbolication.md index fd660996e1..3729fe0900 100644 --- a/doc/reference/RemoteSymbolication.md +++ b/doc/reference/RemoteSymbolication.md @@ -37,8 +37,27 @@ Added fields to store build-id information: - Library index: 17 bits = 131K libraries max - **RemoteFrameInfo**: Structure for JFR serialization (vmEntry.h): - `build_id`: Library build-id string - - `pc_offset`: PC offset within library + - `pc_offset`: PC offset within library — see the addressing contract below - `lib_index`: Library table index + +### Addressing contract for `pc_offset` + +`pc_offset` is an **attribution** address minus the image base: it names the +instruction that transferred control, not the one execution resumes at. For a +frame whose pc was read out of a return-address slot that is `pc - 1`; for an +interrupted leaf it is the pc itself. + +**An off-process symbolizer must not apply its own return-address adjustment.** +Subtracting one again steps off the front of the call instruction and can +attribute the frame to whatever precedes it. + +Every walker emits this convention — `walkFP`, `walkDwarf`, `walkVM` and +`walkKernel` — so all frames in a trace mean the same thing. LBR entries are +recorded unadjusted, which is the same rule rather than an exception: branch +endpoints already name the instruction to symbolize. + +The packed field carries no version bit, so this section and the comment on +`StackWalker` in `stackWalker.h` are the contract. - **BCI_NATIVE_FRAME_REMOTE**: Frame encoding (-19) indicates packed remote data ### 4. **Enhanced Frame Collection** (`profiler.cpp`, `stackWalker.h`) @@ -65,7 +84,7 @@ Modified frame collection to support dual modes: **Stack Walker Integration**: - **walkFP/walkDwarf**: Return raw PCs → `convertNativeTrace()` → `populateRemoteFrame()` -- **walkVM/walkVMX**: Directly call `resolveNativeFrameForWalkVM(pc, pc_is_return_address, lock_index)` during stack walk. `pc_is_return_address` says whether the walker took this pc out of a return-address slot: the symbol and library lookups then key off the attribution address (`pc - 1`), while the emitted `pc_offset` keeps deriving from the raw pc, so the wire value is unchanged. +- **walkVM/walkVMX**: Directly call `resolveNativeFrameForWalkVM(pc, pc_is_return_address, lock_index)` during stack walk. `pc_is_return_address` says whether the walker took this pc out of a return-address slot; both the symbol/library lookups and the emitted `pc_offset` are then derived from the attribution address, per the contract above. ### 5. **JFR Serialization** (`flightRecorder.cpp/h`) From 33dc262eeaffff46018b91c09d3d00ef0883b1a2 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Wed, 30 Sep 2026 11:49:44 +0200 Subject: [PATCH 2/2] docs: update comments to the uniform attribution-address convention 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 --- ddprof-lib/src/main/cpp/profiler.cpp | 12 +++++------- ddprof-lib/src/main/cpp/stackWalker.inline.h | 10 +++++----- ddprof-lib/src/test/cpp/walkVmAttribution_ut.cpp | 10 ++-------- 3 files changed, 12 insertions(+), 20 deletions(-) diff --git a/ddprof-lib/src/main/cpp/profiler.cpp b/ddprof-lib/src/main/cpp/profiler.cpp index bfd8d09945..92f8fafb74 100644 --- a/ddprof-lib/src/main/cpp/profiler.cpp +++ b/ddprof-lib/src/main/cpp/profiler.cpp @@ -346,13 +346,11 @@ int Profiler::getNativeTrace(void *ucontext, ASGCT_CallFrame *frames, * symbol lookups to post-processing while still capturing marks needed for * correct stack walk termination. * - * The emitted pc_offset inherits whatever addressing convention the - * producing walker used for this frame (see StackWalker in stackWalker.h): - * walkFP/walkDwarf frames arrive already pointing inside the call - * instruction, while walkVM/walkKernel frames arrive unadjusted (their leaf - * is the exact interrupted pc, the frames above it raw return addresses). - * An off-process symbolizer that applies its own return-address adjustment - * therefore double-adjusts the former. + * The emitted pc_offset is the attribution address (see the convention note + * on StackWalker in stackWalker.h): every walker already hands over an + * address inside the call instruction for frames loaded from a return-address + * slot, and the exact pc otherwise. An off-process symbolizer must therefore + * not apply its own return-address adjustment. * * @param frame The ASGCT_CallFrame to populate * @param pc The program counter address diff --git a/ddprof-lib/src/main/cpp/stackWalker.inline.h b/ddprof-lib/src/main/cpp/stackWalker.inline.h index 965f554cfc..e10708fab0 100644 --- a/ddprof-lib/src/main/cpp/stackWalker.inline.h +++ b/ddprof-lib/src/main/cpp/stackWalker.inline.h @@ -55,11 +55,11 @@ inline void fillFrame(ASGCT_CallFrame& frame, FrameTypeId type, int bci, jmethod // value; callers that need the exact resume/executed address (JIT handoff, // no-progress guards, pc arithmetic) must keep using the raw walking pc. // -// Only walkFP/walkDwarf produce adjusted addresses. walkVM/walkKernel still -// hand out raw addresses and are therefore still subject to the boundary -// misattribution this function fixes: CodeCache::binarySearch's fallback -// rescues only the padding-gap and zero-size-symbol cases, not zero-gap -// adjacency (see BinarySearchPicksNextSymbolAtZeroGapBoundary). +// walkFP, walkDwarf, walkVM and walkKernel all record this adjusted address. +// CodeCache::binarySearch's fallback rescues only the padding-gap and +// zero-size-symbol cases, not zero-gap adjacency (see +// BinarySearchPicksNextSymbolAtZeroGapBoundary), which is why the adjustment +// is needed. inline const void* attributionPC(const void* pc, bool pc_is_return_address) { return pc_is_return_address ? (const void*)((const char*)pc - 1) : pc; } diff --git a/ddprof-lib/src/test/cpp/walkVmAttribution_ut.cpp b/ddprof-lib/src/test/cpp/walkVmAttribution_ut.cpp index 0dd6b3a154..6b2a069288 100644 --- a/ddprof-lib/src/test/cpp/walkVmAttribution_ut.cpp +++ b/ddprof-lib/src/test/cpp/walkVmAttribution_ut.cpp @@ -7,8 +7,8 @@ // HotspotSupport::walkVM relies on for every non-Java frame: range lookups // (findLibraryByAddress/binarySearch) key off the attribution address so a // call that is the last instruction of its caller still resolves to the -// caller, while the remote-symbolication pc_offset keeps deriving from the -// raw pc because its meaning is a cross-team wire contract. +// caller, while the remote-symbolication pc_offset is derived from that same +// attribution address because its meaning is a cross-team wire contract. // // These use synthetic CodeCaches rather than the test binary's own symbols, // so unlike returnAddressAttribution_ut.cpp they need no GNU-as/ELF-CFI asm @@ -141,12 +141,6 @@ TEST_F(WalkVmAttributionTest, AddressInsideAFunctionResolvesTheSameEitherWay) { // image base, the same address the lookups used, so an off-process symbolizer // can resolve it without knowing how the frame was recovered -- and must not // adjust it again. -// -// This assertion was inverted once. It previously pinned the offset to the raw -// pc, because walkFP/walkDwarf had already moved to attribution addresses and -// changing walkVM too would have made the emitted value differ from what a -// consumer might be relying on. No service consumed it, so the three walkers -// were converged on the one convention instead of preserving the split. TEST_F(WalkVmAttributionTest, PcOffsetUsesTheAttributionAddress) { const char* pc = kBareLibBase + kUnnamedOff;