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

[Lindenmaier, Goetz]

HI Chris,

I found the change in the repo, thanks for sponsoring!

Best regards,
  Goetz

> -----Original Message-----
> From: Chris Plummer [mailto:chris.plummer at oracle.com]
> Sent: Dienstag, 13. September 2016 06:14
> 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"
> 
> 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.jav
> a
> > 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.
> >>
> >>
> >