Suggested improvement to X86Frame.getInterpreterFrameBCI

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

On Wed, 21 Nov 2018 at 19:15, David Griffiths <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> 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> 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/20181121/f5a106cb/attachment.html>