diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index b9adcd52af..1e37a6ef3c 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -235,23 +235,67 @@ 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) { +// 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 +// 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) { - return walkVM(&empty_ucontext, frames, max_depth, features, event_type, - callerPC(), (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); + 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, 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(); @@ -324,7 +368,22 @@ __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 + // 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; @@ -422,7 +481,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. + walk_pc.setReturnAddress(carrier_pc); return true; }; @@ -430,7 +490,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); @@ -455,7 +515,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. @@ -463,17 +523,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 @@ -518,9 +578,14 @@ __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 + const void* anchor_pc = walk_pc.raw(); + if (anchor->getFrame(anchor_pc, sp, fp)) { + // getFrame() redirects pc to lastJavaPC(), a return address. + walk_pc.setReturnAddress(anchor_pc); + if (!nm->contains(walk_pc.raw())) { + anchor = NULL; + continue; // NMethod has changed as a result of correction + } } anchor = NULL; } else if (anchor_eligible && cont_unwind_active) { @@ -546,7 +611,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]); + walk_pc.setReturnAddress(stripPointer(((void**)fp)[FRAME_PC_SLOT])); fp = *(uintptr_t*)INJECT_FAULT_ADDRESS_UNLIKELY(fp); continue; } @@ -560,11 +625,11 @@ __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(); } continue; @@ -598,12 +663,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)) { + 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); continue; } - int scope_offset = nm->findScopeOffset(pc); + int scope_offset = nm->findScopeOffset(walk_pc.raw()); if (scope_offset > 0) { depth--; ScopeDesc scope(nm); @@ -620,7 +687,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(); @@ -638,26 +705,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. + walk_pc.setReturnAddress(((const void**)sp)[-FRAME_PC_SLOT]); continue; - } else if (frame.unwindPrologue(nm, (uintptr_t&)pc, sp, fp)) { + } else if (const void* prologue_pc = walk_pc.raw(); + frame.unwindPrologue(nm, (uintptr_t&)prologue_pc, sp, fp)) { + recordUnwoundPc(walk_pc, 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. + walk_pc.setReturnAddress(entry_frame_pc); if (sp < prev_sp || sp >= bottom || !aligned(sp)) { fillFrame(frames[depth++], BCI_ERROR, "break_entry_frame"); break; @@ -683,7 +756,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(); @@ -691,7 +764,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)) { + 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); continue; } @@ -713,7 +788,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. + walk_pc.setReturnAddress(((const void**)sp)[-FRAME_PC_SLOT]); continue; } @@ -732,7 +808,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)walk_pc.raw(), walk_pc.isReturnAddress(), lock_index); if (resolution.is_marked()) { if (resolution.mark == MARK_JAVA_PROFILER && isHookPrefixedSample(event_type)) { @@ -764,7 +840,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(walk_pc.attribution()) == NULL) { // Try anchor recovery — prefer live anchor, fall back to saved data anchor_recovery_used = true; const void* recovery_pc = NULL; @@ -800,7 +876,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]"); } @@ -810,7 +886,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]); + walk_pc.setReturnAddress(stripPointer(((void**)recovery_fp)[FRAME_PC_SLOT])); fp = *(uintptr_t*)recovery_fp; continue; } @@ -819,18 +895,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; - if (pc != NULL && !CodeHeap::contains(pc) && sp != 0 && aligned(sp) && sp < bottom) { - pc = ((const void**)sp)[-1]; + // lastJavaPC() records where the Java frame resumes, i.e. a + // return address; so is the sp[-1] slot read just below. + 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]"); } @@ -841,8 +920,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, 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) { @@ -868,17 +948,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 = walk_pc.attribution(); + 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 +980,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 @@ -915,9 +996,15 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex } // store the previous pc before unwinding - prev_native_pc = pc; + prev_native_walk_pc = walk_pc; + have_prev_native_pc = true; if (f.fp_off & DW_PC_OFFSET) { - pc = (const char*)pc + (f.fp_off >> 1); + // 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. + 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)); @@ -929,9 +1016,10 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex if (!aligned(pc_addr)) { break; } - pc = stripPointer(SafeAccess::load((void**)pc_addr)); + walk_pc.setRecoveredPc(stripPointer(SafeAccess::load((void**)pc_addr)), + f.isSignalFrame()); } else if (depth == 1) { - pc = (const void*)frame.link(); + walk_pc.setRecoveredPc((const void*)frame.link(), f.isSignalFrame()); } else { break; } @@ -945,7 +1033,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; } } @@ -974,7 +1063,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]); + 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; @@ -983,9 +1072,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)) { - if (!CodeHeap::contains(pc) && sp != 0 && aligned(sp) && sp < bottom) { - pc = ((const void**)sp)[-1]; + 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. + 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/libraries.h b/ddprof-lib/src/main/cpp/libraries.h index 18b59c9631..a7045a934c 100644 --- a/ddprof-lib/src/main/cpp/libraries.h +++ b/ddprof-lib/src/main/cpp/libraries.h @@ -82,6 +82,15 @@ class Libraries { return _native_libs; } +#ifdef UNIT_TEST + // Publishes a caller-owned CodeCache into the process-wide set so address + // 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; Libraries& operator=(const Libraries&) = delete; 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); 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) { 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 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)