[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-12-12 17:08:33
Message-ID: CAF9BGBxBVPAp1dmYR-AEMGQtwKCNgecWxnowtmuFBTH4VrTUmw () mail ! gmail ! com
[Download RAW message or body]
Hi David,
Thanks for the changes! Your latest patch looks good to me (I think a
follow-up could/might be to go to a builder system for the frame because
the various constructors are a bit confusing but that might be out of scope
of this :-)),
Jc
On Wed, Dec 5, 2018 at 3:10 AM David Griffiths <david.griffiths@gmail.com>
wrote:
> Hi, thanks for reviewing. I have made the changes you suggested and also
> tidied up the constructors a bit (there was already a 4x Address
> constructor), hope that's ok.
>
> Cheers,
>
> David
>
> On Fri, 30 Nov 2018 at 17:06, JC Beyler <jcbeyler@google.com> wrote:
>
>> 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
>>
>
--
Thanks,
Jc
[Attachment #3 (text/html)]
<div dir="ltr">Hi David,<div><br></div><div>Thanks for the changes! Your latest patch \
looks good to me (I think a follow-up could/might be to go to a builder system for \
the frame because the various constructors are a bit confusing but that might be out \
of scope of this :-)),</div><div>Jc</div></div><br><div class="gmail_quote"><div \
dir="ltr">On Wed, Dec 5, 2018 at 3:10 AM David Griffiths <<a \
href="mailto:david.griffiths@gmail.com">david.griffiths@gmail.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div \
dir="ltr"><div>Hi, thanks for reviewing. I have made the changes you suggested and \
also tidied up the constructors a bit (there was already a 4x Address constructor), \
hope that's ok.</div><div><br></div><div>Cheers,</div><div><br></div><div>David<br></div></div><br><div \
class="gmail_quote"><div dir="ltr">On Fri, 30 Nov 2018 at 17:06, JC Beyler <<a \
href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><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" target="_blank">jini.george@oracle.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left:1px solid rgb(204,204,204);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-m_-9023471760572052622m_-7540626310054304673gmail_signature"><div \
dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div> </blockquote></div>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" \
class="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