[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8234624: jstack mixed mode should refer DWARF
From:       Yasumasa Suenaga <suenaga () oss ! nttdata ! com>
Date:       2019-12-15 1:51:36
Message-ID: e819c572-2172-c3e2-9933-f3859662d400 () oss ! nttdata ! com
[Download RAW message or body]

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@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/sha \
> re/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
> > > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]