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://urldefense.com/v3/__https://software.intel.com/sites/default/files/article/402129/mpx-linux64-abi.pdf__;!!GqivPVa7Brio!J801oKj34Q7f-4SzAWGKL67e6Xq2yMlV6f01eqp_fqqhqgKktCBiUi2RUaQusmjOqA$
