On Fri, 23 Oct 2026 02:24:47 GMT, Long Yang <[email protected]> wrote:
>> Hi, could I have a review of this patch. This patch fixes >> [JDK-8391637](https://bugs.openjdk.org/browse/JDK-8391637). >> >> Due to my GitHub account settings (Enable Copilot Code Review for all my >> PRs), my previous [PR](https://github.com/openjdk/jdk/pull/33358) had >> Copilot Review enabled by default. I have disabled it and now open this new >> PR, with no changes made to the code. >> >> On `risc-v`, >> [LinuxRISCV64CFrame.sender()](https://github.com/openjdk/jdk/blob/c368bac40e185398fbc40dc4d75a457f7a36f954/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/riscv64/LinuxRISCV64CFrame.java#L70) >> validates only FP alignment, then dereferences FP for SP, link, and return >> address. An aligned arbitrary FP (such as 0x90) can therefore cause >> UnmappedAddressException, and lead to incomplete stack for the target thread. >> >> `aarch64` and `amdf64`already use `dwarf` to unwind native frames, see >> https://github.com/openjdk/jdk/blob/c368bac40e185398fbc40dc4d75a457f7a36f954/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/amd64/LinuxAMD64CFrame.java#L36 >> https://github.com/openjdk/jdk/blob/c368bac40e185398fbc40dc4d75a457f7a36f954/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/aarch64/LinuxAARCH64CFrame.java#L38 >> >> This patch makes `risc-v` to unwind with `dwarf` too. >> >> The `risc-v` version basically has the same design as the `aarch64` version. >> There are also several differences , primarily due to differences in calling >> conventions. Almost every difference is accompanied by an explanatory >> comment. >> >> I have ran tests in `test/hotspot/jtreg/serviceability/sa` on `risc-v` and >> `amd64`, all passed. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Long Yang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains nine commits: > > - Merge branch 'master' into fix-8391637 > - Enable TestJhsdbJstackMixedWithXComp.java on riscv64 while excluding QEMU > environments > - Enable -fasynchronous-unwind-tables for linux-riscv64 > - Add try/catch for memory-read exceptions in getSenderFP > - RISC-V: Use compiler-defined macros for DWARF register selection > - Recover caller return addresses across save helpers > - Use the CIE return-address column instead of the hard-coded RA register > - Add comment > - 8391637: On the RISC-V platform, TestJhsdbJstackMixed has a certain > probability of failure The attached patch uses PC−1 consistently for native caller lookups on x86_64 and AArch64, rather than only retrying when DWARF parsing fails. It preserves the actual frame PC and leaves top-frame, signal-context, and Java code-cache PCs unchanged. Could you please take a look? diff --git a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/aarch64/LinuxAARCH64CFrame.java b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/aarch64/LinuxAARCH64CFrame.java index f3a1d2d80a2..5fdffd7df64 100644 --- a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/aarch64/LinuxAARCH64CFrame.java +++ b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/aarch64/LinuxAARCH64CFrame.java @@ -186,41 +186,36 @@ public CFrame sender(ThreadProxy thread, Address senderSP, Address senderFP, Add senderPC = stripPAC(senderPC); } + boolean use1ByteBeforeToLookup = VM.getVM().getCodeCache().findBlobUnsafe(senderPC) == null + && !linuxDbg().isSignalTrampoline(senderPC); + Address lookupPC = use1ByteBeforeToLookup ? senderPC.addOffsetTo(-1) : senderPC; DwarfParser senderDwarf = null; - boolean fallback = false; try { - senderDwarf = createDwarfParser(linuxDbg(), senderPC); + senderDwarf = createDwarfParser(linuxDbg(), lookupPC); } catch (DebuggerException _) { - // Try again with PC-1 in case PC is just outside function bounds, - // due to function ending with a `call` instruction. - try { - senderDwarf = createDwarfParser(linuxDbg(), senderPC.addOffsetTo(-1)); - fallback = true; - } catch (DebuggerException _) { - if (linuxDbg().isSignalTrampoline(senderPC)) { - // We can use the caller frame if it is a signal trampoline. - // DWARF processing might fail because vdso.so .eh_frame is not required on aarch64. - return new LinuxAARCH64CFrame(linuxDbg(), senderSP, senderFP, null, senderPC, senderDwarf); - } - - // Returns CFrame if the sender is native frame even though it does not have DWARF, - // otherwise returns null because we cannot unwind anymore. - return linuxDbg().findLibPtrByAddress(senderPC) != null - ? new LinuxAARCH64CFrame(linuxDbg(), senderSP, senderFP, null /* no CFA */, senderPC, null /* no DWARF */) - : null; + if (linuxDbg().isSignalTrampoline(senderPC)) { + // We can use the caller frame if it is a signal trampoline. + // DWARF processing might fail because vdso.so .eh_frame is not required on aarch64. + return new LinuxAARCH64CFrame(linuxDbg(), senderSP, senderFP, null, senderPC, senderDwarf); } + + // Returns CFrame if the sender is native frame even though it does not have DWARF, + // otherwise returns null because we cannot unwind anymore. + return linuxDbg().findLibPtrByAddress(lookupPC) != null + ? new LinuxAARCH64CFrame(linuxDbg(), senderSP, senderFP, null /* no CFA */, senderPC, null /* no DWARF */, use1ByteBeforeToLookup) + : null; } try { Address senderCFA = getSenderCFA(senderDwarf, senderSP, senderFP); return isValidFrame(senderCFA, senderFP) - ? new LinuxAARCH64CFrame(linuxDbg(), senderSP, senderFP, senderCFA, senderPC, senderDwarf, fallback) + ? new LinuxAARCH64CFrame(linuxDbg(), senderSP, senderFP, senderCFA, senderPC, senderDwarf, use1ByteBeforeToLookup) : null; } catch (DebuggerException e) { if (linuxDbg().isSignalTrampoline(senderPC)) { // We can use the caller frame if it is a signal trampoline. // getSenderCFA() might fail because DwarfParser cannot find out CFA register. - return new LinuxAARCH64CFrame(linuxDbg(), senderSP, senderFP, null, senderPC, senderDwarf, fallback); + return new LinuxAARCH64CFrame(linuxDbg(), senderSP, senderFP, null, senderPC, senderDwarf, use1ByteBeforeToLookup); } // Rethrow the original exception if getSenderCFA() failed diff --git a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/amd64/LinuxAMD64CFrame.java b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/amd64/LinuxAMD64CFrame.java index b9ab97e3cef..2943815af8f 100644 --- a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/amd64/LinuxAMD64CFrame.java +++ b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/amd64/LinuxAMD64CFrame.java @@ -109,35 +109,30 @@ public CFrame sender(ThreadProxy th, Address senderSP, Address senderFP, Address senderFP = getSenderFP(senderFP); + boolean use1ByteBeforeToLookup = VM.getVM().getCodeCache().findBlobUnsafe(senderPC) == null + && !linuxDbg().isSignalTrampoline(senderPC); + Address lookupPC = use1ByteBeforeToLookup ? senderPC.addOffsetTo(-1) : senderPC; DwarfParser senderDwarf = null; - boolean fallback = false; try { - senderDwarf = createDwarfParser(linuxDbg(), senderPC); + senderDwarf = createDwarfParser(linuxDbg(), lookupPC); } catch (DebuggerException _) { - // Try again with PC-1 in case PC is just outside function bounds, - // due to function ending with a `call` instruction. - try { - senderDwarf = createDwarfParser(linuxDbg(), senderPC.addOffsetTo(-1)); - fallback = true; - } catch (DebuggerException _) { - // Returns CFrame if the sender is native frame even though it does not have DWARF, - // otherwise returns null because we cannot unwind anymore. - return linuxDbg().findLibPtrByAddress(senderPC) != null - ? new LinuxAMD64CFrame(linuxDbg(), senderSP, senderFP, null /* no CFA */, senderPC, null /* no DWARF */) - : null; - } + // Returns CFrame if the sender is native frame even though it does not have DWARF, + // otherwise returns null because we cannot unwind anymore. + return linuxDbg().findLibPtrByAddress(lookupPC) != null + ? new LinuxAMD64CFrame(linuxDbg(), senderSP, senderFP, null /* no CFA */, senderPC, null /* no DWARF */, use1ByteBeforeToLookup) + : null; } try { Address senderCFA = getSenderCFA(senderDwarf, senderSP, senderFP); return isValidFrame(senderCFA, senderFP, senderSP) - ? new LinuxAMD64CFrame(linuxDbg(), senderSP, senderFP, senderCFA, senderPC, senderDwarf, fallback) + ? new LinuxAMD64CFrame(linuxDbg(), senderSP, senderFP, senderCFA, senderPC, senderDwarf, use1ByteBeforeToLookup) : null; } catch (DebuggerException e) { if (linuxDbg().isSignalTrampoline(senderPC)) { // We can use the caller frame if it is a signal trampoline. // getSenderCFA() might fail because DwarfParser cannot find out CFA register. - return new LinuxAMD64CFrame(linuxDbg(), senderSP, senderFP, null, senderPC, senderDwarf, fallback); + return new LinuxAMD64CFrame(linuxDbg(), senderSP, senderFP, null, senderPC, senderDwarf, use1ByteBeforeToLookup); } // Rethrow the original exception if getSenderCFA() failed ------------- PR Comment: https://git.openjdk.org/jdk/pull/33374#issuecomment-6356737835
