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

[Chris Plummer]

On 9/12/16 1:37 AM, Lindenmaier, Goetz wrote:
> Hi Chris,
>
> Thanks for offering to sponsor!
>
>> noticed you left out the os::is_first_C_frame(&caller_frame)) check
> I left it out because the comment on linux_x86 says
>     if (os::is_first_C_frame(&myframe)) {
>      // stack is not walkable
> But on ppc the stack is always walkable.
> get_sender_for_C_frame() checks sp() == 0 anyways, so I think this
> should work.
Hi Goetz,

I think the x86 comment is a bit misleading because it doesn't take into 
account how/when os::current_frame() is called. The stack is actually 
walkable for at least two frames up from the os::current_frame() frame 
on all platforms. At that point you might be at the "first C frame", but 
that's up to the caller of os::current_frame() to worry about if it 
tries to walk up from the frame returned. I'm ok with what you have in 
place now since I think any checks here are necessary (on all platforms).

thanks,

Chris
>
> Best regards,
>    Goetz.
>
>
>> -----Original Message-----
>> From: Chris Plummer [mailto:chris.plummer at oracle.com]
>> Sent: Freitag, 9. September 2016 19:10
>> 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,
>>
>> The changes look good except I noticed you left out the
>> os::is_first_C_frame(&caller_frame)) check that we have in other ports.
>> I actually concluded in the past that this check shouldn't be needed, or
>> maybe should be an assert. Based on where we currently call
>> os::current_frame(), caller_frame should always have one more C frame
>> above it. I was wondering if you came to the same conclusion so you left
>> the check out.
>>
>> Yes, I can sponsor this for you.
>>
>> thanks,
>>
>> Chris
>>
>> On 9/9/16 2:21 AM, 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.