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

List:       openjdk-serviceability-dev
Subject:    PING: RFR: 8234624: jstack mixed mode should refer DWARF
From:       Yasumasa Suenaga <suenaga () oss ! nttdata ! com>
Date:       2020-01-19 2:16:38
Message-ID: 3ae29ebb-556f-f8c7-c107-61a5d18fce07 () oss ! nttdata ! com
[Download RAW message or body]

PING: Could you review it?

   JBS: https://bugs.openjdk.java.net/browse/JDK-8234624
   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8234624/webrev.03/

I updated webrev. I discussed with Serguei in off list, and I refactored webrev.02 .
It has passed tests on submit repo \
(mach5-one-ysuenaga-JDK-8234624-4-20200118-1353-8149549).


Thanks,

Yasumasa


On 2019/12/15 10:51, Yasumasa Suenaga wrote:
> 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/s \
> > hare/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]