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> 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>> 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>> 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>> > > 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 > > >
interpreter_frame.patch
Description: Binary data
