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

Wed Sep 6 07:26:44 UTC 2017

Chris,

> 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.

NMT_InternalFrames means *maximum possible* number of internal frames.

We check *a name of the frame* before we skip it, so we don't need to
know how many frames got inlined on in particular build by particular
compiler and this is the main idea of the fix.

i.e. It's safe to set (e.g.) NMT_InternalFrames = 10 and keep this logic
in production.

I guard the changes by #ifndef PRODUCT to avoid memory overhead in
production but if we can afford memory/performance penalty of
storing 3 or 4 extra frames, it's better to keep this logic
enabled ever in a product build.

*Testing:*
I'd tested 3 builds (release, fastdebug, slowdebug) on two platforms
(Linux/x86_64, Linux/aarch64). All hotspot/runtime/NMT tests are passed
including CheckForProperDetailStackTrace. Also, manual comparison of
stacktraces on Linux/x86_64 don't show any changes in output.

-Dmitry

On 05.09.2017 21:34, Chris Plummer wrote:
> 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
>>>>
>>>>
>>>
>>>
> 