8246378: [Windows] assert on MethodHandle logging code

Reviewed-by: iklam, vlivanov

src/hotspot/cpu/aarch64/stubRoutines_aarch64.cpp

@@ -33,7 +33,6 @@
 // Implementation of the platform-specific part of StubRoutines - for
 // a description of how to extend it, see the stubRoutines.hpp file.
 
-address StubRoutines::aarch64::_get_previous_fp_entry = NULL;
 address StubRoutines::aarch64::_get_previous_sp_entry = NULL;
 
 address StubRoutines::aarch64::_f2i_fixup = NULL;

src/hotspot/cpu/aarch64/stubRoutines_aarch64.hpp

@@ -43,7 +43,6 @@ class aarch64 {
  friend class StubGenerator;
 
  private:
-  static address _get_previous_fp_entry;
   static address _get_previous_sp_entry;
 
   static address _f2i_fixup;
@@ -77,11 +76,6 @@ class aarch64 {
 
  public:
 
-  static address get_previous_fp_entry()
-  {
-    return _get_previous_fp_entry;
-  }
-
   static address get_previous_sp_entry()
   {
     return _get_previous_sp_entry;

src/hotspot/cpu/x86/methodHandles_x86.cpp

@@ -528,6 +528,12 @@ void trace_method_handle_stub(const char* adaptername,
     }
     tty->cr();
 
+    // Note: We want to allow trace_method_handle from any call site.
+    // While trace_method_handle creates a frame, it may be entered
+    // without a PC on the stack top (e.g. not just after a call).
+    // Walking that frame could lead to failures due to that invalid PC.
+    // => carefully detect that frame when doing the stack walking
+
     {
       // dumping last frame with frame::describe
 
@@ -536,43 +542,38 @@ void trace_method_handle_stub(const char* adaptername,
       PRESERVE_EXCEPTION_MARK; // may not be needed but safer and inexpensive here
       FrameValues values;
 
-      // Note: We want to allow trace_method_handle from any call site.
-      // While trace_method_handle creates a frame, it may be entered
-      // without a PC on the stack top (e.g. not just after a call).
-      // Walking that frame could lead to failures due to that invalid PC.
-      // => carefully detect that frame when doing the stack walking
-
       // Current C frame
       frame cur_frame = os::current_frame();
 
-      // Robust search of trace_calling_frame (independant of inlining).
+      if (cur_frame.fp() != 0) {  // not walkable
-      // Assumes saved_regs comes from a pusha in the trace_calling_frame.
-      assert(cur_frame.sp() < saved_regs, "registers not saved on stack ?");
-      frame trace_calling_frame = os::get_sender_for_C_frame(&cur_frame);
-      while (trace_calling_frame.fp() < saved_regs) {
-        trace_calling_frame = os::get_sender_for_C_frame(&trace_calling_frame);
-      }
 
-      // safely create a frame and call frame::describe
+        // Robust search of trace_calling_frame (independent of inlining).
-      intptr_t *dump_sp = trace_calling_frame.sender_sp();
+        // Assumes saved_regs comes from a pusha in the trace_calling_frame.
-      intptr_t *dump_fp = trace_calling_frame.link();
+        assert(cur_frame.sp() < saved_regs, "registers not saved on stack ?");
+        frame trace_calling_frame = os::get_sender_for_C_frame(&cur_frame);
+        while (trace_calling_frame.fp() < saved_regs) {
+          trace_calling_frame = os::get_sender_for_C_frame(&trace_calling_frame);
+        }
 
-      bool walkable = has_mh; // whether the traced frame shoud be walkable
+        // safely create a frame and call frame::describe
+        intptr_t *dump_sp = trace_calling_frame.sender_sp();
+        intptr_t *dump_fp = trace_calling_frame.link();
 
-      if (walkable) {
+        if (has_mh) {
-        // The previous definition of walkable may have to be refined
+          // The previous definition of walkable may have to be refined
-        // if new call sites cause the next frame constructor to start
+          // if new call sites cause the next frame constructor to start
-        // failing. Alternatively, frame constructors could be
+          // failing. Alternatively, frame constructors could be
-        // modified to support the current or future non walkable
+          // modified to support the current or future non walkable
-        // frames (but this is more intrusive and is not considered as
+          // frames (but this is more intrusive and is not considered as
-        // part of this RFE, which will instead use a simpler output).
+          // part of this RFE, which will instead use a simpler output).
-        frame dump_frame = frame(dump_sp, dump_fp);
+          frame dump_frame = frame(dump_sp, dump_fp);
-        dump_frame.describe(values, 1);
+          dump_frame.describe(values, 1);
-      } else {
+        } else {
-        // Stack may not be walkable (invalid PC above FP):
+          // Stack may not be walkable (invalid PC above FP):
-        // Add descriptions without building a Java frame to avoid issues
+          // Add descriptions without building a Java frame to avoid issues
-        values.describe(-1, dump_fp, "fp for #1 <not parsed, cannot trust pc>");
+          values.describe(-1, dump_fp, "fp for #1 <not parsed, cannot trust pc>");
-        values.describe(-1, dump_sp, "sp for #1");
+          values.describe(-1, dump_sp, "sp for #1");
+        }
       }
       values.describe(-1, entry_sp, "raw top of stack");
 

src/hotspot/cpu/x86/stubGenerator_x86_64.cpp

@@ -566,26 +566,6 @@ class StubGenerator: public StubCodeGenerator {
     return start;
   }
 
-  // Support for intptr_t get_previous_fp()
-  //
-  // This routine is used to find the previous frame pointer for the
-  // caller (current_frame_guess). This is used as part of debugging
-  // ps() is seemingly lost trying to find frames.
-  // This code assumes that caller current_frame_guess) has a frame.
-  address generate_get_previous_fp() {
-    StubCodeMark mark(this, "StubRoutines", "get_previous_fp");
-    const Address old_fp(rbp, 0);
-    const Address older_fp(rax, 0);
-    address start = __ pc();
-
-    __ enter();
-    __ movptr(rax, old_fp); // callers fp
-    __ movptr(rax, older_fp); // the frame for ps()
-    __ pop(rbp);
-    __ ret(0);
-
-    return start;
-  }
 
   // Support for intptr_t get_previous_sp()
   //
@@ -6717,7 +6697,6 @@ address generate_avx_ghash_processBlocks() {
     StubRoutines::_fence_entry                = generate_orderaccess_fence();
 
     // platform dependent
-    StubRoutines::x86::_get_previous_fp_entry = generate_get_previous_fp();
     StubRoutines::x86::_get_previous_sp_entry = generate_get_previous_sp();
 
     StubRoutines::x86::_verify_mxcsr_entry    = generate_verify_mxcsr();

src/hotspot/cpu/x86/stubRoutines_x86.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2013, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -42,7 +42,6 @@ class x86 {
 
 #ifdef _LP64
  private:
-  static address _get_previous_fp_entry;
   static address _get_previous_sp_entry;
 
   static address _f2i_fixup;
@@ -57,10 +56,6 @@ class x86 {
 
  public:
 
-  static address get_previous_fp_entry() {
-    return _get_previous_fp_entry;
-  }
-
   static address get_previous_sp_entry() {
     return _get_previous_sp_entry;
   }

src/hotspot/cpu/x86/stubRoutines_x86_64.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -31,7 +31,6 @@
 // Implementation of the platform-specific part of StubRoutines - for
 // a description of how to extend it, see the stubRoutines.hpp file.
 
-address StubRoutines::x86::_get_previous_fp_entry = NULL;
 address StubRoutines::x86::_get_previous_sp_entry = NULL;
 
 address StubRoutines::x86::_f2i_fixup = NULL;

src/hotspot/os_cpu/windows_aarch64/os_windows_aarch64.cpp

@@ -142,32 +142,14 @@ bool os::win32::get_frame_at_stack_banging_point(JavaThread* thread,
   return true;
 }
 
-// By default, gcc always saves frame pointer rfp on this stack. This
-// may get turned off by -fomit-frame-pointer.
 frame os::get_sender_for_C_frame(frame* fr) {
-  return frame(fr->link(), fr->link(), fr->sender_pc());
+  ShouldNotReachHere();
+  return frame();
 }
 
 frame os::current_frame() {
-  typedef intptr_t*      get_fp_func           ();
+  return frame();  // cannot walk Windows frames this way.  See os::get_native_stack
-  get_fp_func* func = CAST_TO_FN_PTR(get_fp_func*,
+                   // and os::platform_print_native_stack
-                                     StubRoutines::aarch64::get_previous_fp_entry());
-  if (func == NULL) return frame();
-  intptr_t* fp = (*func)();
-  if (fp == NULL) {
-    return frame();
-  }
-
-  frame myframe((intptr_t*)os::current_stack_pointer(),
-                (intptr_t*)fp,
-                CAST_FROM_FN_PTR(address, os::current_frame));
-  if (os::is_first_C_frame(&myframe)) {
-
-    // stack is not walkable
-    return frame();
-  } else {
-    return os::get_sender_for_C_frame(&myframe);
-  }
 }
 
 ////////////////////////////////////////////////////////////////////////////////

src/hotspot/os_cpu/windows_x86/os_windows_x86.cpp

@@ -323,13 +323,6 @@ frame os::fetch_frame_from_context(const void* ucVoid) {
   return frame(sp, fp, epc);
 }
 
-// VC++ does not save frame pointer on stack in optimized build. It
-// can be turned off by /Oy-. If we really want to walk C frames,
-// we can use the StackWalk() API.
-frame os::get_sender_for_C_frame(frame* fr) {
-  return frame(fr->sender_sp(), fr->link(), fr->sender_pc());
-}
-
 #ifndef AMD64
 // Ignore "C4172: returning address of local variable or temporary" on 32bit
 PRAGMA_DIAG_PUSH
@@ -390,49 +383,18 @@ bool os::win32::get_frame_at_stack_banging_point(JavaThread* thread,
   return true;
 }
 
-#ifndef AMD64
+
-intptr_t* _get_previous_fp() {
+// VC++ does not save frame pointer on stack in optimized build. It
-  intptr_t **frameptr;
+// can be turned off by /Oy-. If we really want to walk C frames,
-  __asm {
+// we can use the StackWalk() API.
-    mov frameptr, ebp
+frame os::get_sender_for_C_frame(frame* fr) {
-  };
+  ShouldNotReachHere();
-  // ebp (frameptr) is for this frame (_get_previous_fp). We want the ebp for the
+  return frame();
-  // caller of os::current_frame*(), so go up two frames. However, for
-  // optimized builds, _get_previous_fp() will be inlined, so only go
-  // up 1 frame in that case.
-#ifdef _NMT_NOINLINE_
-  return **(intptr_t***)frameptr;
-#else
-  return *frameptr;
-#endif
 }
-#endif // !AMD64
 
 frame os::current_frame() {
-
+  return frame();  // cannot walk Windows frames this way.  See os::get_native_stack
-#ifdef AMD64
+                   // and os::platform_print_native_stack
-  // apparently _asm not supported on windows amd64
-  typedef intptr_t*      get_fp_func           ();
-  get_fp_func* func = CAST_TO_FN_PTR(get_fp_func*,
-                                     StubRoutines::x86::get_previous_fp_entry());
-  if (func == NULL) return frame();
-  intptr_t* fp = (*func)();
-  if (fp == NULL) {
-    return frame();
-  }
-#else
-  intptr_t* fp = _get_previous_fp();
-#endif // AMD64
-
-  frame myframe((intptr_t*)os::current_stack_pointer(),
-                (intptr_t*)fp,
-                CAST_FROM_FN_PTR(address, os::current_frame));
-  if (os::is_first_C_frame(&myframe)) {
-    // stack is not walkable
-    return frame();
-  } else {
-    return os::get_sender_for_C_frame(&myframe);
-  }
 }
 
 void os::print_context(outputStream *st, const void *context) {

src/hotspot/share/runtime/os.cpp

@@ -1170,9 +1170,13 @@ void os::print_location(outputStream* st, intptr_t x, bool verbose) {
 }
 
 // Looks like all platforms can use the same function to check if C
-// stack is walkable beyond current frame. The check for fp() is not
+// stack is walkable beyond current frame.
-// necessary on Sparc, but it's harmless.
 bool os::is_first_C_frame(frame* fr) {
+
+#ifdef _WINDOWS
+  return true; // native stack isn't walkable on windows this way.
+#endif
+
   // Load up sp, fp, sender sp and sender fp, check for reasonable values.
   // Check usp first, because if that's bad the other accessors may fault
   // on some architectures.  Ditto ufp second, etc.

test/hotspot/jtreg/runtime/logging/RedefineClasses.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -31,7 +31,7 @@
  *          java.instrument
  * @requires vm.jvmti
  * @run main RedefineClassHelper
- * @run main/othervm -Xmx256m -XX:MaxMetaspaceSize=64m -javaagent:redefineagent.jar -Xlog:all=trace:file=all.log RedefineClasses
+ * @run main/othervm -Xmx256m -XX:MaxMetaspaceSize=64m -javaagent:redefineagent.jar -XX:+Verbose -Xlog:all=trace:file=all.log RedefineClasses
  */
 
 // package access top-level class to avoid problem with RedefineClassHelper

test/hotspot/jtreg/runtime/logging/TestMethodHandlesVerbose.java

@@ -26,10 +26,11 @@
  * @bug 8244946
  * @summary Run simple test with -XX:+Verbose and -Xlog:methodhandles.
  *
- * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -XX:+Verbose -Xlog:methodhandles TestMethodHandlesVerbose
+ * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -XX:+Verbose -Xlog:methodhandles TestMethodHandlesVerbose a b
  */
 
 public class TestMethodHandlesVerbose {
     public static void main(String[] args) {
+        System.out.println(args[0] + args[1]);
     }
 }