Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8256843: [PPC64] runtime/logging/RedefineClasses.java fails with assert: registers not saved on stack #1724

Closed

Conversation

@TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Dec 9, 2020

Method handle logging is broken in fastdebug builds. Problem is that os::current_frame() doesn't return the right frame in fastdebug builds.
Unfortunately, fixing os::current_frame() would break NMT stack traces (runtime/NMT/CheckForProperDetailStackTrace.java).


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8256843: [PPC64] runtime/logging/RedefineClasses.java fails with assert: registers not saved on stack

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1724/head:pull/1724
$ git checkout pull/1724

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 9, 2020

👋 Welcome back mdoerr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Dec 9, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 9, 2020

@TheRealMDoerr The following label will be automatically applied to this pull request:

  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 9, 2020

Webrevs

@reinrich
Copy link
Contributor

@reinrich reinrich commented Dec 10, 2020

Hi Martin,

the fix is, as you commented too, a hack.

I think that (A) os::current_stack_pointer() is inlined into os::current_frame() with optimized builds (all except slowdebug) so for the slowdebug build (_NMT_NOINLINE_ is defined) the sender of the sender of topframe should be returned. For optimized builds just the sender of topframe should be returned (see implementation on s390).

I think furthermore that (B) the compilers on PPC64 don't use a tail call in NativeCallStack::NativeCallStack() so one more frame needs to be skipped.

I'd imagine a fix like the following.

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..f6114062f71 100644
--- a/src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp
+++ b/src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp
@@ -164,8 +164,12 @@ frame os::current_frame() {
   frame topframe(csp, (address)0x8);
   // Return sender of sender of current topframe which hopefully
   // both have pc != NULL.
+#ifdef _NMT_NOINLINE_
   frame tmp = os::get_sender_for_C_frame(&topframe);
   return os::get_sender_for_C_frame(&tmp);
+#else
+  return os::get_sender_for_C_frame(&topframe);
+#endif
 }
 
 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..18d845cc4f3 100644
--- a/src/hotspot/os_cpu/linux_ppc/os_linux_ppc.cpp
+++ b/src/hotspot/os_cpu/linux_ppc/os_linux_ppc.cpp
@@ -184,8 +184,12 @@ frame os::current_frame() {
   frame topframe(csp, (address)0x8);
   // Return sender of sender of current topframe which hopefully
   // both have pc != NULL.
+#ifdef _NMT_NOINLINE_
   frame tmp = os::get_sender_for_C_frame(&topframe);
   return os::get_sender_for_C_frame(&tmp);
+#else
+  return os::get_sender_for_C_frame(&topframe);
+#endif
 }
 
 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))

I tested it successfully on Linux and AIX with the fastdebug build (RedefineClasses.java with -XX:+Verbose).

What do you think?

Cheers, Richard.

@reinrich
Copy link
Contributor

@reinrich reinrich commented Dec 10, 2020

I tested it successfully on Linux and AIX with the fastdebug build (RedefineClasses.java with -XX:+Verbose).

Tests succeeded also in slowdebug and release on Linux/PPC64 (-XX:+Verbose is not supported in release though).

test/hotspot/jtreg/runtime/NMT/CheckForProperDetailStackTrace.java failed in slowdebug on AIX. But that's no showstopper, is it?
My attempt to build also release on AIX failed because of an internal compiler error raised building libharfbuzz.

@TheRealMDoerr
Copy link
Contributor Author

@TheRealMDoerr TheRealMDoerr commented Dec 11, 2020

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

@reinrich
Copy link
Contributor

@reinrich reinrich commented Dec 14, 2020

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:
/priv/d038402/git/reinrich/jdk2/src/hotspot/os_cpu/linux_ppc/os_linux_ppc.cpp:
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

@TheRealMDoerr
Copy link
Contributor Author

@TheRealMDoerr TheRealMDoerr commented Dec 14, 2020

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.

@TheRealMDoerr TheRealMDoerr deleted the 8256843_ppc64_frame_v2 branch Dec 14, 2020
@reinrich
Copy link
Contributor

@reinrich reinrich commented Dec 14, 2020

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.

Me neither. I found it in the implementation for x86_64 and learned that xlC supports it as well.

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?

According to the documentation (https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html) this is not really safe.

Calling this function with a nonzero argument can have unpredictable effects, including crashing the calling program

So I wouldn't do it.

We could still use it in os::current_stack_pointer() and in os::current_frame().

Anyway, I like your proposal. Would you mind creating a PR for jdk16 as we need it there? I'm closing this one.

That's fine for me.

Thanks, Richard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants