On Mon, 2019-10-07 at 09:11 -0700, Tom Rodriguez wrote:
> Looks good to me.
Thanks for the review!
Cheers,
Severin
> tom
>
> Severin Gehwolf wrote on 10/7/19 2:43 AM:
> > Hi Tom,
> >
> > On Thu, 2019-10-03 at 11:14 -0700, Tom Rodriguez wrote:
> > > I think your fix is a reasonable way to resolve the NPE. We certainly
> > > shouldn't try to create a ScopeDesc from an invalid location. Maybe
> > > getPCDescNearDbg should try to do a better job finding a PcDesc with a
> > > valid scope_decode_offset instead? Something like this maybe?
> > >
> > > diff -r c414c554b38b
> > > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java
> > > ---
> > > a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java
> > > +++
> > > b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java
> > > @@ -306,12 +306,16 @@
> > > /** This is only for use by the debugging system, and is only
> > > intended for use in the topmost frame, where we are not
> > > guaranteed to be at a PC for which we have a PCDesc. It finds
> > > - the PCDesc with realPC closest to the current PC. */
> > > + the PCDesc with realPC closest to the current PC that has a valid
> > > scopeDecodeOffset. */
> > > public PCDesc getPCDescNearDbg(Address pc) {
> > > PCDesc bestGuessPCDesc = null;
> > > long bestDistance = 0;
> > > for (Address p = scopesPCsBegin(); p.lessThan(scopesPCsEnd()); p =
> > > p.addOffsetTo(pcDescSize)) {
> > > PCDesc pcDesc = new PCDesc(p);
> > > + if (pd.getScopeDecodeOffset() ==
> > > DebugInformationRecorder.SERIALIZED_NULL) {
> > > + // We've observed a serialized null decode offset so ignore
> > > this PcDesc
> > > + continue;
> > > + }
> > > // In case pc is null
> > > long distance = -pcDesc.getRealPC(this).minus(pc);
> > > if ((bestGuessPCDesc == null) ||
> > >
> > > As far as I can see getPCDescNearDbg is only used by getScopeDescNearDbg
> > > so the semantic change seems reasonable to me.
> >
> > Updated webrev:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8196969/04/webrev/
> >
> > Preliminary testing looks good.
> >
> > How does it look?
> >
> > Andrew: Are you still OK with this change?
> >
> > Thanks,
> > Severin
> >
> > > tom
> > >
> > > Chris Plummer wrote on 10/3/19 9:19 AM:
> > > > Hi Severin,
> > > >
> > > > I've cc'd Tom Rodriquez who said he would have a look.
> > > >
> > > > thanks,
> > > >
> > > > Chris
> > > >
> > > > On 10/1/19 12:51 PM, Chris Plummer wrote:
> > > > > Hi Severin,
> > > > >
> > > > > Sorry, this is not an area that I have any expertise in. However, I
> > > > > did confirm that it fixes the NPE I was seeing with
> > > > > JShellHeapDumpTest.java, which brings up a question. You said this
> > > > > happens with -Xcomp, but I was never using -Xcomp. Might it also be
> > > > > triggered without -Xcomp?
> > > > >
> > > > > thanks,
> > > > >
> > > > > Chris
> > > > >
> > > > > On 10/1/19 1:58 AM, Severin Gehwolf wrote:
> > > > > > Anyone? Chris maybe?
> > > > > >
> > > > > > On Fri, 2019-09-27 at 14:12 +0200, Severin Gehwolf wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > Could I please get reviews for this SA fix? The issue only happens
> > > > > > > intermittently and with -Xcomp. The new regression test
> > > > > > > reproduces the
> > > > > > > issue somewhat reliably. I got 10/10 fails for unpatched, but
> > > > > > > I've seen
> > > > > > > it pass as well.
> > > > > > >
> > > > > > > When the issue happens, PCDesc's getScopeDecodeOffset() returs 0
> > > > > > > (DebugInformationRecorder.SERIALIZED_NULL). The current SA code
> > > > > > > doesn't
> > > > > > > handle this case and goes on and tries to read ScopeDesc from the
> > > > > > > DebugInfoReadStream at the bogus offset. From then on, bad things
> > > > > > > happen. A NPE in StackTrace could be one symptom.
> > > > > > >
> > > > > > > The same code in hotspot deals with serialized null differently.
> > > > > > > It
> > > > > > > doesn't read from the debug info stream, and manually sets up a
> > > > > > > reasonable frame. Note decode_body is called from ScopeDesc's
> > > > > > > constructor where decode_offset might have been set to 0:
> > > > > > >
> > > > > > > void ScopeDesc::decode_body() {
> > > > > > > if (decode_offset() ==
> > > > > > > DebugInformationRecorder::serialized_null) {
> > > > > > > // This is a sentinel record, which is only relevant to
> > > > > > > // approximate queries. Decode a reasonable frame.
> > > > > > > _sender_decode_offset =
> > > > > > > DebugInformationRecorder::serialized_null;
> > > > > > > _method = _code->method();
> > > > > > > _bci = InvocationEntryBci;
> > > > > > > _locals_decode_offset =
> > > > > > > DebugInformationRecorder::serialized_null;
> > > > > > > _expressions_decode_offset =
> > > > > > > DebugInformationRecorder::serialized_null;
> > > > > > > _monitors_decode_offset =
> > > > > > > DebugInformationRecorder::serialized_null;
> > > > > > > } else {
> > > > > > > // decode header
> > > > > > > DebugInfoReadStream* stream = stream_at(decode_offset());
> > > > > > >
> > > > > > > _sender_decode_offset = stream->read_int();
> > > > > > > _method = stream->read_method();
> > > > > > > _bci = stream->read_bci();
> > > > > > >
> > > > > > > // decode offsets for body and sender
> > > > > > > _locals_decode_offset = stream->read_int();
> > > > > > > _expressions_decode_offset = stream->read_int();
> > > > > > > _monitors_decode_offset = stream->read_int();
> > > > > > > }
> > > > > > > }
> > > > > > >
> > > > > > > The proposed patch handles serialized null scopes similar to the
> > > > > > > hotspot side of things, by returning a null scope. CompiledVFrame
> > > > > > > already deals with null scopes when in debugging mode.
> > > > > > >
> > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8196969
> > > > > > > webrev:
> > > > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8196969/03/webrev/
> > > > > > >
> > > > > > > Testing: tier 1 tests on Linux x86_64 (release/fastdebug).
> > > > > > > jdk-submit
> > > > > > > and ran various reproducer tests including 1000 interations of the
> > > > > > > added regression test. All pass.
> > > > > > >
> > > > > > > Thoughts?
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Severin
