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.griffi...@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 <jcbey...@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.griffi...@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
>>
>
