RFR: 8234624: jstack mixed mode should refer DWARF
Yasumasa Suenaga
suenaga at oss.nttdata.com
Sun Dec 15 01:51:36 UTC 2019
Hi Serguei,
Thanks for your comment!
I refactored LinuxCDebugger and LinuxAMD64CFrame in new webrev.
Also I fixed to free lib->eh_frame.data in libproc_impl.c as Dmitry said.
http://cr.openjdk.java.net/~ysuenaga/JDK-8234624/webrev.02/
This change has been passed all tests on submit repo (mach5-one-ysuenaga-JDK-8234624-3-20191214-1527-7538487).
Thanks,
Yasumasa
On 2019/12/14 10:02, serguei.spitsyn at oracle.com wrote:
> Hi Yasumasa,
>
> This is nice move in general.
> Thank you for working on this!
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8234624/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/LinuxCDebugger.java.frames.html
>
> 96 long libptr = dbg.findLibPtrByAddress(pc); 97 if (libptr == 0L) { // Java frame 98 Address rbp = context.getRegisterAsAddress(AMD64ThreadContext.RBP); 99 if (rbp == null) { 100 return null; 101 } 102 return new LinuxAMD64CFrame(dbg, rbp, pc, null); 103 } else { // Native frame 104 DwarfParser dwarf; 105 try { 106 dwarf = new DwarfParser(libptr); 107 } catch (DebuggerException e) { 108 Address rbp = context.getRegisterAsAddress(AMD64ThreadContext.RBP); 109 if (rbp == null) { 110 return null; 111 } 112 return new LinuxAMD64CFrame(dbg, rbp, pc, null); 113 } 114 dwarf.processDwarf(pc); 115 Address cfa = ((dwarf.getCFARegister() == AMD64ThreadContext.RBP) && 116 !dwarf.isBPOffsetAvailable()) 117 ? context.getRegisterAsAddress(AMD64ThreadContext.RBP) 118 : context.getRegisterAsAddress(dwarf.getCFARegister()) 119 .addOffsetTo(dwarf.getCFAOffset()); 120 if (cfa == null) { 121 return null; 122 } 123 return new LinuxAMD64CFrame(dbg, cfa, pc, dwarf); 124 }
>
>
> I'd suggest to simplify the logic by refactoring to something like below:
>
> long libptr = dbg.findLibPtrByAddress(pc);
> Address cfa = context.getRegisterAsAddress(AMD64ThreadContext.RBP); // Java frame
> DwarfParser dwarf = null;
>
> if (libptr != 0L) { // Native frame
> try {
> dwarf = new DwarfParser(libptr);
> dwarf.processDwarf(pc);
> Address cfa = ((dwarf.getCFARegister() == AMD64ThreadContext.RBP) &&
> !dwarf.isBPOffsetAvailable())
> ? context.getRegisterAsAddress(AMD64ThreadContext.RBP)
> : context.getRegisterAsAddress(dwarf.getCFARegister())
> .addOffsetTo(dwarf.getCFAOffset());
>
> } catch (DebuggerException e) { // bail out to Java frame case
> }
> }
> if (cfa == null) {
> return null;
> }
> return new LinuxAMD64CFrame(dbg, cfa, pc, dwarf);
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8234624/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/amd64/LinuxAMD64CFrame.java.frames.html
>
> 58 long ofs = useDwarf ? dwarf.getReturnAddressOffsetFromCFA()
>
> Better to rename 'ofs' => 'offs'.
>
> 77 nextCFA = nextCFA.addOffsetTo(- nextDwarf.getBasePointerOffsetFromCFA());
>
> Extra space after '-' sign.
>
> 71 private Address getNextCFA(DwarfParser nextDwarf, ThreadContext context) {
>
> It feels like the logic has to be somehow refactored/simplified as
> several typical fragments appears in slightly different contexts.
> But it is not easy to understand what it is.
> Could you, please, add some comments to key places explaining this logic.
> Then I'll check if it is possible to make it a little bit simpler.
>
> 109 private CFrame javaSender(ThreadContext context) { 110 Address nextCFA; 111 Address nextPC; 112 113 nextPC = getNextPC(false); 114 if (nextPC == null) { 115 return null; 116 } 117 118 DwarfParser nextDwarf = null; 119 long libptr = dbg.findLibPtrByAddress(nextPC); 120 if (libptr != 0L) { // Native frame 121 try { 122 nextDwarf = new DwarfParser(libptr); 123 } catch (DebuggerException e) { 124 nextCFA = getNextCFA(null, context); 125 return (nextCFA == null) ? null : new LinuxAMD64CFrame(dbg, nextCFA, nextPC, null); 126 } 127 nextDwarf.processDwarf(nextPC); 128 } 129 130 nextCFA = getNextCFA(nextDwarf, context); 131 return (nextCFA == null) ? null 132 : new LinuxAMD64CFrame(dbg, nextCFA, nextPC, nextDwarf); 133 }
>
> The above can be simplified if a DebuggerException can not be thrown from processDwarf(nextPC):
> private CFrame javaSender(ThreadContext context) {
> Address nextPC = getNextPC(false);
> if (nextPC == null) {
> return null;
> }
> long libptr = dbg.findLibPtrByAddress(nextPC);
> DwarfParser nextDwarf = null;
>
> if (libptr != 0L) { // Native frame
> try {
> nextDwarf = new DwarfParser(libptr);
> nextDwarf.processDwarf(nextPC);
> } catch (DebuggerException e) { // Bail out to Java frame
> }
> }
> Address nextCFA = getNextCFA(nextDwarf, context);
> return (nextCFA == null) ? null : new LinuxAMD64CFrame(dbg, nextCFA, nextPC, nextDwarf);
> }
>
> 135 public CFrame sender(ThreadProxy thread) { 136 ThreadContext context = thread.getContext(); 137 138 if (dwarf == null) { // Java frame 139 return javaSender(context); 140 } 141 142 Address nextPC = getNextPC(true); 143 if (nextPC == null) { 144 return null; 145 } 146 147 Address nextCFA; 148 DwarfParser nextDwarf = dwarf; 149 if (!dwarf.isIn(nextPC)) { 150 long libptr = dbg.findLibPtrByAddress(nextPC); 151 if (libptr == 0L) { 152 // Next frame might be Java frame 153 nextCFA = getNextCFA(null, context); 154 return (nextCFA == null) ? null : new LinuxAMD64CFrame(dbg, nextCFA, nextPC, null); 155 } 156 try { 157 nextDwarf = new DwarfParser(libptr); 158 } catch (DebuggerException e) { 159 nextCFA = getNextCFA(null, context); 160 return (nextCFA == null) ? null : new LinuxAMD64CFrame(dbg, nextCFA, nextPC, null); 161 } 162 } 163 164 nextDwarf.processDwarf(nextPC); 165 nextCFA = getNextCFA(nextDwarf, context); 166 return (nextCFA == null) ? null : new LinuxAMD64CFrame(dbg,
> nextCFA, nextPC, nextDwarf); 167 }
>
> This one can be also simplified a little:
>
> public CFrame sender(ThreadProxy thread) {
> ThreadContext context = thread.getContext();
>
> if (dwarf == null) { // Java frame
> return javaSender(context);
> }
> Address nextPC = getNextPC(true);
> if (nextPC == null) {
> return null;
> }
> DwarfParser nextDwarf = null;
> if (!dwarf.isIn(nextPC)) {
> long libptr = dbg.findLibPtrByAddress(nextPC);
> if (libptr != 0L) {
> try {
> nextDwarf = new DwarfParser(libptr);
> nextDwarf.processDwarf(nextPC);
> } catch (DebuggerException e) { // Bail out to Java frame
> }
> }
> }
> Address nextCFA = getNextCFA(nextDwarf, context);
> return (nextCFA == null) ? null : new LinuxAMD64CFrame(dbg, nextCFA, nextPC, nextDwarf);
> }
>
> Finally, it looks like just one method could replace both
> sender(ThreadProxy thread) and javaSender(ThreadContext context):
>
> private CFrame commonSender(ThreadProxy thread) {
> ThreadContext context = thread.getContext();
> Address nextPC = getNextPC(false);
> if (nextPC == null) {
> return null;
> }
> DwarfParser nextDwarf = null;
>
> long libptr = dbg.findLibPtrByAddress(nextPC);
> if (dwarf == null || !dwarf.isIn(nextPC)) {
> long libptr = dbg.findLibPtrByAddress(nextPC);
> if (libptr != 0L) {
> try {
> nextDwarf = new DwarfParser(libptr);
> nextDwarf.processDwarf(nextPC);
> } catch (DebuggerException e) { // Bail out to Java frame
> }
> }
> }
> Address nextCFA = getNextCFA(nextDwarf, context);
> return (nextCFA == null) ? null : new LinuxAMD64CFrame(dbg, nextCFA, nextPC, nextDwarf);
> }
>
> I'm still reviewing the dwarf parser files.
>
> Thanks,
> Serguei
>
>
> On 11/28/19 4:39 AM, Yasumasa Suenaga wrote:
>> Hi,
>>
>> I refactored LinuxAMD64CFrame.java . It works fine in serviceability/sa tests and
>> all tests on submit repo (mach5-one-ysuenaga-JDK-8234624-2-20191128-0928-7059923).
>> Could you review new webrev?
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8234624/webrev.01/
>>
>> The diff from previous webrev is here:
>> http://hg.openjdk.java.net/jdk/submit/rev/4bc47efbc90b
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2019/11/25 14:08, Yasumasa Suenaga wrote:
>>> Hi all,
>>>
>>> Please review this change:
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8234624
>>> webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8234624/webrev.00/
>>>
>>>
>>> According to 2.7 Stack Unwind Algorithm in System V Application Binary Interface AMD64
>>> Architecture Processor Supplement [1], we need to use DWARF in .eh_frame or .debug_frame
>>> for stack unwinding.
>>>
>>> As JDK-8022183 said, omit-frame-pointer is enabled by default since GCC 4.6, so system
>>> library (e.g. libc) might be compiled with this feature.
>>>
>>> However `jhsdb jstack --mixed` does not do so, it uses base pointer register (RBP).
>>> So it might be lack of stack frames.
>>>
>>> I guess JDK-8219201 is caused by same issue.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> [1] https://software.intel.com/sites/default/files/article/402129/mpx-linux64-abi.pdf
>
More information about the serviceability-dev
mailing list