RFR: 8256843: [PPC64] runtime/logging/RedefineClasses.java fails with assert: registers not saved on stack
Martin Doerr
mdoerr at openjdk.java.net
Mon Dec 14 13:45:57 UTC 2020
On Mon, 14 Dec 2020 10:47:01 GMT, Richard Reingruber <rrich at openjdk.org> wrote:
>> Thanks for your proposal. I'll try it.
>>
>>> test/hotspot/jtreg/runtime/NMT/CheckForProperDetailStackTrace.java failed in slowdebug on AIX. But that's no showstopper, is it?
>>
>> If it fails without this change, it'd be no regression and hence ok.
>>
>>> My attempt to build also release on AIX failed because of an internal compiler error raised building libharfbuzz.
>>
>> That's a known xlc bug. Workaround:
>> --- a/make/modules/java.desktop/lib/Awt2dLibraries.gmk
>> +++ b/make/modules/java.desktop/lib/Awt2dLibraries.gmk
>> @@ -475,6 +475,9 @@ else
>> #
>>
>> LIBHARFBUZZ_OPTIMIZATION := HIGH
>> + ifeq ($(call isTargetOs, aix), true)
>> + LIBHARFBUZZ_OPTIMIZATION := LOW
>> + endif
>>
>> LIBHARFBUZZ_CFLAGS += $(X_CFLAGS) -DLE_STANDALONE -DHEADLESS
>
> Hi Martin
>
> we can even do without _NMT_NOINLINE_.
>
> diff --git a/src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp b/src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp
> index 2eaf38d05cd..83f8af3b751 100644
> --- a/src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp
> +++ b/src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp
> @@ -159,13 +159,9 @@ frame os::get_sender_for_C_frame(frame* fr) {
>
>
> frame os::current_frame() {
> - intptr_t* csp = (intptr_t*) *((intptr_t*) os::current_stack_pointer());
> - // hack.
> - frame topframe(csp, (address)0x8);
> - // Return sender of sender of current topframe which hopefully
> - // both have pc != NULL.
> - frame tmp = os::get_sender_for_C_frame(&topframe);
> - return os::get_sender_for_C_frame(&tmp);
> + intptr_t* csp = *(intptr_t**) __builtin_frame_address(0);
> + frame topframe(csp, CAST_FROM_FN_PTR(address, os::current_frame));
> + return os::get_sender_for_C_frame(&topframe);
> }
>
> bool PosixSignals::pd_hotspot_signal_handler(int sig, siginfo_t* info,
> diff --git a/src/hotspot/os_cpu/linux_ppc/os_linux_ppc.cpp b/src/hotspot/os_cpu/linux_ppc/os_linux_ppc.cpp
> index 5d68c4b3407..8e0af2acecc 100644
> --- a/src/hotspot/os_cpu/linux_ppc/os_linux_ppc.cpp
> +++ b/src/hotspot/os_cpu/linux_ppc/os_linux_ppc.cpp
> @@ -179,13 +179,9 @@ frame os::get_sender_for_C_frame(frame* fr) {
>
>
> frame os::current_frame() {
> - intptr_t* csp = (intptr_t*) *((intptr_t*) os::current_stack_pointer());
> - // hack.
> - frame topframe(csp, (address)0x8);
> - // Return sender of sender of current topframe which hopefully
> - // both have pc != NULL.
> - frame tmp = os::get_sender_for_C_frame(&topframe);
> - return os::get_sender_for_C_frame(&tmp);
> + intptr_t* csp = *(intptr_t**) __builtin_frame_address(0);
> + frame topframe(csp, CAST_FROM_FN_PTR(address, os::current_frame));
> + return os::get_sender_for_C_frame(&topframe);
> }
>
> bool PosixSignals::pd_hotspot_signal_handler(int sig, siginfo_t* info,
> diff --git a/src/hotspot/share/utilities/nativeCallStack.cpp b/src/hotspot/share/utilities/nativeCallStack.cpp
> index 1093a13eaec..45a29424fd7 100644
> --- a/src/hotspot/share/utilities/nativeCallStack.cpp
> +++ b/src/hotspot/share/utilities/nativeCallStack.cpp
> @@ -36,7 +36,7 @@ NativeCallStack::NativeCallStack(int toSkip, bool fillStack) :
> // to call os::get_native_stack. A tail call is used if _NMT_NOINLINE_ is not defined
> // (which means this is not a slowdebug build), and we are on 64-bit (except Windows).
> // This is not necessarily a rule, but what has been obvserved to date.
> -#if (defined(_NMT_NOINLINE_) || defined(_WINDOWS) || !defined(_LP64))
> +#if (defined(_NMT_NOINLINE_) || defined(_WINDOWS) || !defined(_LP64) || defined(PPC64))
> // Not a tail call.
> toSkip++;
> #if (defined(_NMT_NOINLINE_) && defined(BSD) && defined(_LP64))
> diff --git a/test/hotspot/jtreg/runtime/logging/RedefineClasses.java b/test/hotspot/jtreg/runtime/logging/RedefineClasses.java
>
> My manual tests succeed with this on aix and linuxppc64le, except for the slowdebug build on aix where test/hotspot/jtreg/runtime/NMT/CheckForProperDetailStackTrace.java fails with _and_ without the fix.
>
> Currently I don't understand why the result of __builtin_frame_address(0) has to
> be dereferenced? Looking at the disassembly of the slowdebug build it returns
> the sp of os::current_frame(). The head revision does the same though.
>
> (gdb) disass /s 0x7fffb71ed974,+64
> Dump of assembler code from 0x7fffb71ed974 to 0x7fffb71ed9b4:
>
> 181 frame os::current_frame() {
> 0x00007fffb71ed974 <os::current_frame()+0>: addis r2,r12,161
> 0x00007fffb71ed978 <os::current_frame()+4>: addi r2,r2,-31604
> 0x00007fffb71ed97c <os::current_frame()+8>: mflr r0
> 0x00007fffb71ed980 <os::current_frame()+12>: std r0,16(r1)
> 0x00007fffb71ed984 <os::current_frame()+16>: std r31,-8(r1)
> 0x00007fffb71ed988 <os::current_frame()+20>: stdu r1,-128(r1)
> 0x00007fffb71ed98c <os::current_frame()+24>: mr r31,r1
> 0x00007fffb71ed990 <os::current_frame()+28>: std r3,40(r31)
>
> 182 intptr_t* csp = *(intptr_t**) __builtin_frame_address(0);
> 0x00007fffb71ed994 <os::current_frame()+32>: mr r9,r31
> 0x00007fffb71ed998 <os::current_frame()+36>: ld r9,0(r9)
> 0x00007fffb71ed99c <os::current_frame()+40>: std r9,56(r31)
>
> 183 // hack.
> 184 frame topframe(csp, CAST_FROM_FN_PTR(address, os::current_frame));
> 0x00007fffb71ed9a0 <os::current_frame()+44>: addi r9,r31,64
> 0x00007fffb71ed9a4 <os::current_frame()+48>: addis r5,r2,-161
> 0x00007fffb71ed9a8 <os::current_frame()+52>: addi r5,r5,31604
> 0x00007fffb71ed9ac <os::current_frame()+56>: ld r4,56(r31)
> 0x00007fffb71ed9b0 <os::current_frame()+60>: mr r3,r9
Hi Richard,
I was not aware of that __builtin_frame_address is available on all PPC64 platforms. Makes sense to use it and to get rid of the inline assembly.
> Currently I don't understand why the result of __builtin_frame_address(0) has to be dereferenced?
I don't understand that, either. Seems like it was implemented by trial and error. I think trying to predict the C++ compiler's inlining is a terrible design, but it's only used by non-critical code like NMT stack traces, so we can focus on more critical issues.
We could use __builtin_frame_address(1) and remove the dereferencing. I think we should use that also in os::current_stack_pointer(). What do you think?
Anyway, I like your proposal. Would you mind creating a PR for jdk16 as we need it there? I'm closing this one.
-------------
PR: https://git.openjdk.java.net/jdk/pull/1724
More information about the hotspot-dev
mailing list