From 7c21727a7f6b4432866d80ec1ccaf6d3d5896c20 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Tue, 29 Sep 2026 13:56:16 +0200 Subject: [PATCH 1/4] fix(profiling): hand back raw return addresses from the unwind helpers unwindStub, unwindPrologue, unwindEpilogue and unwindCompiled recover a sender pc from a stack slot or the link register. x86_64 subtracted one from it inside the helper; aarch64 did not. Nothing in the signature said which, so walkVM could only guess from the target architecture, which it did through recordUnwoundPc and an #ifdef. The guess was wrong in a way that mattered. On x86_64 the pc reaching the top of the walk loop was already decremented, so isContReturnBarrier, isContEntryReturnPc and isEntryFrame -- all comparing for exact equality against genuine return addresses -- could never match. Continuation boundaries and entry frames simply stopped being detected there, quietly, producing worse unwinding rather than any visible failure. x86_64 was not even self-consistent: unwindPrologue's isFrameComplete arm and unwindCompiled's entry-barrier arm returned the address unadjusted while their siblings subtracted one, so no single rule described what a caller had been handed. All eleven adjustments are gone. Every helper now returns the address exactly as it sat in the slot, and walkVM applies the attribution adjustment itself through the WalkPc it already carries -- so the lookups keep seeing pc - 1 and the exact-address consumers finally see the real return address. aarch64 is unchanged: its helpers already returned raw and walkVM already recorded them as return addresses. The whole behavioural delta is on x86_64. unwindCompiled is reached only from getJavaTraceAsync, so that path now sees raw addresses on x86_64 where it saw adjusted ones before. aarch64 already handed it raw, so this makes the two agree rather than introducing a new split; calling it out because it is a behaviour change on a legacy path that no test covers. unwindHelperContract_ut.cpp pins the contract directly, with no JVM, no VMStructs and no nmethod -- reaching these helpers through walkVM would need a fake JavaThread and CodeHeap, and a fixture that far from a real layout starts passing for the wrong reason. Reintroducing a folded adjustment on either arch fails both tests. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/main/cpp/hotspot/hotspotStackFrame.h | 17 ++ .../cpp/hotspot/hotspotStackFrame_x64.cpp | 24 ++- .../src/main/cpp/hotspot/hotspotSupport.cpp | 28 +--- .../src/test/cpp/unwindHelperContract_ut.cpp | 147 ++++++++++++++++++ 4 files changed, 178 insertions(+), 38 deletions(-) create mode 100644 ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame.h b/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame.h index 1e360027c7..f5d05efa62 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame.h +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame.h @@ -60,6 +60,23 @@ class HotspotStackFrame : public StackFrame { } }; + // UNWIND HELPER CONTRACT + // + // On success each of these replaces `pc` with the sender's *raw* return + // address -- the address control returns to, exactly as it sat in the + // stack slot or the link register. None of them applies the attribution + // adjustment; that is the caller's decision, because only the caller + // knows what it is about to do with the address. + // + // x86_64 used to subtract one inside the helper, and not on every branch + // (unwindPrologue's isFrameComplete arm omitted it), while aarch64 never + // did. A caller could not tell which it had been handed, so walkVM + // guessed from the target architecture. Exact-address consumers on the + // far side -- isContReturnBarrier, isContEntryReturnPc, isEntryFrame, + // all comparing against genuine return addresses -- were silently + // failing on x86_64 as a result. + // + // unwindHelperContract_ut.cpp pins this. bool unwindCompiled(VMNMethod* nm) { return unwindCompiled(nm, pc(), sp(), fp()); } diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame_x64.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame_x64.cpp index dad23b0dac..f7c53d9ace 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame_x64.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame_x64.cpp @@ -19,7 +19,7 @@ __attribute__((no_sanitize("address"))) bool HotspotStackFrame::unwindStub(instr || strncmp(name, "vtable", 6) == 0 || strcmp(name, "InlineCacheBuffer") == 0) { - pc = ((uintptr_t*)sp)[0] - 1; + pc = ((uintptr_t*)sp)[0]; sp += 8; return true; } else if (entry != NULL && ([&] { unsigned int val; memcpy(&val, entry, sizeof(val)); return val; }()) == 0xec8b4855) { @@ -27,13 +27,13 @@ __attribute__((no_sanitize("address"))) bool HotspotStackFrame::unwindStub(instr // push rbp // mov rbp, rsp if (ip == entry + 1) { - pc = ((uintptr_t*)sp)[1] - 1; + pc = ((uintptr_t*)sp)[1]; sp += 16; return true; } else if (withinCurrentStack(fp)) { sp = fp + 16; fp = ((uintptr_t*)sp)[-2]; - pc = ((uintptr_t*)sp)[-1] - 1; + pc = ((uintptr_t*)sp)[-1]; return true; } } @@ -101,26 +101,24 @@ bool HotspotStackFrame::unwindCompiled(VMNMethod* nm, uintptr_t& pc, uintptr_t& || ip[-1] == 0x5d // after pop rbp || (ip[0] == 0x41 && ip[1] == 0x85 && ip[2] == 0x02 && ip[3] == 0xc3)) // poll return { - // Subtract 1 for PC to point to the call instruction, - // otherwise it may be attributed to a wrong bytecode - pc = ((uintptr_t*)sp)[0] - 1; + pc = ((uintptr_t*)sp)[0]; sp += 8; return true; } else if (*ip == 0x5d) { // pop rbp fp = ((uintptr_t*)sp)[0]; - pc = ((uintptr_t*)sp)[1] - 1; + pc = ((uintptr_t*)sp)[1]; sp += 16; return true; } else if (ip <= entry + 15 && ((uintptr_t)ip & 0xfff) && ip[-1] == 0x55) { // push rbp - pc = ((uintptr_t*)sp)[1] - 1; + pc = ((uintptr_t*)sp)[1]; sp += 16; return true; } else if (ip <= entry + 7 && ip[0] == 0x48 && ip[1] == 0x89 && ip[2] == 0x6c && ip[3] == 0x24) { // mov [rsp + #off], rbp sp += ip[4] + 16; - pc = ((uintptr_t*)sp)[-1] - 1; + pc = ((uintptr_t*)sp)[-1]; return true; } else if ((ip[0] == 0x41 && ip[1] == 0x81 && ip[2] == 0x7f && *(u32*)(ip + 4) == 1) || (ip >= entry + 8 && ip[-8] == 0x41 && ip[-7] == 0x81 && ip[-6] == 0x7f && *(u32*)(ip - 4) == 1)) { @@ -142,11 +140,11 @@ bool HotspotStackFrame::unwindPrologue(VMNMethod* nm, uintptr_t& pc, uintptr_t& instruction_t* ip = (instruction_t*)pc; instruction_t* entry = (instruction_t*)nm->entry(); if (ip <= entry || *ip == 0x55 || nm->frameSize() == 0) { // push rbp - pc = ((uintptr_t*)sp)[0] - 1; + pc = ((uintptr_t*)sp)[0]; sp += 8; return true; } else if (ip <= entry + 15 && ip[-1] == 0x55) { // right after push rbp - pc = ((uintptr_t*)sp)[1] - 1; + pc = ((uintptr_t*)sp)[1]; sp += 16; return true; } else if (ip <= entry + 31 && isFrameComplete(entry, ip)) { @@ -165,12 +163,12 @@ bool HotspotStackFrame::unwindEpilogue(VMNMethod* nm, uintptr_t& pc, uintptr_t& // ret instruction_t* ip = (instruction_t*)pc; if (*ip == 0xc3 || isPollReturn(ip)) { // ret - pc = ((uintptr_t*)sp)[0] - 1; + pc = ((uintptr_t*)sp)[0]; sp += 8; return true; } else if (*ip == 0x5d) { // pop rbp fp = ((uintptr_t*)sp)[0]; - pc = ((uintptr_t*)sp)[1] - 1; + pc = ((uintptr_t*)sp)[1]; sp += 16; return true; } diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index 109fd415b3..26ecf48648 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -236,28 +236,6 @@ 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 -} __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) { @@ -662,7 +640,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)) { - recordUnwoundPc(walk_pc, epilogue_pc); + walk_pc.setReturnAddress(epilogue_pc); continue; } @@ -707,7 +685,7 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } else { const void* prologue_pc = walk_pc.raw(); if (frame.unwindPrologue(nm, (uintptr_t&)prologue_pc, sp, fp)) { - recordUnwoundPc(walk_pc, prologue_pc); + walk_pc.setReturnAddress(prologue_pc); continue; } } @@ -764,7 +742,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)) { - recordUnwoundPc(walk_pc, stub_pc); + walk_pc.setReturnAddress(stub_pc); continue; } diff --git a/ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp b/ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp new file mode 100644 index 0000000000..bc298b7c2c --- /dev/null +++ b/ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp @@ -0,0 +1,147 @@ +/* + * Copyright 2026, Datadog, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +// Pins what HotspotStackFrame's unwind helpers hand back: the sender's raw +// return address, on every architecture and every branch, with no attribution +// adjustment applied. +// +// They used to disagree. x86_64 subtracted one inside the helper, and not even +// on every branch -- unwindPrologue's isFrameComplete arm returned the address +// untouched while its two siblings adjusted it -- while aarch64 never did. A +// caller could not tell which it had been handed, so walkVM guessed from the +// target architecture, and the exact-address consumers on the far side +// (isContReturnBarrier, isContEntryReturnPc, isEntryFrame) silently stopped +// matching on x86_64. +// +// These tests drive the helpers directly: they need a fabricated frame and a +// few bytes of synthetic code, but no JVM, no VMStructs and no nmethod. That +// is deliberate -- reaching the same helpers through walkVM means faking a +// JavaThread and a CodeHeap, and the further the fixture drifts from a real +// JVM layout the more likely it is to pass for the wrong reason. + +#include +#include +#include +#include +#include "arch.h" +#include "hotspot/hotspotStackFrame.h" +#include "stackFrame.h" + +namespace { + +// Enough room for the generic aarch64 prologue scan, which reads up to eight +// instructions past `entry` before giving up. +const size_t kCodeWords = 16; +const size_t kStackWords = 32; + +const uintptr_t kSenderRa = 0x5a5a00001000ULL; // what sits in the stack slot +const uintptr_t kLinkReg = 0x5a5a00002000ULL; // what sits in the link register + +class UnwindHelperContractTest : public ::testing::Test { + protected: + void SetUp() override { + memset(_code, 0, sizeof(_code)); + memset(_stack, 0, sizeof(_stack)); + memset(&_uc, 0, sizeof(_uc)); +#ifdef __APPLE__ + // uc_mcontext is a pointer on Darwin and embedded on Linux, so a + // zeroed ucontext needs storage to point at before any register + // access. + memset(&_mc, 0, sizeof(_mc)); + _uc.uc_mcontext = &_mc; +#endif + setLinkRegister(kLinkReg); + } + + // The link register is not writable through StackFrame (link() is const), + // so the test writes the ucontext slot that link() reads back. + static void setLinkRegister(uintptr_t value) { +#if defined(__aarch64__) && defined(__APPLE__) + _uc.uc_mcontext->__ss.__lr = value; +#elif defined(__aarch64__) + _uc.uc_mcontext.regs[30] = value; +#else + (void)value; // x86_64 has no link register; StackFrame::link() is 0 +#endif + } + + // Stack slot the helpers read a saved pc out of. Kept mid-buffer so an + // sp that moves in either direction stays inside the allocation. + uintptr_t* stackSlot() { return &_stack[kStackWords / 2]; } + + instruction_t* code() { return (instruction_t*)_code; } + + static ucontext_t _uc; +#ifdef __APPLE__ + static _STRUCT_MCONTEXT64 _mc; +#endif + static uintptr_t _code[kCodeWords]; + static uintptr_t _stack[kStackWords]; +}; + +ucontext_t UnwindHelperContractTest::_uc; +#ifdef __APPLE__ +_STRUCT_MCONTEXT64 UnwindHelperContractTest::_mc; +#endif +uintptr_t UnwindHelperContractTest::_code[kCodeWords]; +uintptr_t UnwindHelperContractTest::_stack[kStackWords]; + +// The shared entry branch: pc sitting exactly on the stub's first +// instruction. Both arches take it, and both now hand back a raw address -- +// they differ only in where the sender pc lives on that architecture. +TEST_F(UnwindHelperContractTest, UnwindStubAtEntryRecoversTheRawSenderPc) { + HotspotStackFrame frame(&_uc); + + uintptr_t* slot = stackSlot(); + *slot = kSenderRa; + + uintptr_t pc = (uintptr_t)code(); + uintptr_t sp = (uintptr_t)slot; + uintptr_t fp = (uintptr_t)slot; + + ASSERT_TRUE(frame.unwindStub(code(), "someStub", pc, sp, fp)) + << "pc on the stub's entry instruction is the branch both arches take"; + +#if defined(__x86_64__) + EXPECT_EQ(kSenderRa, pc) + << "the saved return address is handed back exactly as it sat in the " + << "slot -- no adjustment folded in by the helper"; + EXPECT_EQ((uintptr_t)slot + sizeof(void*), sp) + << "and the slot it consumed is popped"; +#elif defined(__aarch64__) + EXPECT_EQ(kLinkReg, pc) + << "the link register is handed back verbatim"; + EXPECT_EQ((uintptr_t)slot, sp) + << "and sp is left alone, the return address never having been on the stack"; +#endif +} + +// The property that matters to a caller, stated without reference to which +// architecture this is: whatever the helper returns is a raw return address, +// so the caller can apply the attribution adjustment itself and be right +// everywhere. Before the contract was unified this could not be written -- +// the answer depended on the target. +TEST_F(UnwindHelperContractTest, HelperNeverAppliesTheAdjustmentItself) { + HotspotStackFrame frame(&_uc); + + uintptr_t* slot = stackSlot(); + *slot = kSenderRa; + + uintptr_t pc = (uintptr_t)code(); + uintptr_t sp = (uintptr_t)slot; + uintptr_t fp = (uintptr_t)slot; + + ASSERT_TRUE(frame.unwindStub(code(), "someStub", pc, sp, fp)); + + // Whichever slot this architecture sources the sender pc from, it comes + // back unmodified. An off-by-one here means someone reintroduced a folded + // adjustment, and walkVM would then subtract a second one. + const bool is_raw = (pc == kSenderRa) || (pc == kLinkReg); + EXPECT_TRUE(is_raw) + << "expected an unmodified return address, got pc=0x" << std::hex << pc + << " (saved slot 0x" << kSenderRa << ", link register 0x" << kLinkReg << ")"; +} + +} // namespace From 8fa524ea2f6d90dcb0915d7f1969c43b273a1a3c Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Tue, 29 Sep 2026 14:42:44 +0200 Subject: [PATCH 2/4] test: cover unwindPrologue/unwindEpilogue with a fake nmethod The unwind helper contract tests reached unwindStub only. unwindPrologue and unwindEpilogue take a VMNMethod*, which left the two helpers that carried most of the folded `- 1` adjustments untested. A 32-byte buffer standing in for an nmethod closes that. Only entry() and frameSize() are read on the branches under test, so the fixture points VMStructs' field offsets at its own layout for the duration of the test and restores them afterwards, via a per-TU VMStructsTestAccessor like the ones in hotspotMethodId_ut.cpp and hotspot_crash_protection_ut.cpp. Three branches are pinned: pc on the method's entry instruction, pc one instruction past the push/stp that saved the caller's fp (the branch where x86_64 read a different stack slot than its sibling), and pc on the `ret`. Each of the three was mutation-checked by reintroducing the adjustment at that site; each failure was caught by exactly one test. The fixture selects entry()'s pre-JDK-23 arm, since that one is a single indirection. What is under test is the addressing convention rather than the field layout, but the tests say so, so nobody reads them as covering both nmethod shapes. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/test/cpp/unwindHelperContract_ut.cpp | 193 +++++++++++++++++- 1 file changed, 188 insertions(+), 5 deletions(-) diff --git a/ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp b/ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp index bc298b7c2c..7820311eed 100644 --- a/ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp +++ b/ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp @@ -15,11 +15,19 @@ // (isContReturnBarrier, isContEntryReturnPc, isEntryFrame) silently stopped // matching on x86_64. // -// These tests drive the helpers directly: they need a fabricated frame and a -// few bytes of synthetic code, but no JVM, no VMStructs and no nmethod. That -// is deliberate -- reaching the same helpers through walkVM means faking a -// JavaThread and a CodeHeap, and the further the fixture drifts from a real -// JVM layout the more likely it is to pass for the wrong reason. +// These tests drive the helpers directly: a fabricated frame, a few bytes of +// synthetic code, and -- for unwindPrologue/unwindEpilogue, which take a +// VMNMethod* -- a byte buffer standing in for an nmethod. No JVM is started. +// That is deliberate: reaching the same helpers through walkVM means faking a +// JavaThread and a CodeHeap too, and the further the fixture drifts from a +// real JVM layout the more likely it is to pass for the wrong reason. +// +// The nmethod fixture pins the pre-JDK-23 field shape, because entry() +// branches on `_nmethod_entry_offset != -1` and the older arm is a single +// indirection rather than a code_offset/verified_entry_offset pair. What is +// under test is the addressing convention, not the field layout, so either +// arm would do -- but read these tests as covering the helpers' contract, +// not as covering both nmethod layouts. #include #include @@ -27,8 +35,28 @@ #include #include "arch.h" #include "hotspot/hotspotStackFrame.h" +#include "hotspot/vmStructs.h" #include "stackFrame.h" +// Grants access to VMStructs' private field offsets (see the +// `friend class VMStructsTestAccessor;` declaration in vmStructs.h) so the +// nmethod fixture below can describe its own layout without a live JVM. +// Mirrors the same-named accessors in hotspotMethodId_ut.cpp and +// hotspot_crash_protection_ut.cpp -- each is a separate +// translation-unit-local definition naming only the fields that file needs; +// friendship is granted by name+scope, not by a single shared type. It has to +// sit outside the anonymous namespace, since that is the scope vmStructs.h +// befriends. +class VMStructsTestAccessor { +public: + static offset getFrameSizeOffset() { return VMStructs::_frame_size_offset; } + static void setFrameSizeOffset(offset value) { VMStructs::_frame_size_offset = value; } + static offset getNMethodEntryOffset() { return VMStructs::_nmethod_entry_offset; } + static void setNMethodEntryOffset(offset value) { VMStructs::_nmethod_entry_offset = value; } + static offset getNMethodEntryAddress() { return VMStructs::_nmethod_entry_address; } + static void setNMethodEntryAddress(offset value) { VMStructs::_nmethod_entry_address = value; } +}; + namespace { // Enough room for the generic aarch64 prologue scan, which reads up to eight @@ -144,4 +172,159 @@ TEST_F(UnwindHelperContractTest, HelperNeverAppliesTheAdjustmentItself) { << " (saved slot 0x" << kSenderRa << ", link register 0x" << kLinkReg << ")"; } +// --------------------------------------------------------------------------- +// unwindPrologue / unwindEpilogue +// +// Same contract, but these two take a VMNMethod*, so they need a stand-in. +// Only two fields are ever read on the branches under test: entry(), to +// locate the start of the method's code, and frameSize(). The fixture is a +// byte buffer laid out to satisfy exactly those, with the offsets pointed at +// it for the duration of the test and restored afterwards -- the offsets are +// process-global, and a real JVM run would have populated them from +// vmStructs. +// --------------------------------------------------------------------------- + +// Byte offsets within _nmethod. Arbitrary, beyond needing natural alignment +// for the types stored there and staying inside the buffer. +const int kEntryAddressOffset = 8; // holds a void* to the method's code +const int kFrameSizeOffset = 16; // holds an int, in words + +const uintptr_t kOtherSlot = 0x5a5a00003000ULL; // a slot that must NOT be read + +class UnwindNMethodHelperContractTest : public UnwindHelperContractTest { + protected: + void SetUp() override { + UnwindHelperContractTest::SetUp(); + + _saved_frame_size_offset = VMStructsTestAccessor::getFrameSizeOffset(); + _saved_entry_offset = VMStructsTestAccessor::getNMethodEntryOffset(); + _saved_entry_address = VMStructsTestAccessor::getNMethodEntryAddress(); + + memset(_nmethod, 0, sizeof(_nmethod)); + *(const void**)(bytes() + kEntryAddressOffset) = code(); + *(int*)(bytes() + kFrameSizeOffset) = 4; + + VMStructsTestAccessor::setFrameSizeOffset(kFrameSizeOffset); + // -1 selects entry()'s pre-JDK-23 arm: a single load of the stored + // pointer, rather than code_offset + verified_entry_offset. + VMStructsTestAccessor::setNMethodEntryOffset(-1); + VMStructsTestAccessor::setNMethodEntryAddress(kEntryAddressOffset); + } + + void TearDown() override { + VMStructsTestAccessor::setFrameSizeOffset(_saved_frame_size_offset); + VMStructsTestAccessor::setNMethodEntryOffset(_saved_entry_offset); + VMStructsTestAccessor::setNMethodEntryAddress(_saved_entry_address); + } + + VMNMethod* nmethod() { + // cast_raw, not cast: the latter asserts the buffer is as large as the + // JVM-reported type size, which is zero here because no JVM populated + // it. + return VMNMethod::cast_raw(_nmethod); + } + + // uintptr_t rather than char, so the buffer is aligned for the void* and + // int the layout above stores in it. + char* bytes() { return (char*)_nmethod; } + + static uintptr_t _nmethod[4]; + + private: + offset _saved_frame_size_offset; + offset _saved_entry_offset; + offset _saved_entry_address; +}; + +uintptr_t UnwindNMethodHelperContractTest::_nmethod[4]; + +// pc sitting on the method's first instruction: nothing of the frame has been +// built yet, so the sender pc is still wherever the call left it. +TEST_F(UnwindNMethodHelperContractTest, UnwindPrologueAtEntryRecoversTheRawSenderPc) { + HotspotStackFrame frame(&_uc); + + uintptr_t* slot = stackSlot(); + slot[0] = kSenderRa; + + uintptr_t pc = (uintptr_t)code(); // == nm->entry() + uintptr_t sp = (uintptr_t)slot; + uintptr_t fp = (uintptr_t)slot; + + ASSERT_TRUE(frame.unwindPrologue(nmethod(), pc, sp, fp)); + +#if defined(__x86_64__) + EXPECT_EQ(kSenderRa, pc) + << "the pushed return address comes back exactly as it sat in the slot"; + EXPECT_EQ((uintptr_t)slot + sizeof(void*), sp); +#elif defined(__aarch64__) + EXPECT_EQ(kLinkReg, pc) << "the link register comes back verbatim"; + EXPECT_EQ((uintptr_t)slot, sp); +#endif +} + +// One instruction further in, past the push/stp that saved the caller's fp: +// both architectures now source the sender pc from the *second* stack slot and +// pop two. This is the branch where x86_64's folded adjustment used to sit at a +// different slot than its sibling's, so it is worth pinning separately. +TEST_F(UnwindNMethodHelperContractTest, UnwindPrologueAfterFrameSetupRecoversTheRawSenderPc) { + HotspotStackFrame frame(&_uc); + + uintptr_t* slot = stackSlot(); + slot[0] = kOtherSlot; // the saved fp, not a return address + slot[1] = kSenderRa; + +#if defined(__x86_64__) + code()[0] = 0x55; // push %rbp +#elif defined(__aarch64__) + code()[0] = 0xa9bf7bfd; // stp x29, x30, [sp, #-16]! + code()[1] = 0x910003fd; // mov x29, sp +#endif + + uintptr_t pc = (uintptr_t)&code()[1]; + uintptr_t sp = (uintptr_t)slot; + uintptr_t fp = (uintptr_t)slot; + + ASSERT_TRUE(frame.unwindPrologue(nmethod(), pc, sp, fp)); + + EXPECT_EQ(kSenderRa, pc) + << "expected the raw return address from the second slot; got 0x" + << std::hex << pc << " (adjacent slot holds 0x" << kOtherSlot << ")"; + EXPECT_EQ((uintptr_t)slot + 2 * sizeof(void*), sp) + << "and both slots are popped"; +} + +// pc on the `ret` itself: the frame is already torn down, so the sender pc is +// back where the prologue found it. +TEST_F(UnwindNMethodHelperContractTest, UnwindEpilogueAtReturnRecoversTheRawSenderPc) { + HotspotStackFrame frame(&_uc); + + uintptr_t* slot = stackSlot(); + slot[0] = kSenderRa; + + // Placed a couple of instructions in so the ip[-1] reads on the branches + // this does not take stay inside the buffer. + const size_t ret_index = 2; +#if defined(__x86_64__) + code()[ret_index] = 0xc3; // ret +#elif defined(__aarch64__) + code()[ret_index] = 0xd65f03c0; // ret +#endif + + uintptr_t pc = (uintptr_t)&code()[ret_index]; + uintptr_t sp = (uintptr_t)slot; + uintptr_t fp = (uintptr_t)slot; + + ASSERT_TRUE(frame.unwindEpilogue(nmethod(), pc, sp, fp)); + +#if defined(__x86_64__) + EXPECT_EQ(kSenderRa, pc) + << "the return address about to be consumed by `ret` is handed back " + << "unmodified"; + EXPECT_EQ((uintptr_t)slot + sizeof(void*), sp); +#elif defined(__aarch64__) + EXPECT_EQ(kLinkReg, pc) << "the link register comes back verbatim"; + EXPECT_EQ((uintptr_t)slot, sp); +#endif +} + } // namespace From b449e309965ca323745c49ea865cb967a8dc20f3 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Wed, 30 Sep 2026 12:48:24 +0200 Subject: [PATCH 3/4] fix(profiling): step into the call for the AsyncGetCallTrace retry on x86_64 The register-less unwindStub/unwindCompiled overloads write the sender pc into the real ucontext and hand it straight to AsyncGetCallTrace, which has no WalkPc to apply the attribution adjustment. Now that the helpers return raw return addresses, subtract one in those overloads on x86_64, as the helpers used to. Co-Authored-By: Claude Sonnet 5.5 --- .../src/main/cpp/hotspot/hotspotStackFrame.h | 21 +++++++++++++++-- .../src/test/cpp/unwindHelperContract_ut.cpp | 23 +++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame.h b/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame.h index f5d05efa62..a36c03456f 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame.h +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame.h @@ -77,12 +77,21 @@ class HotspotStackFrame : public StackFrame { // failing on x86_64 as a result. // // unwindHelperContract_ut.cpp pins this. + // The overloads below without explicit registers write the sender into the + // real ucontext for an AsyncGetCallTrace retry, which has no WalkPc to + // apply the attribution adjustment afterwards. On x86_64 HotSpot + // attributes the recovered caller to the instruction the pc points at, so + // the raw return address would select the bytecode after the call; step + // back into the call here. aarch64 has always handed AsyncGetCallTrace + // the raw return address and keeps doing so. bool unwindCompiled(VMNMethod* nm) { - return unwindCompiled(nm, pc(), sp(), fp()); + bool ok = unwindCompiled(nm, pc(), sp(), fp()); + return ok && stepIntoCallForAsgct(); } bool unwindStub(instruction_t* entry, const char* name) { - return unwindStub(entry, name, pc(), sp(), fp()); + bool ok = unwindStub(entry, name, pc(), sp(), fp()); + return ok && stepIntoCallForAsgct(); } bool unwindStub(instruction_t* entry, const char* name, uintptr_t& pc, uintptr_t& sp, uintptr_t& fp); @@ -94,6 +103,14 @@ class HotspotStackFrame : public StackFrame { bool unwindEpilogue(VMNMethod* nm, uintptr_t& pc, uintptr_t& sp, uintptr_t& fp); static bool unwindAtomicStub(const StackFrame& frame, const void*& pc); + +private: + bool stepIntoCallForAsgct() { +#if defined(__x86_64__) + pc() -= 1; +#endif + return true; + } }; #endif // _HOTSPOT_HOTSPOTSTACKFRAME_H diff --git a/ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp b/ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp index 7820311eed..a6dc7569bd 100644 --- a/ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp +++ b/ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp @@ -172,6 +172,29 @@ TEST_F(UnwindHelperContractTest, HelperNeverAppliesTheAdjustmentItself) { << " (saved slot 0x" << kSenderRa << ", link register 0x" << kLinkReg << ")"; } +// The register-less overload is what getJavaTraceAsync() calls: it rewrites +// the real ucontext and hands it straight to AsyncGetCallTrace, with no +// WalkPc to adjust afterwards. On x86_64 the ucontext must therefore carry +// the address inside the call instruction; aarch64 keeps the raw one. +TEST_F(UnwindHelperContractTest, AsgctOverloadLeavesUcontextReadyForAsyncGetCallTrace) { + HotspotStackFrame frame(&_uc); + + uintptr_t* slot = stackSlot(); + *slot = kSenderRa; + + frame.pc() = (uintptr_t)code(); + frame.sp() = (uintptr_t)slot; + frame.fp() = (uintptr_t)slot; + + ASSERT_TRUE(frame.unwindStub(code(), "someStub")); + +#if defined(__x86_64__) + EXPECT_EQ(kSenderRa - 1, frame.pc()); +#else + EXPECT_EQ(kLinkReg, frame.pc()); +#endif +} + // --------------------------------------------------------------------------- // unwindPrologue / unwindEpilogue // From 4ee10c4723f9c8c215d78d1dca347371542c55b6 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Wed, 30 Sep 2026 15:51:46 +0200 Subject: [PATCH 4/4] docs: state the unwind helper contract on its own terms Explain why the helpers return raw return addresses (exact-address consumers compare them, symbolizers derive the attribution address from them) instead of describing earlier behavior. Co-Authored-By: Claude Sonnet 5.5 --- .../src/main/cpp/hotspot/hotspotStackFrame.h | 20 +++++++------ .../src/test/cpp/unwindHelperContract_ut.cpp | 30 +++++++++---------- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame.h b/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame.h index a36c03456f..00f505f1bb 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame.h +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame.h @@ -68,22 +68,24 @@ class HotspotStackFrame : public StackFrame { // adjustment; that is the caller's decision, because only the caller // knows what it is about to do with the address. // - // x86_64 used to subtract one inside the helper, and not on every branch - // (unwindPrologue's isFrameComplete arm omitted it), while aarch64 never - // did. A caller could not tell which it had been handed, so walkVM - // guessed from the target architecture. Exact-address consumers on the - // far side -- isContReturnBarrier, isContEntryReturnPc, isEntryFrame, - // all comparing against genuine return addresses -- were silently - // failing on x86_64 as a result. + // Returning the raw address on every architecture and every branch lets a + // caller rely on one meaning without knowing the target. Some consumers + // need the genuine return address: isContReturnBarrier, + // isContEntryReturnPc and isEntryFrame compare it for equality against + // known addresses, so a value already reduced by one never matches. + // Consumers that symbolize derive the attribution address from the raw one + // (see attributionPC in stackWalker.inline.h); applying the adjustment + // inside a helper would make that a second subtraction. // // unwindHelperContract_ut.cpp pins this. + // // The overloads below without explicit registers write the sender into the // real ucontext for an AsyncGetCallTrace retry, which has no WalkPc to // apply the attribution adjustment afterwards. On x86_64 HotSpot // attributes the recovered caller to the instruction the pc points at, so // the raw return address would select the bytecode after the call; step - // back into the call here. aarch64 has always handed AsyncGetCallTrace - // the raw return address and keeps doing so. + // back into the call here. aarch64 passes the raw return address to + // AsyncGetCallTrace unchanged. bool unwindCompiled(VMNMethod* nm) { bool ok = unwindCompiled(nm, pc(), sp(), fp()); return ok && stepIntoCallForAsgct(); diff --git a/ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp b/ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp index a6dc7569bd..3209f0acf5 100644 --- a/ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp +++ b/ddprof-lib/src/test/cpp/unwindHelperContract_ut.cpp @@ -7,13 +7,12 @@ // return address, on every architecture and every branch, with no attribution // adjustment applied. // -// They used to disagree. x86_64 subtracted one inside the helper, and not even -// on every branch -- unwindPrologue's isFrameComplete arm returned the address -// untouched while its two siblings adjusted it -- while aarch64 never did. A -// caller could not tell which it had been handed, so walkVM guessed from the -// target architecture, and the exact-address consumers on the far side -// (isContReturnBarrier, isContEntryReturnPc, isEntryFrame) silently stopped -// matching on x86_64. +// The raw address is the right thing to return because callers differ in what +// they need from it. Exact-address consumers (isContReturnBarrier, +// isContEntryReturnPc, isEntryFrame) compare it against known return +// addresses, and symbolizing callers derive the attribution address from it +// themselves; a helper that adjusted internally would break the former and +// double-adjust for the latter. // // These tests drive the helpers directly: a fabricated frame, a few bytes of // synthetic code, and -- for unwindPrologue/unwindEpilogue, which take a @@ -117,8 +116,8 @@ uintptr_t UnwindHelperContractTest::_code[kCodeWords]; uintptr_t UnwindHelperContractTest::_stack[kStackWords]; // The shared entry branch: pc sitting exactly on the stub's first -// instruction. Both arches take it, and both now hand back a raw address -- -// they differ only in where the sender pc lives on that architecture. +// instruction. Both arches take it and hand back a raw address; they differ +// only in where the sender pc lives on that architecture. TEST_F(UnwindHelperContractTest, UnwindStubAtEntryRecoversTheRawSenderPc) { HotspotStackFrame frame(&_uc); @@ -149,8 +148,7 @@ TEST_F(UnwindHelperContractTest, UnwindStubAtEntryRecoversTheRawSenderPc) { // The property that matters to a caller, stated without reference to which // architecture this is: whatever the helper returns is a raw return address, // so the caller can apply the attribution adjustment itself and be right -// everywhere. Before the contract was unified this could not be written -- -// the answer depended on the target. +// everywhere. TEST_F(UnwindHelperContractTest, HelperNeverAppliesTheAdjustmentItself) { HotspotStackFrame frame(&_uc); @@ -164,8 +162,8 @@ TEST_F(UnwindHelperContractTest, HelperNeverAppliesTheAdjustmentItself) { ASSERT_TRUE(frame.unwindStub(code(), "someStub", pc, sp, fp)); // Whichever slot this architecture sources the sender pc from, it comes - // back unmodified. An off-by-one here means someone reintroduced a folded - // adjustment, and walkVM would then subtract a second one. + // back unmodified. An off-by-one here means the helper applied the + // adjustment itself, and walkVM would then subtract a second one. const bool is_raw = (pc == kSenderRa) || (pc == kLinkReg); EXPECT_TRUE(is_raw) << "expected an unmodified return address, got pc=0x" << std::hex << pc @@ -286,9 +284,9 @@ TEST_F(UnwindNMethodHelperContractTest, UnwindPrologueAtEntryRecoversTheRawSende } // One instruction further in, past the push/stp that saved the caller's fp: -// both architectures now source the sender pc from the *second* stack slot and -// pop two. This is the branch where x86_64's folded adjustment used to sit at a -// different slot than its sibling's, so it is worth pinning separately. +// both architectures source the sender pc from the *second* stack slot and pop +// two, a different slot than the branch above reads, so it is pinned +// separately. TEST_F(UnwindNMethodHelperContractTest, UnwindPrologueAfterFrameSetupRecoversTheRawSenderPc) { HotspotStackFrame frame(&_uc);