Thank you very much for working on the fix for this issue, David. It
would be great if you can send in a complete patch for the review (With
a first cut look, there seems to be missing pieces).
I have created a bug for this:
https://bugs.openjdk.java.net/browse/JDK-8214226
Thank you,
Jini
On 11/22/2018 12:50 AM, David Griffiths wrote:
PS: should have added a new X86Frame constructor really, may have just
been put off because there is already a four address constructor so
would have had to add dummy argument or something.
On Wed, 21 Nov 2018 at 19:15, David Griffiths <david.griffi...@gmail.com
<mailto:david.griffi...@gmail.com>> wrote:
Hi, thanks, apart from adding a setter for R13 in X86Frame, the
other half of the fix is this:
public Frame getCurrentFrameGuess(JavaThread thread, Address
addr) {
ThreadProxy t = getThreadProxy(addr);
AMD64ThreadContext context = (AMD64ThreadContext) t.getContext();
AMD64CurrentFrameGuess guesser = new
AMD64CurrentFrameGuess(context, thread);
if (!guesser.run(GUESS_SCAN_RANGE)) {
return null;
}
if (guesser.getPC() == null) {
return new X86Frame(guesser.getSP(), guesser.getFP());
} else if (VM.getVM().getInterpreter().contains(guesser.getPC())) {
// pass the value of R13 which contains the bcp for the top
level frame
Address r13 =
context.getRegisterAsAddress(AMD64ThreadContext.R13);
X86Frame frame = new X86Frame(guesser.getSP(),
guesser.getFP(), guesser.getPC());
frame.setR13(r13);
return frame;
} else {
return new X86Frame(guesser.getSP(), guesser.getFP(),
guesser.getPC());
}
}
(the whole "if pc in interpreter" block is new)
Overhead likely to be low as this is only used in diagnostic code.
Can't think of any risk but I'm not an expert on this code.
Cheers,
David
On Wed, 21 Nov 2018 at 19:01, JC Beyler <jcbey...@google.com
<mailto:jcbey...@google.com>> wrote:
Hi David,
I think the easiest would be to see whole change to understand
the repercussions of the change. I would imagine that any change
that helps stacktraces being more precise is a good thing but
there are questions that arise every time:
- What is the overhead of adding this?
- Does this add any risk of failure?
I'd imagine that the change is relatively small and should be
easy to assess this but seeing it would make things easier to
determine.
That being said, I'm not a reviewer for OpenJDK so this is
really just my 2cents,
Jc
On Wed, Nov 21, 2018 at 9:17 AM David Griffiths
<david.griffi...@gmail.com <mailto:david.griffi...@gmail.com>>
wrote:
Hi, I'm new to this mailing list and working on a project
that makes use of the SA classes to get stack traces from a
paused in flight JVM (we can't use JDWP). I have observed
that if the top frame is in the interpreter it reports the
BCI and line number incorrectly. This is because
X86Frame.getInterpreterFrameBCI uses the value stored on the
stack rather than the actual live value stored in R13.
I have a patch for this which lets
LinuxAMD64JavaThreadPDAccess.getCurrentFrameGuess pass the
R13 value to X86Frame so that the latter can then do:
public int getInterpreterFrameBCI() {
Address bcp =
addressOfInterpreterFrameBCX().getAddressAt(0);
// If we are in the top level frame then R13 may have
been set for us which contains
// the BCP. If so then let it take priority. If we are
in a top level interpreter frame,
// the BCP is live in R13 (on x86) and not saved in the
BCX stack slot.
if (r13 != null) {
bcp = r13;
}
Address methodHandle =
addressOfInterpreterFrameMethod().getAddressAt(0);
and this fixes the problem.
Does this sound like a good idea and if so should I submit a
patch?
Cheers,
David
--
Thanks,
Jc
