From 3ead3e2b3f81aa54599629735336881cf71aa866 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Tue, 22 Sep 2026 16:54:41 +0200 Subject: [PATCH 1/7] fix(profiling): select rows and symbols in walkVM by attribution address walkVM fed the raw walking pc to findLibraryByAddress, findFrameDesc and the DW_REG_PLT stub-offset test. Past the leaf that pc is a return address, so a call that is the last instruction of its caller selected the following function's CFA row, derived a sender sp from it, and could miss a MARK_THREAD_ENTRY sitting on the caller -- the same defect already fixed in StackWalker::walkFP/walkDwarf, on the path CSTACK_DEFAULT actually resolves to. Track per-frame whether the pc came out of a return-address slot and route the range-based lookups through attributionPC(). Exact-address consumers (isContReturnBarrier, isContEntryReturnPc, isEntryFrame), the DW_PC_OFFSET arithmetic and the no-progress guard keep the raw pc, and a signal-frame CIE suppresses the adjustment exactly as it does in walkDwarf. resolveNativeFrameForWalkVM was using one address for two jobs. It now takes the attribution address for findLibraryByAddress/binarySearch while the emitted pc_offset keeps deriving from the raw pc, so the remote-symbolication wire value is unchanged and no cross-team contract moves. unwindPrologue/unwindEpilogue/unwindStub are left alone: x86_64 already folds the adjustment into the pc they return and does so inconsistently (the isFrameComplete branch omits it) while aarch64 returns it raw, so their results are flagged as non-return-addresses and cannot be adjusted twice. Unifying that contract also fixes the exact-address comparisons those helpers currently break on x86_64, and is left to its own change. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/main/cpp/hotspot/hotspotSupport.cpp | 87 +++++++++++++++---- ddprof-lib/src/main/cpp/profiler.cpp | 16 +++- ddprof-lib/src/main/cpp/profiler.h | 2 +- 3 files changed, 83 insertions(+), 22 deletions(-) diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index b9adcd52af..1cb499ef76 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -326,6 +326,25 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex const void* prev_native_pc = NULL; + // Whether `pc` currently holds an address loaded from a return-address slot + // (a saved pc or a link register), which points at the instruction *after* a + // call. Range-based lookups -- findLibraryByAddress, findFrameDesc, the + // DW_REG_PLT stub-offset test -- key off attributionPC() instead, otherwise a + // call that is the last instruction of its caller selects whatever follows + // the caller. Exact-address consumers (isContReturnBarrier, + // isContEntryReturnPc, isEntryFrame), the DW_PC_OFFSET arithmetic and the + // no-progress guard keep using the raw pc. The entry pc comes from the + // ucontext, so it is an exact interrupted address. + // + // unwindPrologue/unwindEpilogue/unwindStub are deliberately excluded: on + // x86_64 they already fold the adjustment into the pc they return, and not + // even uniformly (unwindPrologue's isFrameComplete branch omits it), while on + // aarch64 they return the return address as-is. Flagging their results as + // non-return-addresses keeps this change from double-adjusting them; making + // that contract explicit is tracked separately. + bool pc_is_ra = false; + bool prev_native_pc_is_ra = false; + // Last ContinuationEntry crossed; advanced via parent() for nested continuations. VMContinuationEntry* cont_entry = nullptr; @@ -423,6 +442,8 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex sp = carrier_sp; fp = carrier_fp; pc = carrier_pc; + // Read out of the carrier frame's saved-pc slot. + pc_is_ra = true; return true; }; @@ -518,9 +539,13 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex saved_anchor_sp = anchor->lastJavaSP(); saved_anchor_fp = anchor->lastJavaFP(); } - if (anchor->getFrame(pc, sp, fp) && !nm->contains(pc)) { - anchor = NULL; - continue; // NMethod has changed as a result of correction + if (anchor->getFrame(pc, sp, fp)) { + // getFrame() redirects pc to lastJavaPC(), a return address. + pc_is_ra = true; + if (!nm->contains(pc)) { + anchor = NULL; + continue; // NMethod has changed as a result of correction + } } anchor = NULL; } else if (anchor_eligible && cont_unwind_active) { @@ -547,6 +572,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex HotspotSupport::fillJavaFrame(frames[depth++], FRAME_INTERPRETED, bci, method_id, method); sp = ((uintptr_t*)fp)[InterpreterFrame::sender_sp_offset]; pc = stripPointer(((void**)fp)[FRAME_PC_SLOT]); + pc_is_ra = true; fp = *(uintptr_t*)INJECT_FAULT_ADDRESS_UNLIKELY(fp); continue; } @@ -567,6 +593,8 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex pc = stripPointer(SafeAccess::load((void**)sp)); sp = frame.senderSP(); } + // Both arms read the sender pc out of a return-address slot. + pc_is_ra = true; continue; } } @@ -600,6 +628,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex if (nm->isFrameCompleteAt(pc)) { if (depth == 1 && frame.unwindEpilogue(nm, (uintptr_t&)pc, sp, fp)) { + pc_is_ra = false; continue; } @@ -639,8 +668,11 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex sp = (uintptr_t)INJECT_FAULT_ADDRESS_UNLIKELY(sp); fp = ((uintptr_t*)sp)[-FRAME_PC_SLOT - 1]; pc = ((const void**)sp)[-FRAME_PC_SLOT]; + // Saved return address of the caller frame. + pc_is_ra = true; continue; } else if (frame.unwindPrologue(nm, (uintptr_t&)pc, sp, fp)) { + pc_is_ra = false; continue; } @@ -658,6 +690,8 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex // End of Java stack break; } + // getFrame() redirects pc to lastJavaPC(), a return address. + pc_is_ra = true; if (sp < prev_sp || sp >= bottom || !aligned(sp)) { fillFrame(frames[depth++], BCI_ERROR, "break_entry_frame"); break; @@ -692,6 +726,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } if (frame.unwindStub((instruction_t*)start, name, (uintptr_t&)pc, sp, fp)) { + pc_is_ra = false; continue; } @@ -714,6 +749,8 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex fp = ((uintptr_t*)sp)[-FRAME_PC_SLOT - 1]; pc = ((const void**)sp)[-FRAME_PC_SLOT]; + // Saved return address of the caller frame. + pc_is_ra = true; continue; } @@ -732,7 +769,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } } else { // Resolve native frame (may use remote symbolication if enabled) - Profiler::NativeFrameResolution resolution = profiler->resolveNativeFrameForWalkVM((uintptr_t)pc, lock_index); + Profiler::NativeFrameResolution resolution = profiler->resolveNativeFrameForWalkVM((uintptr_t)pc, pc_is_ra, lock_index); if (resolution.is_marked()) { if (resolution.mark == MARK_JAVA_PROFILER && isHookPrefixedSample(event_type)) { @@ -764,7 +801,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex const char* method_name = resolution.method_name; int frame_bci = resolution.bci; if (method_name == NULL && details && !anchor_recovery_used - && profiler->findLibraryByAddress(pc) == NULL) { + && profiler->findLibraryByAddress(attributionPC(pc, pc_is_ra)) == NULL) { // Try anchor recovery — prefer live anchor, fall back to saved data anchor_recovery_used = true; const void* recovery_pc = NULL; @@ -811,6 +848,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex HotspotSupport::fillJavaFrame(frames[depth++], FRAME_INTERPRETED, bci, method_id, method); sp = ((uintptr_t*)recovery_fp)[InterpreterFrame::sender_sp_offset]; pc = stripPointer(((void**)recovery_fp)[FRAME_PC_SLOT]); + pc_is_ra = true; fp = *(uintptr_t*)recovery_fp; continue; } @@ -820,6 +858,9 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex sp = recovery_sp; fp = recovery_fp; pc = recovery_pc; + // lastJavaPC() records where the Java frame resumes, i.e. a + // return address; so is the sp[-1] slot read just below. + pc_is_ra = true; if (pc != NULL && !CodeHeap::contains(pc) && sp != 0 && aligned(sp) && sp < bottom) { pc = ((const void**)sp)[-1]; } @@ -842,7 +883,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex // Only check marks for traditionally-resolved frames; packed remote // frames store an integer in the method_name union, not a valid pointer. if (prev_native_pc != NULL) { - Profiler::NativeFrameResolution prev_resolution = profiler->resolveNativeFrameForWalkVM((uintptr_t)prev_native_pc, lock_index); + Profiler::NativeFrameResolution prev_resolution = profiler->resolveNativeFrameForWalkVM((uintptr_t)prev_native_pc, prev_native_pc_is_ra, lock_index); if (prev_resolution.bci != BCI_NATIVE_FRAME_REMOTE) { const char* prev_method_name = prev_resolution.method_name; if (prev_method_name != NULL) { @@ -868,17 +909,16 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } dwarf_unwind: - // Known defect, deliberately not fixed here: past the leaf, `pc` is a - // return address for exactly the same reason it is in - // StackWalker::walkDwarf, so selecting a row or a symbol with it - // unadjusted misattributes a call that is the last instruction of its - // caller -- wrong CFA row, wrong sender sp, and a MARK_THREAD_ENTRY - // check that can miss its mark. The same -1 adjustment applies; it is - // deferred because walkVM interleaves Java and native frames and needs - // its own per-frame return-address tracking and its own tests. + // Past the leaf, `pc` is usually a return address, so row and symbol + // selection go through the attribution address: a call that is the last + // instruction of its caller would otherwise select the following + // function's CFA row, derive a sender sp from it, and miss a + // MARK_THREAD_ENTRY sitting on the caller. The raw pc is still what the + // DW_PC_OFFSET arithmetic and the no-progress guard below operate on. uintptr_t prev_sp = sp; - CodeCache* cc = profiler->findLibraryByAddress(pc); - FrameDesc f = cc != NULL ? cc->findFrameDesc(pc) : FrameDesc::fallback_default_frame(); + const void* attribution_pc = attributionPC(pc, pc_is_ra); + CodeCache* cc = profiler->findLibraryByAddress(attribution_pc); + FrameDesc f = cc != NULL ? cc->findFrameDesc(attribution_pc) : FrameDesc::fallback_default_frame(); u8 cfa_reg = (u8)f.cfa; int cfa_off = f.cfa >> 8; @@ -901,7 +941,9 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } sp = fp + cfa_off; } else if (cfa_reg == DW_REG_PLT) { - sp += ((uintptr_t)pc & 15) >= 11 ? cfa_off * 2 : cfa_off; + // Tested on the address the row was selected with, so the stub + // offset and the CFA doubling cannot be decided on different pcs. + sp += ((uintptr_t)attribution_pc & 15) >= 11 ? cfa_off * 2 : cfa_off; } // Check if the next frame is below on the current stack @@ -916,8 +958,14 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex // store the previous pc before unwinding prev_native_pc = pc; + prev_native_pc_is_ra = pc_is_ra; if (f.fp_off & DW_PC_OFFSET) { + // DW_OP_breg names the value of the pc *register*, i.e. the raw + // walking pc, so the offset applies to that and not to the lookup + // address. A signal-frame CIE declares its return-address column to + // hold the exact interrupted pc, which must not be adjusted again. pc = (const char*)pc + (f.fp_off >> 1); + pc_is_ra = !f.isSignalFrame(); } else { if (f.fp_off != DW_SAME_FP && f.fp_off < MAX_FRAME_SIZE && f.fp_off > -MAX_FRAME_SIZE) { fp = (uintptr_t)SafeAccess::load((void**)(sp + f.fp_off)); @@ -930,8 +978,10 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex break; } pc = stripPointer(SafeAccess::load((void**)pc_addr)); + pc_is_ra = !f.isSignalFrame(); } else if (depth == 1) { pc = (const void*)frame.link(); + pc_is_ra = !f.isSignalFrame(); } else { break; } @@ -975,6 +1025,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex HotspotSupport::fillJavaFrame(frames[depth++], FRAME_INTERPRETED, bci, method_id, method); sp = ((uintptr_t*)anchor_fp)[InterpreterFrame::sender_sp_offset]; pc = stripPointer(((void**)anchor_fp)[FRAME_PC_SLOT]); + pc_is_ra = true; fp = *(uintptr_t*)anchor_fp; if (sp != 0 && sp < bottom && aligned(sp)) { goto unwind_loop; @@ -984,6 +1035,8 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } // Fallback: redirect via anchor frame and sp[-1] if (anchor != NULL && anchor->getFrame(pc, sp, fp)) { + // Both the anchor's lastJavaPC() and the sp[-1] slot are return addresses. + pc_is_ra = true; if (!CodeHeap::contains(pc) && sp != 0 && aligned(sp) && sp < bottom) { pc = ((const void**)sp)[-1]; } diff --git a/ddprof-lib/src/main/cpp/profiler.cpp b/ddprof-lib/src/main/cpp/profiler.cpp index a3cf0aaf4f..1dd9263183 100644 --- a/ddprof-lib/src/main/cpp/profiler.cpp +++ b/ddprof-lib/src/main/cpp/profiler.cpp @@ -35,6 +35,7 @@ #include "samplerPerf.h" #include "stackFrame.h" #include "stackWalker.h" +#include "stackWalker.inline.h" #include "symbols.h" #include "threadLocalData.inline.h" #include "tsc.h" @@ -388,14 +389,21 @@ void Profiler::populateRemoteFrame(ASGCT_CallFrame* frame, uintptr_t pc, CodeCac * - Checks marks after symbol resolution (same O(log n) + O(1) cost) * - If no symbol found but PC is in a known library, packs as * BCI_NATIVE_FRAME_REMOTE for library-relative rendering ([lib+0xoffset]) + * + * 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. */ -Profiler::NativeFrameResolution Profiler::resolveNativeFrameForWalkVM(uintptr_t pc, int lock_index) { - CodeCache* lib = _libs->findLibraryByAddress((void*)pc); +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); + CodeCache* lib = _libs->findLibraryByAddress(lookup_pc); if (_remote_symbolication && lib != nullptr && lib->hasBuildId()) { // Get symbol name and check mark const char *method_name = nullptr; - lib->binarySearch((void*)pc, &method_name); + lib->binarySearch(lookup_pc, &method_name); char mark = (method_name != nullptr) ? NativeFunc::read_mark(method_name) : 0; if (mark != 0) { @@ -413,7 +421,7 @@ Profiler::NativeFrameResolution Profiler::resolveNativeFrameForWalkVM(uintptr_t // Traditional symbol resolution const char *method_name = nullptr; if (lib != nullptr) { - lib->binarySearch((void*)pc, &method_name); + lib->binarySearch(lookup_pc, &method_name); } if (method_name != nullptr) { char mark = NativeFunc::read_mark(method_name); diff --git a/ddprof-lib/src/main/cpp/profiler.h b/ddprof-lib/src/main/cpp/profiler.h index 7227960359..2d3d0bd09e 100644 --- a/ddprof-lib/src/main/cpp/profiler.h +++ b/ddprof-lib/src/main/cpp/profiler.h @@ -437,7 +437,7 @@ class alignas(alignof(SpinLock)) Profiler { }; void populateRemoteFrame(ASGCT_CallFrame* frame, uintptr_t pc, CodeCache* lib, char mark); - NativeFrameResolution resolveNativeFrameForWalkVM(uintptr_t pc, int lock_index); + NativeFrameResolution resolveNativeFrameForWalkVM(uintptr_t pc, bool pc_is_return_address, int lock_index); int convertNativeTrace(int native_frames, const void **callchain, ASGCT_CallFrame *frames, int lock_index, bool skip_hook_prefix); From af831868fb646b3b71a5acfdfca4c3ed4276c30c Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Thu, 24 Sep 2026 12:57:23 +0200 Subject: [PATCH 2/7] test(profiling): gate the lookup/wire-format address split in walkVM resolution resolveNativeFrameForWalkVM uses two addresses for two jobs: range lookups key off the attribution address so a call that is the last instruction of its caller still resolves to the caller, while the emitted pc_offset keeps deriving from the raw pc because its meaning is a wire contract rather than a local detail. Neither half was gated by a test. Both are now pinned at a synthetic zero-gap symbol boundary, the shape where the two addresses disagree: flagging the pc as a return address must move the resolved symbol from the following function back to the caller, and must leave the emitted offset untouched. A third case pins that an address well inside a function resolves identically either way, so the adjustment cannot be read as a blanket shift. Both assertions were mutation-checked -- reverting the lookup to the raw pc fails the boundary case alone, and deriving pc_offset from the attribution address fails the wire case alone. The fixtures publish synthetic CodeCaches through a new test-only entry point rather than the test binary's own symbols, which is what makes a controlled zero-gap boundary possible at all and keeps these tests free of the GNU-as/ELF-CFI and updateSymbols() dependencies that confine returnAddressAttribution_ut to Linux. Co-Authored-By: Claude Opus 5 (1M context) --- ddprof-lib/src/main/cpp/libraries.h | 6 + .../src/test/cpp/walkVmAttribution_ut.cpp | 126 ++++++++++++++++++ 2 files changed, 132 insertions(+) create mode 100644 ddprof-lib/src/test/cpp/walkVmAttribution_ut.cpp diff --git a/ddprof-lib/src/main/cpp/libraries.h b/ddprof-lib/src/main/cpp/libraries.h index 18b59c9631..efe9b54337 100644 --- a/ddprof-lib/src/main/cpp/libraries.h +++ b/ddprof-lib/src/main/cpp/libraries.h @@ -82,6 +82,12 @@ class Libraries { return _native_libs; } + // Publishes a caller-owned CodeCache into the process-wide set so address + // lookups resolve against it. Test-only: CodeCacheArray is append-only, so + // the cache has to outlive every later lookup, and the usual population + // path is updateSymbols() reading the real loaded libraries. + bool addLibraryForTest(CodeCache *lib) { return _native_libs.add(lib); } + // Delete copy constructor and assignment operator to prevent copies Libraries(const Libraries&) = delete; Libraries& operator=(const Libraries&) = delete; diff --git a/ddprof-lib/src/test/cpp/walkVmAttribution_ut.cpp b/ddprof-lib/src/test/cpp/walkVmAttribution_ut.cpp new file mode 100644 index 0000000000..03ff788d8c --- /dev/null +++ b/ddprof-lib/src/test/cpp/walkVmAttribution_ut.cpp @@ -0,0 +1,126 @@ +/* + * Copyright 2026, Datadog, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +// Gates the address split in Profiler::resolveNativeFrameForWalkVM, which +// 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. +// +// 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 +// and no reliance on updateSymbols() parsing the main executable -- they run +// on every platform this repo builds. + +#include +#include +#include "codeCache.h" +#include "libraries.h" +#include "profiler.h" +#include "vmEntry.h" + +namespace { + +// Far above anything the loader maps, so these never overlap a real library +// in the process-wide set they are published into. +const char* const kSymbolLibBase = (const char*)0x5a5a00000000ULL; +const char* const kBareLibBase = (const char*)0x5a5b00000000ULL; +const size_t kLibSpan = 0x1000; + +// Zero-gap pair: `second` starts on the byte immediately after `first` ends, +// which is exactly where a return address lands when the call is the last +// instruction of `first`. +const int kFirstOff = 0x100; +const int kFuncLen = 0x10; +const int kBoundary = kFirstOff + kFuncLen; + +// No symbol covers this, so resolution falls through to the library-relative +// packing path that carries pc_offset. +const int kUnnamedOff = 0x800; + +class WalkVmAttributionTest : public ::testing::Test { + protected: + static void SetUpTestSuite() { + // CodeCacheArray is append-only and the set is process-wide, so both + // caches are published once and outlive every test in this binary. + static CodeCache symbol_lib("walkvm_attr_symbols", /*lib_index=*/-1, + kSymbolLibBase, kSymbolLibBase + kLibSpan, + /*image_base=*/kSymbolLibBase); + symbol_lib.add(kSymbolLibBase + kFirstOff, kFuncLen, "walkvm_attr_first"); + symbol_lib.add(kSymbolLibBase + kBoundary, kFuncLen, "walkvm_attr_second"); + symbol_lib.sort(); + + static CodeCache bare_lib("walkvm_attr_nosymbols", /*lib_index=*/-1, + kBareLibBase, kBareLibBase + kLibSpan, + /*image_base=*/kBareLibBase); + + ASSERT_TRUE(Libraries::instance()->addLibraryForTest(&symbol_lib)); + ASSERT_TRUE(Libraries::instance()->addLibraryForTest(&bare_lib)); + } + + static Profiler::NativeFrameResolution resolve(const char* pc, bool pc_is_ra) { + return Profiler::instance()->resolveNativeFrameForWalkVM( + (uintptr_t)pc, pc_is_ra, /*lock_index=*/0); + } +}; + +// The defect this fix addresses: at a zero-gap boundary the raw return +// address names the following function, and only the attribution address +// still names the caller that actually made the call. +TEST_F(WalkVmAttributionTest, ReturnAddressAtZeroGapBoundaryResolvesToTheCaller) { + const char* boundary = kSymbolLibBase + kBoundary; + + Profiler::NativeFrameResolution exact = resolve(boundary, /*pc_is_ra=*/false); + ASSERT_EQ(BCI_NATIVE_FRAME, exact.bci); + ASSERT_NE(nullptr, exact.method_name); + EXPECT_STREQ("walkvm_attr_second", exact.method_name) + << "an exact pc on the first byte of a function resolves to that function"; + + Profiler::NativeFrameResolution as_ra = resolve(boundary, /*pc_is_ra=*/true); + ASSERT_EQ(BCI_NATIVE_FRAME, as_ra.bci); + ASSERT_NE(nullptr, as_ra.method_name); + EXPECT_STREQ("walkvm_attr_first", as_ra.method_name) + << "the same address flagged as a return address must resolve to the caller, " + << "not to whatever happens to follow it"; +} + +// Inside a function the adjustment must be invisible -- it only ever moves the +// lookup by one byte, so it may not reclassify a pc that is nowhere near a +// boundary. +TEST_F(WalkVmAttributionTest, AddressInsideAFunctionResolvesTheSameEitherWay) { + const char* inside = kSymbolLibBase + kFirstOff + kFuncLen / 2; + + Profiler::NativeFrameResolution exact = resolve(inside, /*pc_is_ra=*/false); + Profiler::NativeFrameResolution as_ra = resolve(inside, /*pc_is_ra=*/true); + + ASSERT_NE(nullptr, exact.method_name); + ASSERT_NE(nullptr, as_ra.method_name); + EXPECT_STREQ("walkvm_attr_first", exact.method_name); + 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) { + const char* pc = kBareLibBase + kUnnamedOff; + + Profiler::NativeFrameResolution exact = resolve(pc, /*pc_is_ra=*/false); + Profiler::NativeFrameResolution as_ra = resolve(pc, /*pc_is_ra=*/true); + + ASSERT_EQ(BCI_NATIVE_FRAME_REMOTE, exact.bci); + ASSERT_EQ(BCI_NATIVE_FRAME_REMOTE, as_ra.bci); + + 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"; +} + +} // namespace From 2fc332696eb5502162065eacd352526fa0b18d5a Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Thu, 24 Sep 2026 14:16:08 +0200 Subject: [PATCH 3/7] refactor(profiling): carry walkVM's pc and its nature as one value walkVM tracked "is this pc a return address?" in a bool sitting beside the pc, so the two could drift: a newly added `pc = ...` inherited whatever the previous frame had set, silently attributing the next lookup to the wrong address. That is the exact hazard WalkPc was introduced for in walkFP/walkDwarf, and walkVM is the default cstack path, so it is the one that most needed it. walkVM now drives a WalkPc. Every pc source states what it produced -- setReturnAddress for a saved-pc slot or link register, setRecoveredPc for a DWARF row, setExactAddress where the arch unwind helpers already folded the adjustment in -- and consumers read raw() or attribution() explicitly. Adding a pc source is now a compile-time decision instead of something a reviewer has to notice. Threading the seed through surfaced a real gap: walkVM is entered either from a ucontext, whose pc is the exact interrupted address, or from callerPC(), which is a genuine return address on every architecture where CALLER_PC_IS_RETURN_ADDRESS holds. Both arrived as a bare pointer and were treated as exact, so the callerPC() entry never got the adjustment it needed. The private overload now takes that distinction from its caller, the same way walkFP and walkDwarf seed themselves. No behaviour change otherwise: the mutator chosen at each site reproduces the flag that site already set. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/main/cpp/hotspot/hotspotSupport.cpp | 170 +++++++++--------- .../src/main/cpp/hotspot/hotspotSupport.h | 6 +- ddprof-lib/src/main/cpp/stackWalker.inline.h | 5 + 3 files changed, 96 insertions(+), 85 deletions(-) diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index 1cb499ef76..29fa8ab0e4 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -238,20 +238,25 @@ static const bool CONT_UNWIND_DISABLED = (std::getenv("DDPROF_DISABLE_CONT_UNWIN __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, StackWalkFeatures features, EventType event_type, int lock_index, bool* truncated) { if (ucontext == NULL) { + // callerPC()'s nature is per-arch, the same distinction walkFP/walkDwarf + // seed themselves with. return walkVM(&empty_ucontext, frames, max_depth, features, event_type, - callerPC(), (uintptr_t)callerSP(), (uintptr_t)callerFP(), lock_index, truncated); + callerPC(), CALLER_PC_IS_RETURN_ADDRESS, + (uintptr_t)callerSP(), (uintptr_t)callerFP(), lock_index, truncated); } else { HotspotStackFrame frame(ucontext); return walkVM(ucontext, frames, max_depth, features, event_type, - (const void*)frame.pc(), frame.sp(), frame.fp(), lock_index, truncated); + (const void*)frame.pc(), /*pc_is_return_address=*/false, + frame.sp(), frame.fp(), lock_index, truncated); } } __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, StackWalkFeatures features, EventType event_type, - const void* pc, uintptr_t sp, uintptr_t fp, int lock_index, bool* truncated) { + const void* entry_pc, bool pc_is_return_address, + uintptr_t sp, uintptr_t fp, int lock_index, bool* truncated) { - // VMStructs is only available for hotspot JVM + // VMStructs is only available for hotspot JVM assert(VM::isHotspot()); ProfiledThread* prof_thread = ProfiledThread::acquireCurrent(); @@ -326,24 +331,22 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex const void* prev_native_pc = NULL; - // Whether `pc` currently holds an address loaded from a return-address slot - // (a saved pc or a link register), which points at the instruction *after* a - // call. Range-based lookups -- findLibraryByAddress, findFrameDesc, the - // DW_REG_PLT stub-offset test -- key off attributionPC() instead, otherwise a - // call that is the last instruction of its caller selects whatever follows - // the caller. Exact-address consumers (isContReturnBarrier, - // isContEntryReturnPc, isEntryFrame), the DW_PC_OFFSET arithmetic and the - // no-progress guard keep using the raw pc. The entry pc comes from the - // ucontext, so it is an exact interrupted address. - // - // unwindPrologue/unwindEpilogue/unwindStub are deliberately excluded: on - // x86_64 they already fold the adjustment into the pc they return, and not - // even uniformly (unwindPrologue's isFrameComplete branch omits it), while on - // aarch64 they return the return address as-is. Flagging their results as - // non-return-addresses keeps this change from double-adjusting them; making - // that contract explicit is tracked separately. - bool pc_is_ra = false; - bool prev_native_pc_is_ra = false; + // The walking pc and whether it came out of a return-address slot travel + // together, so a newly added pc source has to say which it is instead of + // inheriting whatever the previous frame happened to set. Range-based + // lookups -- findLibraryByAddress, findFrameDesc, the DW_REG_PLT + // stub-offset test -- go through attribution(), otherwise a call that is + // the last instruction of its caller selects whatever follows the caller. + // Exact-address consumers (isContReturnBarrier, isContEntryReturnPc, + // isEntryFrame), the DW_PC_OFFSET arithmetic and the no-progress guard + // read raw(). + WalkPc walk_pc; + walk_pc.setSeed(entry_pc, pc_is_return_address); + + // The previous frame's pc, kept for the MARK_THREAD_ENTRY check, with the + // same distinction attached. + WalkPc prev_native_walk_pc; + bool have_prev_native_pc = false; // Last ContinuationEntry crossed; advanced via parent() for nested continuations. VMContinuationEntry* cont_entry = nullptr; @@ -441,9 +444,8 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } sp = carrier_sp; fp = carrier_fp; - pc = carrier_pc; // Read out of the carrier frame's saved-pc slot. - pc_is_ra = true; + walk_pc.setReturnAddress(carrier_pc); return true; }; @@ -451,7 +453,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex // Walk until the bottom of the stack or until the first Java frame while (depth < actual_max_depth) { - if (CodeHeap::contains(pc)) { + if (CodeHeap::contains(walk_pc.raw())) { Counters::increment(WALKVM_HIT_CODEHEAP); if (fp_chain_fallback) { Counters::increment(WALKVM_FP_CHAIN_REACHED_CODEHEAP); @@ -476,7 +478,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex fillFrame(frames[depth++], BCI_ERROR, "break_no_vmthread"); break; } - prev_native_pc = NULL; // we are in JVM code, no previous 'native' PC + have_prev_native_pc = false; // we are in JVM code, no previous 'native' PC // Both continuation boundary PCs are JVM stubs whose findNMethod() // returns NULL; detect them by exact-PC match before the nmethod // dispatch below. @@ -484,17 +486,17 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex // frames remain in the StackChunk (blocking/remounted VT). // cont_entry_return_pc: bottom thawed frame returns here when the // continuation is fully thawed (CPU-bound VT, never yielded). - if (!CONT_UNWIND_DISABLED && VMStructs::isContReturnBarrier(pc)) { + if (!CONT_UNWIND_DISABLED && VMStructs::isContReturnBarrier(walk_pc.raw())) { Counters::increment(WALKVM_CONT_BARRIER_HIT); if (walkThroughContinuation(false)) continue; break; } - if (!CONT_UNWIND_DISABLED && VMStructs::isContEntryReturnPc(pc)) { + if (!CONT_UNWIND_DISABLED && VMStructs::isContEntryReturnPc(walk_pc.raw())) { Counters::increment(WALKVM_ENTER_SPECIAL_HIT); if (walkThroughContinuation(true)) continue; break; } - VMNMethod* nm = CodeHeap::findNMethod(pc); + VMNMethod* nm = CodeHeap::findNMethod(walk_pc.raw()); if (nm == NULL) { // On JDK 21+ builds, the continuation entry PC may be absent // from vmStructs OR resolved but pointing to the wrong address @@ -539,10 +541,11 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex saved_anchor_sp = anchor->lastJavaSP(); saved_anchor_fp = anchor->lastJavaFP(); } - if (anchor->getFrame(pc, sp, fp)) { + const void* anchor_pc = walk_pc.raw(); + if (anchor->getFrame(anchor_pc, sp, fp)) { // getFrame() redirects pc to lastJavaPC(), a return address. - pc_is_ra = true; - if (!nm->contains(pc)) { + walk_pc.setReturnAddress(anchor_pc); + if (!nm->contains(walk_pc.raw())) { anchor = NULL; continue; // NMethod has changed as a result of correction } @@ -571,8 +574,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex int bci = bytecode_start == NULL || bcp < bytecode_start ? 0 : bcp - bytecode_start; HotspotSupport::fillJavaFrame(frames[depth++], FRAME_INTERPRETED, bci, method_id, method); sp = ((uintptr_t*)fp)[InterpreterFrame::sender_sp_offset]; - pc = stripPointer(((void**)fp)[FRAME_PC_SLOT]); - pc_is_ra = true; + walk_pc.setReturnAddress(stripPointer(((void**)fp)[FRAME_PC_SLOT])); fp = *(uintptr_t*)INJECT_FAULT_ADDRESS_UNLIKELY(fp); continue; } @@ -586,15 +588,13 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex HotspotSupport::fillJavaFrame(frames[depth++], FRAME_INTERPRETED, 0, method_id, method); if (is_plausible_interpreter_frame) { uintptr_t* fp_addr = (uintptr_t*)INJECT_FAULT_ADDRESS_UNLIKELY(fp); - pc = stripPointer(((void**)fp_addr)[FRAME_PC_SLOT]); + walk_pc.setReturnAddress(stripPointer(((void**)fp_addr)[FRAME_PC_SLOT])); sp = frame.senderSP(); fp = *fp_addr; } else { - pc = stripPointer(SafeAccess::load((void**)sp)); + walk_pc.setReturnAddress(stripPointer(SafeAccess::load((void**)sp))); sp = frame.senderSP(); } - // Both arms read the sender pc out of a return-address slot. - pc_is_ra = true; continue; } } @@ -626,13 +626,14 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex jmethodID method_id = method->id(); HotspotSupport::fillJavaFrame(frames[depth++], type, 0, method_id, method); - if (nm->isFrameCompleteAt(pc)) { - if (depth == 1 && frame.unwindEpilogue(nm, (uintptr_t&)pc, sp, fp)) { - pc_is_ra = false; + if (nm->isFrameCompleteAt(walk_pc.raw())) { + const void* epilogue_pc = walk_pc.raw(); + if (depth == 1 && frame.unwindEpilogue(nm, (uintptr_t&)epilogue_pc, sp, fp)) { + walk_pc.setExactAddress(epilogue_pc); continue; } - int scope_offset = nm->findScopeOffset(pc); + int scope_offset = nm->findScopeOffset(walk_pc.raw()); if (scope_offset > 0) { depth--; ScopeDesc scope(nm); @@ -649,7 +650,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } // Handle situations when sp is temporarily changed in the compiled code - frame.adjustSP(nm->entry(), pc, sp); + frame.adjustSP(nm->entry(), walk_pc.raw(), sp); // Validate NMethod metadata before using frameSize() int frame_size = nm->frameSize(); @@ -667,31 +668,32 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } sp = (uintptr_t)INJECT_FAULT_ADDRESS_UNLIKELY(sp); fp = ((uintptr_t*)sp)[-FRAME_PC_SLOT - 1]; - pc = ((const void**)sp)[-FRAME_PC_SLOT]; // Saved return address of the caller frame. - pc_is_ra = true; + walk_pc.setReturnAddress(((const void**)sp)[-FRAME_PC_SLOT]); continue; - } else if (frame.unwindPrologue(nm, (uintptr_t&)pc, sp, fp)) { - pc_is_ra = false; + } else if (const void* prologue_pc = walk_pc.raw(); + frame.unwindPrologue(nm, (uintptr_t&)prologue_pc, sp, fp)) { + walk_pc.setExactAddress(prologue_pc); continue; } Counters::increment(WALKVM_BREAK_COMPILED); fillFrame(frames[depth++], BCI_ERROR, "break_compiled"); break; - } else if (nm->isEntryFrame(pc) && !features.mixed) { + } else if (nm->isEntryFrame(walk_pc.raw()) && !features.mixed) { VMJavaFrameAnchor* next_anchor = VMJavaFrameAnchor::fromEntryFrame(fp); if (next_anchor == NULL) { fillFrame(frames[depth++], BCI_ERROR, "break_entry_frame"); break; } uintptr_t prev_sp = sp; - if (!next_anchor->getFrame(pc, sp, fp)) { + const void* entry_frame_pc = walk_pc.raw(); + if (!next_anchor->getFrame(entry_frame_pc, sp, fp)) { // End of Java stack break; } // getFrame() redirects pc to lastJavaPC(), a return address. - pc_is_ra = true; + walk_pc.setReturnAddress(entry_frame_pc); if (sp < prev_sp || sp >= bottom || !aligned(sp)) { fillFrame(frames[depth++], BCI_ERROR, "break_entry_frame"); break; @@ -717,7 +719,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } } - CodeBlob* stub = JitCodeCache::findRuntimeStub(pc); + CodeBlob* stub = JitCodeCache::findRuntimeStub(walk_pc.raw()); const void* start = stub != NULL ? stub->_start : nm->code(); const char* name = stub != NULL ? stub->_name : nm->name(); @@ -725,8 +727,9 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex fillFrame(frames[depth++], BCI_NATIVE_FRAME, name); } - if (frame.unwindStub((instruction_t*)start, name, (uintptr_t&)pc, sp, fp)) { - pc_is_ra = false; + const void* stub_pc = walk_pc.raw(); + if (frame.unwindStub((instruction_t*)start, name, (uintptr_t&)stub_pc, sp, fp)) { + walk_pc.setExactAddress(stub_pc); continue; } @@ -748,9 +751,8 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } fp = ((uintptr_t*)sp)[-FRAME_PC_SLOT - 1]; - pc = ((const void**)sp)[-FRAME_PC_SLOT]; // Saved return address of the caller frame. - pc_is_ra = true; + walk_pc.setReturnAddress(((const void**)sp)[-FRAME_PC_SLOT]); continue; } @@ -769,7 +771,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } } else { // Resolve native frame (may use remote symbolication if enabled) - Profiler::NativeFrameResolution resolution = profiler->resolveNativeFrameForWalkVM((uintptr_t)pc, pc_is_ra, lock_index); + Profiler::NativeFrameResolution resolution = profiler->resolveNativeFrameForWalkVM((uintptr_t)walk_pc.raw(), walk_pc.isReturnAddress(), lock_index); if (resolution.is_marked()) { if (resolution.mark == MARK_JAVA_PROFILER && isHookPrefixedSample(event_type)) { @@ -801,7 +803,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex const char* method_name = resolution.method_name; int frame_bci = resolution.bci; if (method_name == NULL && details && !anchor_recovery_used - && profiler->findLibraryByAddress(attributionPC(pc, pc_is_ra)) == NULL) { + && profiler->findLibraryByAddress(walk_pc.attribution()) == NULL) { // Try anchor recovery — prefer live anchor, fall back to saved data anchor_recovery_used = true; const void* recovery_pc = NULL; @@ -837,7 +839,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex jmethodID method_id = getMethodId(method); if (method_id != JMETHODID_NOT_WALKABLE) { anchor = NULL; - prev_native_pc = NULL; + have_prev_native_pc = false; if (depth > 0 && depth + 1 < actual_max_depth) { fillFrame(frames[depth++], BCI_ERROR, "[skipped frames]"); } @@ -847,8 +849,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex int bci = bytecode_start == NULL || bcp < bytecode_start ? 0 : bcp - bytecode_start; HotspotSupport::fillJavaFrame(frames[depth++], FRAME_INTERPRETED, bci, method_id, method); sp = ((uintptr_t*)recovery_fp)[InterpreterFrame::sender_sp_offset]; - pc = stripPointer(((void**)recovery_fp)[FRAME_PC_SLOT]); - pc_is_ra = true; + walk_pc.setReturnAddress(stripPointer(((void**)recovery_fp)[FRAME_PC_SLOT])); fp = *(uintptr_t*)recovery_fp; continue; } @@ -857,21 +858,21 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex // Fallback: redirect via recovery SP/FP/PC sp = recovery_sp; fp = recovery_fp; - pc = recovery_pc; // lastJavaPC() records where the Java frame resumes, i.e. a // return address; so is the sp[-1] slot read just below. - pc_is_ra = true; - if (pc != NULL && !CodeHeap::contains(pc) && sp != 0 && aligned(sp) && sp < bottom) { - pc = ((const void**)sp)[-1]; + walk_pc.setReturnAddress(recovery_pc); + if (walk_pc.raw() != NULL && !CodeHeap::contains(walk_pc.raw()) + && sp != 0 && aligned(sp) && sp < bottom) { + walk_pc.setReturnAddress(((const void**)sp)[-1]); } - if (sp != 0 && pc != NULL) { + if (sp != 0 && walk_pc.raw() != NULL) { anchor = NULL; if (sp >= bottom || !aligned(sp)) { Counters::increment(WALKVM_ANCHOR_INLINE_BAD_SP); fillFrame(frames[depth++], BCI_ERROR, "break_no_anchor"); break; } - prev_native_pc = NULL; + have_prev_native_pc = false; if (depth > 0) { fillFrame(frames[depth++], BCI_ERROR, "[skipped frames]"); } @@ -882,8 +883,9 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex // Check previous frame for thread entry points (Rust, libc/pthread) // Only check marks for traditionally-resolved frames; packed remote // frames store an integer in the method_name union, not a valid pointer. - if (prev_native_pc != NULL) { - Profiler::NativeFrameResolution prev_resolution = profiler->resolveNativeFrameForWalkVM((uintptr_t)prev_native_pc, prev_native_pc_is_ra, lock_index); + if (have_prev_native_pc) { + Profiler::NativeFrameResolution prev_resolution = profiler->resolveNativeFrameForWalkVM( + (uintptr_t)prev_native_walk_pc.raw(), prev_native_walk_pc.isReturnAddress(), lock_index); if (prev_resolution.bci != BCI_NATIVE_FRAME_REMOTE) { const char* prev_method_name = prev_resolution.method_name; if (prev_method_name != NULL) { @@ -916,7 +918,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex // MARK_THREAD_ENTRY sitting on the caller. The raw pc is still what the // DW_PC_OFFSET arithmetic and the no-progress guard below operate on. uintptr_t prev_sp = sp; - const void* attribution_pc = attributionPC(pc, pc_is_ra); + const void* attribution_pc = walk_pc.attribution(); CodeCache* cc = profiler->findLibraryByAddress(attribution_pc); FrameDesc f = cc != NULL ? cc->findFrameDesc(attribution_pc) : FrameDesc::fallback_default_frame(); @@ -957,15 +959,15 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } // store the previous pc before unwinding - prev_native_pc = pc; - prev_native_pc_is_ra = pc_is_ra; + prev_native_walk_pc = walk_pc; + have_prev_native_pc = true; if (f.fp_off & DW_PC_OFFSET) { // DW_OP_breg names the value of the pc *register*, i.e. the raw // walking pc, so the offset applies to that and not to the lookup // address. A signal-frame CIE declares its return-address column to // hold the exact interrupted pc, which must not be adjusted again. - pc = (const char*)pc + (f.fp_off >> 1); - pc_is_ra = !f.isSignalFrame(); + walk_pc.setRecoveredPc((const char*)walk_pc.raw() + (f.fp_off >> 1), + f.isSignalFrame()); } else { if (f.fp_off != DW_SAME_FP && f.fp_off < MAX_FRAME_SIZE && f.fp_off > -MAX_FRAME_SIZE) { fp = (uintptr_t)SafeAccess::load((void**)(sp + f.fp_off)); @@ -977,11 +979,10 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex if (!aligned(pc_addr)) { break; } - pc = stripPointer(SafeAccess::load((void**)pc_addr)); - pc_is_ra = !f.isSignalFrame(); + walk_pc.setRecoveredPc(stripPointer(SafeAccess::load((void**)pc_addr)), + f.isSignalFrame()); } else if (depth == 1) { - pc = (const void*)frame.link(); - pc_is_ra = !f.isSignalFrame(); + walk_pc.setRecoveredPc((const void*)frame.link(), f.isSignalFrame()); } else { break; } @@ -995,7 +996,8 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } } - if (inDeadZone(pc) || (pc == prev_native_pc && sp == prev_sp)) { + if (inDeadZone(walk_pc.raw()) + || (walk_pc.raw() == prev_native_walk_pc.raw() && sp == prev_sp)) { break; } } @@ -1024,8 +1026,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex int bci = bytecode_start == NULL || bcp < bytecode_start ? 0 : bcp - bytecode_start; HotspotSupport::fillJavaFrame(frames[depth++], FRAME_INTERPRETED, bci, method_id, method); sp = ((uintptr_t*)anchor_fp)[InterpreterFrame::sender_sp_offset]; - pc = stripPointer(((void**)anchor_fp)[FRAME_PC_SLOT]); - pc_is_ra = true; + walk_pc.setReturnAddress(stripPointer(((void**)anchor_fp)[FRAME_PC_SLOT])); fp = *(uintptr_t*)anchor_fp; if (sp != 0 && sp < bottom && aligned(sp)) { goto unwind_loop; @@ -1034,11 +1035,12 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } } // Fallback: redirect via anchor frame and sp[-1] - if (anchor != NULL && anchor->getFrame(pc, sp, fp)) { + const void* fallback_pc = walk_pc.raw(); + if (anchor != NULL && anchor->getFrame(fallback_pc, sp, fp)) { // Both the anchor's lastJavaPC() and the sp[-1] slot are return addresses. - pc_is_ra = true; - if (!CodeHeap::contains(pc) && sp != 0 && aligned(sp) && sp < bottom) { - pc = ((const void**)sp)[-1]; + walk_pc.setReturnAddress(fallback_pc); + if (!CodeHeap::contains(walk_pc.raw()) && sp != 0 && aligned(sp) && sp < bottom) { + walk_pc.setReturnAddress(((const void**)sp)[-1]); } Counters::increment(WALKVM_ANCHOR_FALLBACK); anchor = NULL; diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h index 4c52d70219..8588635dfc 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h @@ -25,9 +25,13 @@ class HotspotSupport { friend class HotspotSupportTestAccessor; private: + // pc_is_return_address describes the seed pc: a ucontext pc is the exact + // interrupted address, while callerPC() is a real return address on every + // architecture except the one where CALLER_PC_IS_RETURN_ADDRESS is false. static int walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, StackWalkFeatures features, EventType event_type, - const void* pc, uintptr_t sp, uintptr_t fp, int lock_index, bool* truncated); + const void* pc, bool pc_is_return_address, + uintptr_t sp, uintptr_t fp, int lock_index, bool* truncated); static int walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, StackWalkFeatures features, EventType event_type, int lock_index, bool* truncated = nullptr); diff --git a/ddprof-lib/src/main/cpp/stackWalker.inline.h b/ddprof-lib/src/main/cpp/stackWalker.inline.h index 218e8da9e0..965f554cfc 100644 --- a/ddprof-lib/src/main/cpp/stackWalker.inline.h +++ b/ddprof-lib/src/main/cpp/stackWalker.inline.h @@ -82,6 +82,11 @@ class WalkPc { // The address to symbolize with or to select an unwind row with. const void* attribution() const { return attributionPC(_pc, _is_return_address); } + // For handing the pair to a callee that derives the attribution address on + // its own. Reading the flag is fine; it is assigning the pc without + // restating its nature that the mutators below exist to prevent. + bool isReturnAddress() const { return _is_return_address; } + // A pc read out of a return-address slot, a link register, or a // DW_CFA_val_expression on the return-address register column. void setReturnAddress(const void* pc) { From b194a070774c459cdee2457269c31d458c976da4 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Thu, 24 Sep 2026 15:27:48 +0200 Subject: [PATCH 4/7] refactor(profiling): name the walk seed instead of branching twice at the call walkVM's public entry point built its two starting states inline, so the pc and the flag describing it were separate arguments chosen a few tokens apart in two near-identical call expressions. Nothing bound them together, which is the same drift the walk itself now avoids by carrying a WalkPc. Both states are one named value instead. walkVMSeed() decides where a walk starts -- register set and the nature of the pc together -- and the entry point delegates without a branch of its own. The frame no longer outlives the decision: its pc/sp/fp are copied out as scalars, which is all the callee ever used, and the ucontext they point into outlives the walk regardless. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/main/cpp/hotspot/hotspotSupport.cpp | 40 +++++++++++++------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index 29fa8ab0e4..f4c902c4e6 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -235,20 +235,36 @@ static const bool CONT_UNWIND_DISABLED = false; static const bool CONT_UNWIND_DISABLED = (std::getenv("DDPROF_DISABLE_CONT_UNWIND") != nullptr); #endif -__attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, - StackWalkFeatures features, EventType event_type, int lock_index, bool* truncated) { +// Where a walk starts, and what the starting pc actually is. The pc and its +// nature are one decision rather than two independent arguments: a ucontext +// carries the exact interrupted address, while callerPC() yields a real return +// address on every architecture whose CALLER_PC_IS_RETURN_ADDRESS says so. +// Kept in one place so a future caller cannot pick a register set from one +// branch and a nature from the other. +struct WalkVMSeed { + void* ucontext; + const void* pc; + bool pc_is_return_address; + uintptr_t sp; + uintptr_t fp; +}; + +static WalkVMSeed walkVMSeed(void* ucontext) { if (ucontext == NULL) { - // callerPC()'s nature is per-arch, the same distinction walkFP/walkDwarf - // seed themselves with. - return walkVM(&empty_ucontext, frames, max_depth, features, event_type, - callerPC(), CALLER_PC_IS_RETURN_ADDRESS, - (uintptr_t)callerSP(), (uintptr_t)callerFP(), lock_index, truncated); - } else { - HotspotStackFrame frame(ucontext); - return walkVM(ucontext, frames, max_depth, features, event_type, - (const void*)frame.pc(), /*pc_is_return_address=*/false, - frame.sp(), frame.fp(), lock_index, truncated); + return {&empty_ucontext, callerPC(), CALLER_PC_IS_RETURN_ADDRESS, + (uintptr_t)callerSP(), (uintptr_t)callerFP()}; } + HotspotStackFrame frame(ucontext); + return {ucontext, (const void*)frame.pc(), /*pc_is_return_address=*/false, + frame.sp(), frame.fp()}; +} + +__attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, + StackWalkFeatures features, EventType event_type, int lock_index, bool* truncated) { + WalkVMSeed seed = walkVMSeed(ucontext); + return walkVM(seed.ucontext, frames, max_depth, features, event_type, + seed.pc, seed.pc_is_return_address, seed.sp, seed.fp, + lock_index, truncated); } __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, From f989f312710d40b91b30d17261e84c09e3a072f5 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Thu, 24 Sep 2026 17:03:38 +0200 Subject: [PATCH 5/7] fix(profiling): keep the library test seam out of release builds Three review follow-ups. addLibraryForTest shipped in every build. Every other test hook in this codebase is either behind #ifdef UNIT_TEST or reached through a friend *TestAccessor, so a publicly callable mutator for the process-wide library set was both inconsistent and present where nothing should be able to call it. It is now compiled only into gtest binaries, which is where its one caller lives. prev_native_pc lost its last reader when the walk started carrying the previous frame as a WalkPc; the declaration stayed behind. Unused-variable warnings are not errors here, so nothing caught it. RemoteSymbolication.md still described the two-argument resolveNativeFrameForWalkVM. It now names the third parameter and says what it selects -- lookups move to the attribution address, the emitted pc_offset does not -- since the wire value being unchanged is the part a reader of that document needs. Co-Authored-By: Claude Opus 5 (1M context) --- ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp | 2 -- ddprof-lib/src/main/cpp/libraries.h | 9 ++++++--- doc/reference/RemoteSymbolication.md | 6 +++--- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index f4c902c4e6..3b778adabe 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -345,8 +345,6 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } } - const void* prev_native_pc = NULL; - // The walking pc and whether it came out of a return-address slot travel // together, so a newly added pc source has to say which it is instead of // inheriting whatever the previous frame happened to set. Range-based diff --git a/ddprof-lib/src/main/cpp/libraries.h b/ddprof-lib/src/main/cpp/libraries.h index efe9b54337..a7045a934c 100644 --- a/ddprof-lib/src/main/cpp/libraries.h +++ b/ddprof-lib/src/main/cpp/libraries.h @@ -82,11 +82,14 @@ class Libraries { return _native_libs; } +#ifdef UNIT_TEST // Publishes a caller-owned CodeCache into the process-wide set so address - // lookups resolve against it. Test-only: CodeCacheArray is append-only, so - // the cache has to outlive every later lookup, and the usual population - // path is updateSymbols() reading the real loaded libraries. + // lookups resolve against it, letting a test build a library whose symbol + // layout it controls. CodeCacheArray is append-only, so the cache has to + // outlive every later lookup. The real population path is updateSymbols() + // reading the loaded libraries; compiled only into gtest binaries. bool addLibraryForTest(CodeCache *lib) { return _native_libs.add(lib); } +#endif // Delete copy constructor and assignment operator to prevent copies Libraries(const Libraries&) = delete; diff --git a/doc/reference/RemoteSymbolication.md b/doc/reference/RemoteSymbolication.md index 5e9313a938..fd660996e1 100644 --- a/doc/reference/RemoteSymbolication.md +++ b/doc/reference/RemoteSymbolication.md @@ -50,7 +50,7 @@ Modified frame collection to support dual modes: **Key Functions**: - `populateRemoteFrame()`: Packs pc_offset, mark, and lib_index into jmethodID field - `resolveNativeFrameForWalkVM()`: Resolves native frames for walkVM/walkVMX modes - - Performs binarySearch() to get symbol name + - Performs binarySearch() to get symbol name, keyed off the attribution address - Extracts mark via NativeFunc::read_mark() (O(1)) - Packs data using RemoteFramePacker::pack() - `convertNativeTrace()`: Converts raw PCs to frames for walkFP/walkDwarf modes @@ -65,7 +65,7 @@ Modified frame collection to support dual modes: **Stack Walker Integration**: - **walkFP/walkDwarf**: Return raw PCs → `convertNativeTrace()` → `populateRemoteFrame()` -- **walkVM/walkVMX**: Directly call `resolveNativeFrameForWalkVM(pc, lock_index)` during stack walk (patched via gradle/patching.gradle) +- **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. ### 5. **JFR Serialization** (`flightRecorder.cpp/h`) @@ -109,7 +109,7 @@ Patches async-profiler's `stackWalker.h` and `stackWalker.cpp` to integrate remo **Implementation Patches (stackWalker.cpp)**: - Updates all `walkVM` signatures to accept and propagate `lock_index` -- **Critical patch at line 454**: Replaces `profiler->findNativeMethod(pc)` with `profiler->resolveNativeFrameForWalkVM(pc, lock_index)` +- **Critical patch at line 454**: Replaces `profiler->findNativeMethod(pc)` with `profiler->resolveNativeFrameForWalkVM(pc, pc_is_return_address, lock_index)` - Adds dynamic BCI selection (BCI_NATIVE_FRAME vs BCI_NATIVE_FRAME_REMOTE) - Adds `fillFrame()` overload for void* method_id to support both symbol names and RemoteFrameInfo pointers - Handles marked C++ interpreter frames (terminates scan if detected) From a924a47a2d46eac758f17fecd781b1ab2d8b0bfd Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Thu, 24 Sep 2026 17:24:41 +0200 Subject: [PATCH 6/7] fix(profiling): adjust sender pcs the arm64 unwind helpers hand back raw The three arch unwind helpers were all treated as having already applied the attribution adjustment. That holds on x86_64, which subtracts one inside the helper, but not on aarch64: every branch walkVM can reach there assigns the link register or a saved-pc slot, both raw return addresses. The one aarch64 branch that does subtract is guarded by `&pc == &this->pc()`, true only on the AsyncGetCallTrace path where the caller passes the frame's own pc rather than a local, so walkVM never takes it. A sample leaving a generated frame for a native caller whose call is the last instruction before a symbol or FDE boundary was therefore still attributed to whatever follows, on arm64, for exactly the frames this change set out to fix. recordUnwoundPc() names the difference in one place. x86_64 behaviour is unchanged; the distinction disappears once the helpers agree on a contract. Reported by Codex review on #813. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/main/cpp/hotspot/hotspotSupport.cpp | 29 +++++++++++++++++-- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index 3b778adabe..1e37a6ef3c 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -235,6 +235,29 @@ static const bool CONT_UNWIND_DISABLED = false; static const bool CONT_UNWIND_DISABLED = (std::getenv("DDPROF_DISABLE_CONT_UNWIND") != nullptr); #endif +// Records a sender pc recovered by unwindPrologue/unwindEpilogue/unwindStub, +// which disagree across architectures about what they hand back. +// +// x86_64 folds the attribution adjustment into the value itself, and not even +// uniformly -- unwindPrologue's isFrameComplete branch returns the address +// unadjusted while its two siblings subtract one. Adjusting again here would +// double-count the ones that already did it, so the result is taken as-is. +// +// aarch64 subtracts nothing on any branch walkVM can reach: every assignment +// is the link register or a saved-pc slot, both raw return addresses. (The one +// branch that does adjust is guarded by `&pc == &this->pc()`, which only holds +// for the AsyncGetCallTrace path, where the caller passes the frame's own pc +// rather than a local.) So there the recovered pc still needs the adjustment. +// +// Unifying the two contracts removes the need for this distinction. +static void recordUnwoundPc(WalkPc& walk_pc, const void* pc) { +#if defined(__aarch64__) + walk_pc.setReturnAddress(pc); +#else + walk_pc.setExactAddress(pc); +#endif +} + // Where a walk starts, and what the starting pc actually is. The pc and its // nature are one decision rather than two independent arguments: a ucontext // carries the exact interrupted address, while callerPC() yields a real return @@ -643,7 +666,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex if (nm->isFrameCompleteAt(walk_pc.raw())) { const void* epilogue_pc = walk_pc.raw(); if (depth == 1 && frame.unwindEpilogue(nm, (uintptr_t&)epilogue_pc, sp, fp)) { - walk_pc.setExactAddress(epilogue_pc); + recordUnwoundPc(walk_pc, epilogue_pc); continue; } @@ -687,7 +710,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex continue; } else if (const void* prologue_pc = walk_pc.raw(); frame.unwindPrologue(nm, (uintptr_t&)prologue_pc, sp, fp)) { - walk_pc.setExactAddress(prologue_pc); + recordUnwoundPc(walk_pc, prologue_pc); continue; } @@ -743,7 +766,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex const void* stub_pc = walk_pc.raw(); if (frame.unwindStub((instruction_t*)start, name, (uintptr_t&)stub_pc, sp, fp)) { - walk_pc.setExactAddress(stub_pc); + recordUnwoundPc(walk_pc, stub_pc); continue; } From 61959b06f20c0ca25da3c19658b8fffc869b9a17 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Fri, 25 Sep 2026 13:10:55 +0200 Subject: [PATCH 7/7] fix(profiling): realign walkVM's DWARF step with walkDwarf's The two copies of this unwind step have drifted. Each gap is taken from walkDwarf, which is the one that got the attention: - The link register was fed to findFrameDesc unstripped. On aarch64 it carries PAC bits, so the lookup address is nonsense. walkDwarf strips it and says why; this side never picked that up. Only reachable through a frame table carrying DW_LINK_REGISTER, which DwarfParser does not emit and SFrameParser does, so it is latent rather than live today. - The frame-pointer slot was dereferenced without checking its alignment, while the pc slot three lines below is checked. Same load, same exposure. - Neither slot load carried a fault-injection hook, so the recovery path around them went unexercised. LIKELY matches the existing convention: walkVM's seven UNLIKELY sites are raw dereferences, these two go through SafeAccess. Reconciling these first keeps the extraction that follows a pure move, with no behaviour hidden inside it. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/main/cpp/hotspot/hotspotSupport.cpp | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index 1e37a6ef3c..cefdad652f 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -1007,7 +1007,13 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex f.isSignalFrame()); } else { if (f.fp_off != DW_SAME_FP && f.fp_off < MAX_FRAME_SIZE && f.fp_off > -MAX_FRAME_SIZE) { - fp = (uintptr_t)SafeAccess::load((void**)(sp + f.fp_off)); + // Verify alignment before dereferencing sp + offset, as the pc + // slot below already does. + uintptr_t fp_addr = sp + f.fp_off; + if (!aligned(fp_addr)) { + break; + } + fp = (uintptr_t)SafeAccess::load(INJECT_FAULT_ADDRESS_LIKELY((void**)fp_addr)); } if (EMPTY_FRAME_SIZE > 0 || f.pc_off != DW_LINK_REGISTER) { @@ -1016,10 +1022,15 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex if (!aligned(pc_addr)) { break; } - walk_pc.setRecoveredPc(stripPointer(SafeAccess::load((void**)pc_addr)), - f.isSignalFrame()); + walk_pc.setRecoveredPc( + stripPointer(SafeAccess::load(INJECT_FAULT_ADDRESS_LIKELY((void**)pc_addr))), + f.isSignalFrame()); } else if (depth == 1) { - walk_pc.setRecoveredPc((const void*)frame.link(), f.isSignalFrame()); + // Matches the memory-slot path above: StackFrame::link() returns + // the raw link register, which carries PAC bits on aarch64 and + // would otherwise be fed to findFrameDesc as a nonsense address. + walk_pc.setRecoveredPc(stripPointer((const void*)frame.link()), + f.isSignalFrame()); } else { break; }