[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: Suggested improvement to X86Frame.getInterpreterFrameBCI
From: JC Beyler <jcbeyler () google ! com>
Date: 2018-11-30 17:06:14
Message-ID: CAF9BGBwFS6E=CFKWTVMXoRgRKK-DErFbyQpT43yHb8U7EyiNdA () mail ! gmail ! com
[Download RAW message or body]
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@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@oracle.com
> > <mailto:jini.george@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@gmail.com <mailto:david.griffiths@gmail.com>
> > > <mailto:david.griffiths@gmail.com
> > <mailto:david.griffiths@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@google.com
> > <mailto:jcbeyler@google.com>
> > > <mailto:jcbeyler@google.com <mailto:jcbeyler@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@gmail.com
> > <mailto:david.griffiths@gmail.com> <mailto:david.griffiths@gmail.com
> > <mailto:david.griffiths@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
[Attachment #3 (text/html)]
<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">Hi \
both,<div><br></div><div>The webrev looks good to me but I could see gains of just \
adding a new constructor instead of doing a new + \
set.</div><div><br></div><div>LinuxAMD64JavaThreadPDAccess.java would become \
consistent with the rest of the code:<br></div><div><div>+ } else if \
(VM.getVM().getInterpreter().contains(guesser.getPC())) {</div><div>+ // pass \
the value of R13 which contains the bcp for the top level frame</div><div>+ \
return new X86Frame(guesser.getSP(), guesser.getFP(), guesser.getPC(),</div><div>+ \
context.getRegisterAsAddress(AMD64ThreadContext.R13));</div><div> } else \
{<br></div></div><div><br></div><div>- And for X86Frame.java, that means there is no \
set method (there isn't a single one yet there so we are consistent \
there).</div><div>- 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 \
:-)</div><div><br></div><div>But these are nits \
:),</div><div>Jc</div></div></div></div></div></div><br><div class="gmail_quote"><div \
dir="ltr">On Fri, Nov 30, 2018 at 6:21 AM Jini George <<a \
href="mailto:jini.george@oracle.com">jini.george@oracle.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex">Your patch looks good to me, David. \
I can sponsor this for you if we get <br> one more review.<br>
<br>
Thanks,<br>
Jini.<br>
<br>
On 11/22/2018 5:42 PM, David Griffiths wrote:<br>
> Thanks Jini, please find patch for Java 9 attached (I don't have author <br>
> access to the bug itself).<br>
> <br>
> Cheers,<br>
> <br>
> David<br>
> <br>
> On Thu, 22 Nov 2018 at 09:02, Jini George <<a \
href="mailto:jini.george@oracle.com" target="_blank">jini.george@oracle.com</a> <br> \
> <mailto:<a href="mailto:jini.george@oracle.com" \
target="_blank">jini.george@oracle.com</a>>> wrote:<br> > <br>
> Thank you very much for working on the fix for this issue, David. It<br>
> would be great if you can send in a complete patch for the review \
(With<br> > a first cut look, there seems to be missing pieces).<br>
> <br>
> I have created a bug for this:<br>
> <br>
> <a href="https://bugs.openjdk.java.net/browse/JDK-8214226" \
rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8214226</a><br>
> <br>
> Thank you,<br>
> Jini<br>
> <br>
> On 11/22/2018 12:50 AM, David Griffiths wrote:<br>
> > PS: should have added a new X86Frame constructor really, may \
have<br> > just<br>
> > been put off because there is already a four address constructor \
so<br> > > would have had to add dummy argument or something.<br>
> ><br>
> > On Wed, 21 Nov 2018 at 19:15, David Griffiths<br>
> <<a href="mailto:david.griffiths@gmail.com" \
target="_blank">david.griffiths@gmail.com</a> <mailto:<a \
href="mailto:david.griffiths@gmail.com" \
target="_blank">david.griffiths@gmail.com</a>><br> > > <mailto:<a \
href="mailto:david.griffiths@gmail.com" \
target="_blank">david.griffiths@gmail.com</a><br> > <mailto:<a \
href="mailto:david.griffiths@gmail.com" \
target="_blank">david.griffiths@gmail.com</a>>>> wrote:<br> > \
><br> > > Hi, thanks, apart from adding a setter for R13 in \
X86Frame, the<br> > > other half of the fix is this:<br>
> ><br>
> > public Frame getCurrentFrameGuess(JavaThread \
thread,<br> > Address<br>
> > addr) {<br>
> > ThreadProxy t = getThreadProxy(addr);<br>
> > AMD64ThreadContext context = \
(AMD64ThreadContext)<br> > t.getContext();<br>
> > AMD64CurrentFrameGuess guesser = new<br>
> > AMD64CurrentFrameGuess(context, thread);<br>
> > if (!guesser.run(GUESS_SCAN_RANGE)) {<br>
> > return null;<br>
> > }<br>
> > if (guesser.getPC() == null) {<br>
> > return new X86Frame(guesser.getSP(), \
guesser.getFP());<br> > > } else if<br>
> (VM.getVM().getInterpreter().contains(guesser.getPC())) {<br>
> > // pass the value of R13 which contains the bcp \
for<br> > the top<br>
> > level frame<br>
> > Address r13 =<br>
> > context.getRegisterAsAddress(AMD64ThreadContext.R13);<br>
> > X86Frame frame = new \
X86Frame(guesser.getSP(),<br> > > guesser.getFP(), \
guesser.getPC());<br> > > frame.setR13(r13);<br>
> > return frame;<br>
> > } else {<br>
> > return new X86Frame(guesser.getSP(), \
guesser.getFP(),<br> > > guesser.getPC());<br>
> > }<br>
> > }<br>
> ><br>
> > (the whole "if pc in interpreter" block is \
new)<br> > ><br>
> > Overhead likely to be low as this is only used in \
diagnostic<br> > code.<br>
> > Can't think of any risk but I'm not an expert on \
this code.<br> > ><br>
> > Cheers,<br>
> ><br>
> > David<br>
> ><br>
> > On Wed, 21 Nov 2018 at 19:01, JC Beyler <<a \
href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a><br> > \
<mailto:<a href="mailto:jcbeyler@google.com" \
target="_blank">jcbeyler@google.com</a>><br> > > \
<mailto:<a href="mailto:jcbeyler@google.com" \
target="_blank">jcbeyler@google.com</a> <mailto:<a \
href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a>>>> \
wrote:<br> > ><br>
> > Hi David,<br>
> ><br>
> > I think the easiest would be to see whole change \
to<br> > understand<br>
> > the repercussions of the change. I would imagine that \
any<br> > change<br>
> > that helps stacktraces being more precise is a good \
thing but<br> > > there are questions that arise every \
time:<br> > > - What is the overhead of adding \
this?<br> > > - Does this add any risk of failure?<br>
> ><br>
> > I'd imagine that the change is relatively small \
and should be<br> > > easy to assess this but seeing it \
would make things easier to<br> > > determine.<br>
> ><br>
> > That being said, I'm not a reviewer for OpenJDK so \
this is<br> > > really just my 2cents,<br>
> > Jc<br>
> ><br>
> > On Wed, Nov 21, 2018 at 9:17 AM David Griffiths<br>
> > <<a href="mailto:david.griffiths@gmail.com" \
target="_blank">david.griffiths@gmail.com</a><br> > <mailto:<a \
href="mailto:david.griffiths@gmail.com" \
target="_blank">david.griffiths@gmail.com</a>> <mailto:<a \
href="mailto:david.griffiths@gmail.com" \
target="_blank">david.griffiths@gmail.com</a><br> > <mailto:<a \
href="mailto:david.griffiths@gmail.com" \
target="_blank">david.griffiths@gmail.com</a>>>><br> > > \
wrote:<br> > ><br>
> > Hi, I'm new to this mailing list and working \
on a project<br> > > that makes use of the SA \
classes to get stack traces<br> > from a<br>
> > paused in flight JVM (we can't use JDWP). I \
have observed<br> > > that if the top frame is in \
the interpreter it<br> > reports the<br>
> > BCI and line number incorrectly. This is \
because<br> > > X86Frame.getInterpreterFrameBCI uses \
the value stored<br> > on the<br>
> > stack rather than the actual live value stored \
in R13.<br> > ><br>
> > I have a patch for this which lets<br>
> > \
LinuxAMD64JavaThreadPDAccess.getCurrentFrameGuess<br> > pass the<br>
> > R13 value to X86Frame so that the latter can \
then do:<br> > ><br>
> > public int getInterpreterFrameBCI() {<br>
> > Address bcp =<br>
> > \
addressOfInterpreterFrameBCX().getAddressAt(0);<br> > > \
// If we are in the top level frame then R13 may<br> > have<br>
> > been set for us which contains<br>
> > // the BCP. If so then let it take \
priority. If<br> > we are<br>
> > in a top level interpreter frame,<br>
> > // the BCP is live in R13 (on x86) and \
not saved<br> > in the<br>
> > BCX stack slot.<br>
> > if (r13 != null) {<br>
> > bcp = r13;<br>
> > }<br>
> > Address methodHandle =<br>
> > \
addressOfInterpreterFrameMethod().getAddressAt(0);<br> > ><br>
> > and this fixes the problem.<br>
> ><br>
> > Does this sound like a good idea and if so \
should I<br> > submit a<br>
> > patch?<br>
> ><br>
> > Cheers,<br>
> ><br>
> > David<br>
> ><br>
> ><br>
> ><br>
> > --<br>
> ><br>
> > Thanks,<br>
> > Jc<br>
> ><br>
> <br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" \
class="gmail_signature" data-smartmail="gmail_signature"><div \
dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic