RFR(S): 8165315: [ppc] Port "8133749: NMT detail stack trace cleanup"

[David Holmes]

Thanks Goetz! I like this version much better! :)

Cheers,
David

On 9/09/2016 7:21 PM, Lindenmaier, Goetz wrote:
> Hi Chris,
>
> I changed current_frame() and it's finally through our tests.  (We had an
> issue with our build machines the day before ...)
>
> Here the new webrev:
> http://cr.openjdk.java.net/~goetz/wr16/8165315-fixStackTrace/02/webrev.bs/
> http://cr.openjdk.java.net/~goetz/wr16/8165315-fixStackTrace/02/webrev.hs/
>
> The webrev of the base repo is unchanged except for the reviewer attribution.
>
> I would appreciate a lot if you could sponsor this change.
>
> Best regards,
>   Goetz.
>
>> -----Original Message-----
>> From: Chris Plummer [mailto:chris.plummer at oracle.com]
>> Sent: Dienstag, 6. September 2016 18:51
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
>> dev at openjdk.java.net
>> Subject: Re: RFR(S): 8165315: [ppc] Port "8133749: NMT detail stack trace
>> cleanup"
>>
>> Hi Goetz,
>>
>> Comments inline below:
>>
>> On 9/5/16 12:28 AM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> The test coming with 8133749 showed a row of problems on the ppc
>> platforms.
>>> This change fixed these.
>>>
>>>
>>> Also, I moved "isSlowDebugBuild()" to Platform.java as there also is
>>>
>>> isDebugBuild(), and we have other places where we use this.
>> Ok.
>>>
>>> Please review this change. I also please need a sponsor.
>>> http://cr.openjdk.java.net/~goetz/wr16/8165315-
>> fixStackTrace/01/webrev.bs/
>>> http://cr.openjdk.java.net/~goetz/wr16/8165315-
>> fixStackTrace/01/webrev.hs/
>>>
>>> More details:
>>>
>>> On power, current_frame() returns the frame of the method that
>>> called current_frame(). This is as documented in os.hpp.
>>>
>>> Get_native_stack() in os_posix.cpp expects current_frame() to go up
>>> one more frame. To adapt to this expectation, we increment toSkip by
>>> one on ppc, which has the same effect. (If we change current_frame(),
>>> one less frame will be printed to hs_err files etc.)
>> The fact that os::current_frame() actually returns the caller's frame
>> (two frames up from os:current_frame instead of one frame up) is covered
>> by the following CR:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8163900
>> JDK-8163900 os::current_frame has a misleading name
>>
>> I would prefer that you changed PPC/AIX to be consistent with all the
>> other ports rather than change os_posix.cpp to adust for PPC/AIX.
>> JDK-8163900 will most likely be fixed with a simple rename to
>> os::caller_frame() given that this is what all the users of this API
>> actually want. I can't think of any reason why the stack trace would
>> want to include the frame of whoever calls os::current_frame(), so the
>> rename seems like the most practical fix.
>>>
>>> "8153743: AllocateHeap() and ReallocateHeap() should use ALWAYSINLINE
>> macro"
>>> is not properly implemented on Aix. The 'inline' keyword is missing in the
>>> macro on aix.
>> But you also changed the test to expect ALWAYSINLINE not to work with
>> PPC slowdebug builds? When is adding "inline" helping? Is it needed for
>> AIX optimized builds?
>>>
>>> Also, on Aix ALWAYSINLINE has no effect in the slowdebug build. So the
>>> check in the test whether AllocateHeap is inlined must be skipped as on
>>> other platforms.
>> See the above comment. Otherwise, the changes to the check for AIX in
>> the test are fine.
>>
>> BTW, the following CR might also be of interest to you as it relates to
>> why the test needs this extra check:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8163899
>> JDK-8163899 NMT frame skipping code is fragile
>>
>> cheers,
>>
>> Chris
>>>
>>> Best regards,
>>>    Goetz.
>>
>