Suggested improvement to X86Frame.getInterpreterFrameBCI

[David Griffiths]

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

Cheers,

David

On Thu, 22 Nov 2018 at 09:02, Jini George <jini.george at oracle.com> wrote:

> 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.griffiths at gmail.com
> > <mailto:david.griffiths at 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 <jcbeyler at google.com
> >     <mailto:jcbeyler at 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.griffiths at gmail.com <mailto:david.griffiths at 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181122/e053b746/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: interpreter_frame.patch
Type: application/octet-stream
Size: 2610 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181122/e053b746/interpreter_frame.patch>