Suggested improvement to X86Frame.getInterpreterFrameBCI

[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).

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
>