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

[Severin Gehwolf]

Hi Chris,

On Tue, 2019-10-01 at 12:51 -0700, 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?

Yes, as soon as C1/C2 kick in you might be seeing the issue. In fact, I
had one reproducer which triggered the issue quite reliably which
didn't use -Xcomp explicitly. Nevertheless, certain methods got JIT
compiled soon and the issue surfaced. You should be able to verify by
adding -XX:+PrintCompilation to the host program and/or comparing it to
-Xint runs.

TLDR; -Xcomp helps reproducing the issue on smaller test cases, but
isn't essential in the general case. 

Does that make sense?

Thanks,
Severin

> 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