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

[Chris Plummer]

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.