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

Chris Plummer chris.plummer at oracle.com
Tue Sep 13 04:14:00 UTC 2016 

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

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