'Re: : PING: RFR: 8234624: jstack mixed mode should refer DWARF'

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

List:       openjdk-serviceability-dev
Subject:    Re: : PING: RFR: 8234624: jstack mixed mode should refer DWARF
From:       Yasumasa Suenaga <suenaga () oss ! nttdata ! com>
Date:       2020-02-27 5:13:25
Message-ID: b2d223d2-47db-3119-b579-e48ca3b50469 () oss ! nttdata ! com
[Download RAW message or body]

Hi all,

webrev.03 cannot be applied to current jdk/jdk due to 8239224 and 8239462 changes \
(they updated copyright year). So I modified webrev (only copyright year changes) to \
be able to apply to current jdk/jdk. Could you review it?

   http://cr.openjdk.java.net/~ysuenaga/JDK-8234624/webrev.04/

I need one more reviewer to push.


Thanks,

Yasumasa


On 2020/02/17 13:07, Yasumasa Suenaga wrote:
> 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/
> 
> This change has been already reviewed by Serguei.
> I need one more reviewer to push.
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
> On 2020/02/03 1:37, Yasumasa Suenaga wrote:
> > PING: Could you reveiw this change?
> > 
> > > JBS: https://bugs.openjdk.java.net/browse/JDK-8234624
> > > webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8234624/webrev.03/
> > 
> > I believe this change helps troubleshooter to fight to postmortem analysis.
> > 
> > 
> > Thanks,
> > 
> > Yasumasa
> > 
> > 
> > On 2020/01/19 3:16, Yasumasa Suenaga wrote:
> > > 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.a \
> > > > > gent/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.a \
> > > > > gent/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
> > > > > > > 
> > > > > 


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic