Suggested improvement to X86Frame.getInterpreterFrameBCI
JC Beyler
jcbeyler at google.com
Fri Nov 30 17:06:14 UTC 2018
Hi both,
The webrev looks good to me but I could see gains of just adding a new
constructor instead of doing a new + set.
LinuxAMD64JavaThreadPDAccess.java would become consistent with the rest of
the code:
+ } else if (VM.getVM().getInterpreter().contains(guesser.getPC())) {
+ // pass the value of R13 which contains the bcp for the top level
frame
+ return new X86Frame(guesser.getSP(), guesser.getFP(),
guesser.getPC(),
+ context.getRegisterAsAddress(AMD64ThreadContext.R13));
} else {
- And for X86Frame.java, that means there is no set method (there isn't a
single one yet there so we are consistent there).
- Finally, instead of just r13 internally to the Frame, we could just call
it the bcp since that is what you are saying it is and then adapt the
getInterpreterFrameBCI a bit because a bcp local variable is there :-)
But these are nits :),
Jc
On Fri, Nov 30, 2018 at 6:21 AM Jini George <jini.george at oracle.com> wrote:
> 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
> > >
> >
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181130/a0d88d83/attachment-0001.html>
More information about the serviceability-dev
mailing list