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.geo...@oracle.com
<mailto:jini.geo...@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.griffi...@gmail.com <mailto:david.griffi...@gmail.com>
> <mailto:david.griffi...@gmail.com
<mailto: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
<mailto:jcbey...@google.com>
> <mailto:jcbey...@google.com <mailto: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
<mailto:david.griffi...@gmail.com> <mailto:david.griffi...@gmail.com
<mailto: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
>
