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
