Suggested improvement to X86Frame.getInterpreterFrameBCI
JC Beyler
jcbeyler at google.com
Wed Dec 12 17:08:33 UTC 2018
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 at 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 at 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 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
>>
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181212/5146390e/attachment-0001.html>
More information about the serviceability-dev
mailing list