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

[Lindenmaier, Goetz]

Hi Chris, 

thanks for looking at my change!
See my comments inline.

> -----Original Message-----
> From: Chris Plummer [mailto:chris.plummer at oracle.com]
> Sent: Tuesday, September 06, 2016 6:51 PM
> 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.
OK, I'll change it accordingly.  . I'll build (and test) the requested
current_frame implementation tomorrow - I want to 
run all our tests on it.

> > "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?
Yes, the inline keyword fixes the fastdebug build. The keyword was removed 
from in front of the functions in 8153743 and moved to the macro. This edit
was missed in globalDefinitions_xlc.hpp.

> > 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 
Yes I'm aware of this. I did the current fix by looking into the code with the
debugger. As soon as we change compiler, etc it might obviously break.
(Therefore I also think it's bad to skip that much frames in current_frame, 
because if too many changes are skipped the info is useless, if too less 
frames are skipped you can at least find the frame with the allocation.
(Don't know whether it breaks NMT tools, though). If you use current_frame, 
you have to know all your callers and control their inlining to make 
sure the proper frame is found!  But I don't want to open this discussion here.)

Best regards,
  Goetz.

> 
> cheers,
> 
> Chris
> >
> > Best regards,
> >    Goetz.
>