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
