RFR (S): 8218458: [TESTBUG] runtime/NMT/CheckForProperDetailStackTrace.java fails with Expected stack trace missing from output

[Chris Plummer]

On 4/3/19 11:23 PM, David Holmes wrote:
> Hi Chris,
>
> On 4/04/2019 4:12 pm, Chris Plummer wrote:
>> Hi David,
>>
>> I have concerns that this will hide some of the other bugs I've 
>> mentioned: JDK-8133749, JDK-8133747, and JDK-8133740. These bugs 
>> result in 1 or two frames appearing in the stacktrace that should be 
>> skipped. Notably NativeCallStack::NativeCallStack() and 
>> os::get_native_stack().
>
> The test still checks those are not present first:
>
> 73         // We should never see either of these frames because they 
> are supposed to be skipped. */
> 74 output.shouldNotContain("NativeCallStack::NativeCallStack");
> 75         output.shouldNotContain("os::get_native_stack");
Ah yes. I skimmed over the test looking for it but missed it.
>
>> Also, AllocateHeap() should normally not be in the stack trace, but 
>> the test has specifically allowed for it for windows and solaris 
>> slowdebug builds. Although these builds should have honored the 
>> ALWAYSINLINE directive, it was deemed acceptable that it was not in 
>> slowdebug builds. However, I would not want to allow AllocateHeap() 
>> to appear in a product build, and best not to see it in fastdebug 
>> either.
>
> This is a test of NMT detail not a test of whether a given compiler 
> chooses to inline something like AllocateHeap. I don't think it is the 
> job of this test to be checking for something specific to the native 
> compiler. The previous handling of AllocateHeap seemed to be there 
> simply because it was the only way to deal with an optional frame - 
> but now that's handled generically.
It's appearance means you effectively only have 3 frames to identity 
callsites instead of 4. If it does appear in a product build, a solution 
should be looked into to get rid of it. If the port owner decides it 
can't get rid of it (or is unwilling to), then an exception should be 
added to the test like was done for solaris and windows slowdebug builds.

thanks,

Chris
>
> Thanks,
> David
>
>> Given the changes you made to allow more flexibly in which frames 
>> appear, I think you need to now also make sure the above 3 mentioned 
>> frames are not present, except for allowing AllocateHeap() in 
>> slowdebug builds.
>>
>> thanks,
>>
>> Chris
>>
>> On 4/3/19 10:53 PM, David Holmes wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8218458
>>> Webrev: http://cr.openjdk.java.net/~dholmes/8218458/webrev/
>>>
>>> The actual stack trace reported by NMT detail is affected by the 
>>> inlining decisions of the native compiler, and on the type of build. 
>>> So we define an "ideal" stacktrace and then allow for some frames to 
>>> be missing based on empirical observations. So to date we have seen 
>>> two frames that may or may not be inlined and so we allow for 2 
>>> non-matching entries.
>>>
>>> The special-casing of AllocateHeap is removed as now it is just an 
>>> optional frame.
>>>
>>> Chris: does this maintain the "spirit" of the test as you intended?
>>>
>>> Zhengyu: can you test this on your system(s) please.
>>>
>>> Thanks,
>>> David
>>
>>