RFR(M): JDK-8163011: AArch64: NMT detail stack trace cleanup

[Chris Plummer]

Hi Dmitry,

I've looked over the changes and some of the comments so far, and do 
agree with Zhengyu regarding removal _NMT_NOINLINE_, but I also have 
concerns about other platform dependent code you have removed.

_NMT_NOINLINE is only defined for slowdebug builds. You now are instead 
trying to change the frame skipping logic to be based on the PRODUCT 
flag. fastdebug builds do not set the PRODUCT flag, so you cannot 
possibly get the frame skipping for both slowdebug and fastdebug builds 
by checking the PRODUCT flag since they each have different frame 
skipping requirements. Since we have/had no other way of telling the 
difference between slowdebug and fastdebug builds, I continued to 
leverage the _NMT_NOINLINE flag for this.

NMT_InternalFrames will should not always be 3 for non product builds. 
It should not only vary between slowdebug and fastdebug, but it also 
vary between platforms. This is due both to compiler differences and 
code differences. That's why there is code like this:

   36     // We need to skip the NativeCallStack::NativeCallStack frame 
if a tail call is NOT used
   37     // to call os::get_native_stack. A tail call is used if 
_NMT_NOINLINE_ is not defined
   38     // (which means this is not a slowdebug build), and we are on 
64-bit (except Windows).
   39     // This is not necessarily a rule, but what has been obvserved 
to date.
   40 #define TAIL_CALL (!defined(_NMT_NOINLINE_) && !defined(WINDOWS) 
&& defined(_LP64))
   41 #if !TAIL_CALL
   42     toSkip++;
   43 #if (defined(_NMT_NOINLINE_) && defined(BSD) && defined(_LP64))
   44     // Mac OS X slowdebug builds have this odd behavior where 
NativeCallStack::NativeCallStack
   45     // appears as two frames, so we need to skip an extra frame.
   46     toSkip++;
   47 #endif
   48 #endif

And also in _get_previous_fp(), which varies in name and implementation 
for various os/cpu combos, the logic for the number of frames to skip is 
not always the same.

You noted that:
> 1. This patch doesn't affect product build. On product build we have all
> NMT frames inlined and don't need to skip anything.
It's not true that for product builds we don't need to skip anything. 
The code above indicates the tail call differences on some platforms, 
which requires skipping a frame in product builds on some platforms, but 
not others.

Thomas wrote:
> Code is easier to read now and less vulnerable
> to compiler decisions.
When the reality is that with your changes compiler decisions are being 
ignored, as are implementation decisions. You are always skipping 3 
frames when it's a non-product build, and that isn't uniformly correct 
for all platforms and for slowdebug vs fastdebug.

I think your NMT_InternalFrames solution makes parts of the code easier 
to read, but in order to be correct its computation needs to be platform 
dependent, and also account for slowdebug/fastdebug differences.

I did write an NMT test to try to make sure frame skipping is correct. 
It's called CheckForProperDetailStackTrace.java. However, I doubt it's 
100% reliable. You should run it with all 3 build flavors: release, 
slowdebug, fastdebug. And you need to run it on all supported platforms. 
For linux-arm32, it would be good to actually use an -marm build instead 
of -mthumb since we don't even get stack traces with -mthumb.

thanks,

Chris

On 9/5/17 8:28 AM, Zhengyu Gu wrote:
> Hi Dmitry,
>
> I have concerns on this change:
>
> Although, you only extend tracking stacks for none-production build, 
> eliminating _NMT_NOINLINE_ actually affect production code. Have you 
> tested production build?
>
> Chris (cc'ed) worked on fixing stack walking before this change, we 
> should get a feedback from him.
>
> Thanks,
>
> -Zhengyu
>
>
> On 09/05/2017 10:49 AM, dmitry.samersov wrote:
>> Everybody,
>>
>> Please, review updated webrev:
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8163011/webrev.06/
>>
>> Only files below different from the previous webrev.
>>
>>   src/share/vm/services/nmtCommon.hpp
>>   src/share/vm/utilities/nativeCallStack.cpp
>>   src/share/vm/utilities/nativeCallStack.hpp
>>
>>
>> 1. Changes guarded by #ifndef PRODUCT
>> 2. Addressed Thomas comments
>>
>> -Dmitry
>>
>> On 31.08.2017 10:49, dmitry.samersov wrote:
>>> Everybody,
>>>
>>> Please review:
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8163011/webrev.05/
>>>
>>>      I would propose different approach to fix JDK-8133740
>>> platform-independent way: record all frames but strip unnecessary
>>> NMT-internal ones on printing.
>>>
>>> This approach is safe (we don't depend to compiler inlining and we 
>>> never
>>> strip non-NMT frames) and platform independent, but cost us some extra
>>> memory.
>>>
>>> -Dmitry
>>>
>>>
>>
>>