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

David Holmes david.holmes at oracle.com
Mon Sep 12 01:07:33 UTC 2016

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


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.

More information about the hotspot-runtime-dev mailing list