[RFR] 8196969: JTreg Failure: serviceability/sa/ClhsdbJstack.java causes NPE

[Severin Gehwolf]

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