Skip to content

Hand back raw return addresses from the unwind helpers #831

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rkennke wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Hand back raw return addresses from the unwind helpers #831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7c21727
fix(profiling): hand back raw return addresses from the unwind helpers
rkennke Sep 29, 2026
8fa524e
test: cover unwindPrologue/unwindEpilogue with a fake nmethod
rkennke Sep 29, 2026
b449e30
fix(profiling): step into the call for the AsyncGetCallTrace retry on…
rkennke Sep 30, 2026
dd0013f
Merge branch 'main' into fix/prof-16033-helper-contract
rkennke Sep 30, 2026
4ee10c4
docs: state the unwind helper contract on its own terms
rkennke Sep 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 38 additions & 2 deletions ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,40 @@ 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.
//
// 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 passes the raw return address to
// AsyncGetCallTrace unchanged.
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);
Expand All @@ -77,6 +105,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
Expand Down
24 changes: 11 additions & 13 deletions ddprof-lib/src/main/cpp/hotspot/hotspotStackFrame_x64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,21 @@ __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) {
// The stub begins with
// 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;
}
}
Expand Down Expand Up @@ -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];
Comment thread
rkennke marked this conversation as resolved.
Comment thread
rkennke marked this conversation as resolved.
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)) {
Expand All @@ -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)) {
Expand All @@ -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;
}
Expand Down
28 changes: 3 additions & 25 deletions ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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;
}

Expand Down
Loading
Loading