Emit one addressing convention for every frame

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();
 

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);

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);
 };

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;
 }

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";
 }
 
 // ===========================================================================

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`)