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

[Tom Rodriguez]

Looks good to me.

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
>