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..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 @@ -393,8 +391,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 +410,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 +433,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/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 d4a7389e28..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 @@ -137,10 +137,11 @@ 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. +TEST_F(WalkVmAttributionTest, PcOffsetUsesTheAttributionAddress) { const char* pc = kBareLibBase + kUnnamedOff; Profiler::NativeFrameResolution exact = resolve(pc, /*pc_is_ra=*/false); @@ -152,10 +153,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`)