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

[Chris Plummer]

On 9/12/16 2:05 PM, Chris Plummer wrote:
> 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).
Hi Goetz,

hotspot/test/testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java 
is failing due to your changes in Platform.java. I believe 
isFastDebugBuild and isSlowDebugBuild need to be added to the ignored 
set, although I haven't confirmed this yet. Can you generate a new 
changeset for me with the fix?

thanks,

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