Suggested improvement to X86Frame.getInterpreterFrameBCI

Discussion:

David Griffiths

2018-11-21 17:15:57 UTC

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

JC Beyler

2018-11-21 19:01:14 UTC

Permalink

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

Post by David Griffiths
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
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

David Griffiths

2018-11-21 19:15:11 UTC

Permalink

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

Post by JC Beyler
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
- 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

--
Thanks,
Jc

David Griffiths

2018-11-21 19:20:06 UTC

Permalink

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.

Post by David Griffiths
Hi, thanks, apart from adding a setter for R13 in X86Frame, the other half
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

Post by David Griffiths
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
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

Jini George

2018-11-22 09:02:20 UTC

Permalink

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

Post by David Griffiths
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.
Hi, thanks, apart from adding a setter for R13 in X86Frame, the
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
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
- 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
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
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

David Griffiths

2018-11-22 12:12:24 UTC

Permalink

Thanks Jini, please find patch for Java 9 attached (I don't have author
access to the bug itself).

Cheers,

David

Post by Jini George
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).
https://bugs.openjdk.java.net/browse/JDK-8214226
Thank you,
Jini

t.getContext();

Post by David Griffiths
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())) {

Post by David Griffiths
// 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
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
- 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
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
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

Jini George

2018-11-30 14:21:01 UTC

Permalink

Your patch looks good to me, David. I can sponsor this for you if we get
one more review.

Thanks,
Jini.

Post by David Griffiths
Thanks Jini, please find patch for Java 9 attached (I don't have author
access to the bug itself).
Cheers,
David
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).
https://bugs.openjdk.java.net/browse/JDK-8214226
Thank you,
Jini

Post by David Griffiths
PS: should have added a new X86Frame constructor really, may have

just

Post by David Griffiths
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
Hi, thanks, apart from adding a setter for R13 in X86Frame, the
public Frame getCurrentFrameGuess(JavaThread thread,

Address

Post by David Griffiths
addr) {
ThreadProxy t = getThreadProxy(addr);
AMD64ThreadContext context = (AMD64ThreadContext)

t.getContext();

Post by David Griffiths
    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())) {

Post by David Griffiths
// pass the value of R13 which contains the bcp for

the top

Post by David Griffiths
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.

Post by David Griffiths
Can't think of any risk but I'm not an expert on this code.
Cheers,
David
Hi David,
I think the easiest would be to see whole change to

understand

Post by David Griffiths
the repercussions of the change. I would imagine that any

change

Post by David Griffiths
that helps stacktraces being more precise is a good thing but
- 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
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

Post by David Griffiths
paused in flight JVM (we can't use JDWP). I have observed
that if the top frame is in the interpreter it

reports the

Post by David Griffiths
BCI and line number incorrectly. This is because
X86Frame.getInterpreterFrameBCI uses the value stored

on the

Post by David Griffiths
stack rather than the actual live value stored in R13.
I have a patch for this which lets
LinuxAMD64JavaThreadPDAccess.getCurrentFrameGuess

pass the

Post by David Griffiths
public int getInterpreterFrameBCI() {
Address bcp =
addressOfInterpreterFrameBCX().getAddressAt(0);
// If we are in the top level frame then R13 may

have

Post by David Griffiths
been set for us which contains
// the BCP. If so then let it take priority. If

we are

Post by David Griffiths
in a top level interpreter frame,
// the BCP is live in R13 (on x86) and not saved

in the

Post by David Griffiths
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

Post by David Griffiths
patch?
Cheers,
David
--
Thanks,
Jc

JC Beyler

2018-11-30 17:06:14 UTC

Permalink

Hi both,

The webrev looks good to me but I could see gains of just adding a new
constructor instead of doing a new + set.

LinuxAMD64JavaThreadPDAccess.java would become consistent with the rest of
the code:
+ } else if (VM.getVM().getInterpreter().contains(guesser.getPC())) {
+ // pass the value of R13 which contains the bcp for the top level
frame
+ return new X86Frame(guesser.getSP(), guesser.getFP(),
guesser.getPC(),
+ context.getRegisterAsAddress(AMD64ThreadContext.R13));
} else {

- And for X86Frame.java, that means there is no set method (there isn't a
single one yet there so we are consistent there).
- Finally, instead of just r13 internally to the Frame, we could just call
it the bcp since that is what you are saying it is and then adapt the
getInterpreterFrameBCI a bit because a bcp local variable is there :-)

But these are nits :),
Jc

Post by Jini George
Your patch looks good to me, David. I can sponsor this for you if we get
one more review.
Thanks,
Jini.

(With

Post by David Griffiths
a first cut look, there seems to be missing pieces).
https://bugs.openjdk.java.net/browse/JDK-8214226
Thank you,
Jini

Post by David Griffiths
PS: should have added a new X86Frame constructor really, may have

just

Post by David Griffiths
been put off because there is already a four address constructor