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

[Chris Plummer]

On 10/2/19 8:53 AM, Severin Gehwolf wrote:
> On Wed, 2019-10-02 at 08:35 -0700, Chris Plummer wrote:
>> On 10/2/19 1:57 AM, Severin Gehwolf wrote:
>>> On Wed, 2019-10-02 at 10:39 +0200, Severin Gehwolf wrote:
>>>> 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.
>>> On second thought, I haven't investigated whether running the SA on a
>>> heap dump triggers the issue as well. There might be another code path
>>> not covered with this patch. If there still is, it might be a similar,
>>> but slightly different bug.
>> So  you are saying your fix here is for when producing the heap dump,
>> but parsing the heap dump might also have a similar issue? I think we
>> are not seeing any issue with the parsing. My new
>> HeapDumpTestWithActiveProcess.java test used to reproduce the NPE when
>> dumping the heap, and your fix has resolved that issue. The test then
>> goes on to dump all the stacks in the heap dump, so I think if there was
>> a problem we would have seen it.
> Glad to hear that.
>
> Process question: Do I need a second review for SA changes?
>
> Thanks,
> Severin
>
I've always assumed you needed two reviewers. Have you tried asking if 
someone on the compiler team is knowledgeable in this area?

Chris