Suggested improvement to X86Frame.getInterpreterFrameBCI

[Jini George]

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

Thanks,
Jini.

On 11/22/2018 5:42 PM, David Griffiths wrote:
> 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 
> <mailto: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>
>      > <mailto: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>
>      >     <mailto: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> <mailto: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
>      >
>