Discussion:
Suggested improvement to X86Frame.getInterpreterFrameBCI
(too old to reply)
David Griffiths
2018-11-21 17:15:57 UTC
Permalink
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
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: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 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
On Wed, Nov 21, 2018 at 9:17 AM David Griffiths <
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
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();
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.
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
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
so
Post by David Griffiths
Post by David Griffiths
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
Post by David Griffiths
Post by David Griffiths
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
Post by David Griffiths
Post by David Griffiths
- 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
Post by David Griffiths
Post by David Griffiths
easy to assess this but seeing it would make things
easier to
Post by David Griffiths
Post by David Griffiths
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
Post by David Griffiths
Post by David Griffiths
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
Post by David Griffiths
Post by David Griffiths
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
--
Thanks,
Jc
David Griffiths
2018-12-03 09:35:58 UTC
Permalink
Ok great, I will submit new patch then. Thanks for reviewing!

Cheers,

David
Post by JC Beyler
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
+ } 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.
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
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
so
Post by David Griffiths
Post by David Griffiths
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
Post by David Griffiths
Post by David Griffiths
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
Post by David Griffiths
Post by David Griffiths
- 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
Post by David Griffiths
Post by David Griffiths
easy to assess this but seeing it would make things
easier to
Post by David Griffiths
Post by David Griffiths
determine.
That being said, I'm not a reviewer for OpenJDK so this
is
Post by David Griffiths
Post by David Griffiths
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
Post by David Griffiths
Post by David Griffiths
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
Post by David Griffiths
Post by David Griffiths
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.
Post by David Griffiths
Post by David Griffiths
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
--
Thanks,
Jc
David Griffiths
2018-12-05 11:09:54 UTC
Permalink
Hi, thanks for reviewing. I have made the changes you suggested and also
tidied up the constructors a bit (there was already a 4x Address
constructor), hope that's ok.

Cheers,

David
Post by JC Beyler
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
+ } 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.
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
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
so
Post by David Griffiths
Post by David Griffiths
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
Post by David Griffiths
Post by David Griffiths
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
Post by David Griffiths
Post by David Griffiths
- 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
Post by David Griffiths
Post by David Griffiths
easy to assess this but seeing it would make things
easier to
Post by David Griffiths
Post by David Griffiths
determine.
That being said, I'm not a reviewer for OpenJDK so this
is
Post by David Griffiths
Post by David Griffiths
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
Post by David Griffiths
Post by David Griffiths
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
Post by David Griffiths
Post by David Griffiths
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.
Post by David Griffiths
Post by David Griffiths
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
--
Thanks,
Jc
Loading...