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

Thu Apr 4 15:48:44 UTC 2019

Hi David,

On 4/4/19 12:14 AM, David Holmes wrote:
> On 4/04/2019 4:35 pm, Chris Plummer wrote:
>> 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. 
>
> Both stacktraces in the old test had 4 elements and expected 4 
> matches. The current bug is that one of those (new_entry) could 
> actually be inlined as well, resulting in only 3 matches. So that is 
> what the revised test checks for: at least 3 matches. Often there will 
> be 4 matches.
I think you misunderstood my "3 frames" comment. I was referring to how 
many frames NMT uses to identify the callsite. It wants to use 4, but if 
AllocateHeap() doesn't get inlined, it effectively is using 3. The test 
should detect when this happens so the NMT implementation can address 
the issue.
>
> Hmmm but now I'm wondering why this trace:
>
>   50     public static String stackTraceAllocateHeap =
>   51         ".*AllocateHeap.*\n" +
>   52         ".*ModuleEntryTable.*new_entry.*\n" +
>
> doesn't include ".*Hashtable.*allocate_new_entry.*"? Was it getting 
> inlined already when AllocateHeap was not? Even so we still end up 
> with 4 frames matching normally.
I noticed that last night also and scratch my head over it for a while 
and then went to bed. The only explanation I could come up with is that 
allocate_new_entry() is getting inlined, and as a result (due to being a 
slowdebug build and doing minimal inlining) AllocateHeap() was not inlined.
>
>> 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.
>
> Are we specifically trying to test the compiler's ability to inline 
> that function and just happen to be using this test to verify that? 
> Doesn't seem like a suitable place to do this - and why do we need to 
> do it? The Visual Studio docs state:
>
> "You cannot force the compiler to inline a particular function, even 
> with the __forceinline keyword."
>
> so ALWAYSINLINE is just a hint even in product builds and could change 
> with any update to the compiler.
>
> For Solaris Studio it is again not guaranteed to inline - specifically 
> -xinline only has an effect at –xO3 or higher. Which likely explains 
> why it is ignored in slowdebug. And there are other cases where it 
> won't honour the ALWAYSINLINE.
>
> Even with gcc we seem to be misusing the attribute if we want to 
> ensure inlining when not optimising:
>
> "GCC does not inline any functions when not optimizing unless you 
> specify the ‘always_inline’ attribute for the function, like this:
>
> /* Prototype.  */
> inline void foo (const char) __attribute__((always_inline));"
>
> and we don't write it that way.
>
> So if we're that concerned about release builds guaranteeing to inline 
> AllocateHeap then I think we need something a bit more explicit than 
> this test to determine that.
With respect to the 3 methods/functions we don't want to see in the 
callsite stacktrace, NMT has made a number of assumptions on inlining. 
One of the things the test is doing is making sure those assumptions are 
correct. If incorrect, then you run into issues like I mentioned above 
where callsite backtraces effectively only have 3 unique frames rather 
than 4 (actually before some bug fixes it was often just 2 unique 
frames). So I think it's appropriate to have a test to make sure we are 
not seeing any of these 3 methods/functions.

Now the test also has made inlining assumptions beyond what NMT has 
made, and that is really what this bug is about. In general I think your 
fix is fine in the way it relaxes which frames are actually found, but 
as Thomas points out, it suffers from not actually looking at a single 
stacktrace, but just looking for the specified frames somewhere in the 
output (and in the order specified.) You should probably address this.

thanks,

Chris
>
> Thanks,
> David
>
>> 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
>>>>
>>>>
>>
>>