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

[Lindenmaier, Goetz]

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