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

Chris Plummer chris.plummer at oracle.com
Wed Sep 14 17:21:47 UTC 2016


You're welcome!

Chris

On 9/14/16 12:11 AM, Lindenmaier, Goetz wrote:
> HI Chris,
>
> I found the change in the repo, thanks for sponsoring!
>
> Best regards,
>    Goetz
>
>> -----Original Message-----
>> From: Chris Plummer [mailto:chris.plummer at oracle.com]
>> Sent: Dienstag, 13. September 2016 06:14
>> 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"
>>
>> On 9/12/16 7:14 PM, Chris Plummer wrote:
>>> 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.jav
>> a
>>> 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
>> The following seems to fix the failure:
>>
>> diff --git
>> a/test/testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java
>> b/test/testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java
>> --- a/test/testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java
>> +++ b/test/testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java
>> @@ -50,7 +50,7 @@
>>            OS("isAix", "isLinux", "isOSX", "isSolaris", "isWindows"),
>>            VM_TYPE("isClient", "isServer", "isGraal", "isMinimal",
>> "isZero", "isEmbedded"),
>>            MODE("isInt", "isMixed", "isComp"),
>> -        IGNORED("isDebugBuild", "shouldSAAttach",
>> +        IGNORED("isDebugBuild", "isFastDebugBuild", "isSlowDebugBuild",
>> "shouldSAAttach",
>>                    "canPtraceAttachLinux", "canAttachOSX",
>> "isTieredSupported");
>>
>>            public final List<String> methodNames;
>>
>>>> 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.
>>>>



More information about the hotspot-runtime-dev mailing list