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 <jcbey...@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.geo...@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.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
>> > >
>> >
>>
>
>
> --
>
> Thanks,
> Jc
>
revised_interpreter_frame.patch
Description: Binary data
