ZGC: Review 6

Author: fisk

src/hotspot/share/c1/c1_Runtime1.cpp

@@ -512,7 +512,16 @@ JRT_ENTRY_NO_ASYNC(static address, exception_handler_for_pc_helper(JavaThread* t
   // This function is called when we are about to throw an exception. Therefore,
   // we have to poll the stack watermark barrier to make sure that not yet safe
   // stack frames are made safe before returning into them.
-  StackWatermarkSet::on_unwind(thread);
+  if (thread->last_frame().is_runtime_frame()) {
+    // The Runtime1::handle_exception_from_callee_id handler is invoked after the
+    // frame has been unwinded. It instead builds its own stub frame, to call the
+    // runtime. But the throwing frame has already been unwinded here.
+    StackWatermarkSet::after_unwind(thread);
+  } else {
+    // The other C1 exception handler call in here before the throwing frame has
+    // been unwinded.
+    StackWatermarkSet::before_unwind(thread);
+  }
 
   nm = CodeCache::find_nmethod(pc);
   assert(nm != NULL, "this is not an nmethod");

src/hotspot/share/interpreter/interpreterRuntime.cpp

@@ -1163,7 +1163,7 @@ JRT_ENTRY(void, InterpreterRuntime::at_safepoint(JavaThread* thread))
     // This function is called by the interpreter when single stepping. Such single
     // stepping could unwind a frame. Then, it is important that we process any frames
     // that we might return into.
-    StackWatermarkSet::on_unwind(thread);
+    StackWatermarkSet::before_unwind(thread);
 
     // We are called during regular safepoints and when the VM is
     // single stepping. If any thread is marked for single stepping,
@@ -1184,7 +1184,7 @@ JRT_ENTRY(void, InterpreterRuntime::at_unwind(JavaThread* thread))
   // to single step when unwinding frames for an exception being thrown. Instead,
   // such single stepping code will use the safepoint table, which will use the
   // InterpreterRuntime::at_safepoint callback.
-  StackWatermarkSet::on_unwind(thread);
+  StackWatermarkSet::before_unwind(thread);
 JRT_END
 
 JRT_ENTRY(void, InterpreterRuntime::post_field_access(JavaThread *thread, oopDesc* obj,

src/hotspot/share/opto/runtime.cpp

@@ -1290,7 +1290,7 @@ JRT_ENTRY_NO_ASYNC(address, OptoRuntime::handle_exception_C_helper(JavaThread* t
   // We get here when we are about to throw an exception. The exception could
   // return into a not yet safe to use frame. We catch such conditions in the
   // following stack watermark barrier poll.
-  StackWatermarkSet::on_unwind(thread);
+  StackWatermarkSet::before_unwind(thread);
 
   // Do not confuse exception_oop with pending_exception. The exception_oop
   // is only used to pass arguments into the method. Not for general
@@ -1472,7 +1472,7 @@ address OptoRuntime::rethrow_C(oopDesc* exception, JavaThread* thread, address r
   // The frame we rethrow the exception to might not have been processed by the GC yet.
   // The stack watermark barrier takes care of detecting that and ensuring the frame
   // has updated oops.
-  StackWatermarkSet::on_unwind(thread);
+  StackWatermarkSet::after_unwind(thread);
 
 #ifndef PRODUCT
   SharedRuntime::_rethrow_ctr++;               // count rethrows

src/hotspot/share/runtime/deoptimization.cpp

@@ -258,7 +258,7 @@ Deoptimization::UnrollBlock* Deoptimization::fetch_unroll_info_helper(JavaThread
   // When we get here we are about to unwind a frame. In order to catch not yet
   // safe to use frames, the following stack watermark barrier poll will make
   // such frames safe to use.
-  StackWatermarkSet::on_unwind(thread);
+  StackWatermarkSet::before_unwind(thread);
 
   // Note: there is a safepoint safety issue here. No matter whether we enter
   // via vanilla deopt or uncommon trap we MUST NOT stop at a safepoint once

src/hotspot/share/runtime/safepoint.cpp

@@ -995,9 +995,9 @@ void ThreadSafepointState::handle_polling_page_exception() {
     }
 
     // We get here if compiled return polls found a reason to call into the VM.
-    // One condition for that is that a frame above is not yet safe to use.
+    // One condition for that is that the top frame is not yet safe to use.
     // The following stack watermark barrier poll will catch such situations.
-    StackWatermarkSet::on_unwind(thread());
+    StackWatermarkSet::after_unwind(thread());
 
     // Process pending operation
     SafepointMechanism::process_operation_if_requested_slow(thread());

src/hotspot/share/runtime/sharedRuntime.cpp

@@ -522,7 +522,7 @@ JRT_LEAF(address, SharedRuntime::exception_handler_for_return_address(JavaThread
   // This is called when we are about to throw an exception. We must make sure the
   // frame we are unwinding to is safe to be access w.r.t. concurrent stack processing.
   // The stack watermark code will take care of ensuring that.
-  StackWatermarkSet::on_unwind(thread);
+  StackWatermarkSet::after_unwind(thread);
   return raw_exception_handler_for_return_address(thread, return_address);
 JRT_END
 
@@ -3032,7 +3032,7 @@ JRT_LEAF(intptr_t*, SharedRuntime::OSR_migration_begin( JavaThread *thread) )
   // before it gets unwinded. This is helpful as the size of the compiled frame could be
   // larger than the interpreted frame, which could result in the new frame not being
   // processed correctly.
-  StackWatermarkSet::on_unwind(thread);
+  StackWatermarkSet::before_unwind(thread);
 
   //
   // This code is dependent on the memory layout of the interpreter local

src/hotspot/share/runtime/stackWatermark.cpp

@@ -169,24 +169,9 @@ StackWatermark::~StackWatermark() {
   delete _iterator;
 }
 
-bool StackWatermark::is_frame_processed(frame fr) {
-  MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
-  uint32_t state = Atomic::load(&_state);
-  if (StackWatermarkState::epoch(state) != epoch_id()) {
-    return false;
-  }
-  if (StackWatermarkState::is_done(state)) {
-    return true;
-  }
-  if (_iterator != NULL) {
-    return reinterpret_cast<uintptr_t>(fr.sp()) <= _iterator->caller();
-  }
-  return true;
-}
-
-// A frame is "safe" it it *and* its caller has been processed. This is the invariant
+// A frame is "safe" if it *and* its caller have been processed. This is the invariant
 // that allows exposing a frame, and for that frame to directly access its caller frame
-// without going through any hooks. This is a stricter guarantee than is_frame_processed().
+// without going through any hooks.
 bool StackWatermark::is_frame_safe(frame f) {
   MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
   uint32_t state = Atomic::load(&_state);

src/hotspot/share/runtime/stackWatermark.hpp

@@ -64,7 +64,6 @@ class StackWatermark : public CHeapObj<mtInternal> {
   void yield_processing();
   static bool has_barrier(frame& f);
   void ensure_safe(frame f);
-  bool is_frame_processed(frame f);
   bool is_frame_safe(frame f);
 
   // API for consumers of the stack watermark barrier.
@@ -94,7 +93,8 @@ class StackWatermark : public CHeapObj<mtInternal> {
 
   uintptr_t last_processed();
 
-  void on_unwind();
+  void before_unwind();
+  void after_unwind();
   void on_iteration(frame fr);
   void start_iteration();
   void finish_iteration(void* context);

src/hotspot/share/runtime/stackWatermark.inline.hpp

@@ -59,40 +59,51 @@ inline void StackWatermark::ensure_safe(frame f) {
     return;
   }
 
-  assert(is_frame_processed(f), "frame should be safe before processing");
+  // Get caller sp to get the callee fp.
+  RegisterMap map(_jt, false /* update_map */, false /* process_frames */);
+  frame f_caller = f.sender(&map);
+  uintptr_t f_fp = reinterpret_cast<uintptr_t>(f_caller.sp());
 
-  if (is_above_watermark(reinterpret_cast<uintptr_t>(f.sp()), watermark())) {
+  if (is_above_watermark(f_fp, watermark())) {
     process_one();
   }
 
   assert(is_frame_safe(f), "frame should be safe after processing");
-  assert(!is_above_watermark(reinterpret_cast<uintptr_t>(f.sp()), watermark()), "");
 }
 
-inline void StackWatermark::on_unwind() {
-  const frame& f = _jt->last_frame();
-  assert(is_frame_safe(f), "frame should be safe after processing");
+inline void StackWatermark::before_unwind() {
+  frame f = _jt->last_frame();
 
-  if (f.is_first_frame()) {
-    return;
+  // Skip any stub frames etc up until the frame that triggered before_unwind().
+  RegisterMap map(_jt, false /* update_map */, false /* process_frames */);
+  if (f.is_safepoint_blob_frame() || f.is_runtime_frame()) {
+    f = f.sender(&map);
   }
 
-  // on_unwind() potentially exposes a new frame. The new exposed frame is
-  // always the caller of the top frame, but for two different reasons.
-  //
-  // 1) Return sites in nmethods unwind the frame *before* polling. In other
-  //    words, the frame of the nmethod performing the poll, will not be
-  //    on-stack when it gets to the runtime. However, it trampolines into the
-  //    runtime with a safepoint blob, which will be the top frame. Therefore,
-  //    the caller of the safepoint blob, will be the new exposed frame.
-  //
-  // 2) All other calls to on_unwind() perform the unwinding *after* polling.
-  //    Therefore, the caller of the top frame will be the new exposed frame.
+  assert(is_frame_safe(f), "frame should be safe before processing");
+  assert(!f.is_runtime_frame(), "should have skipped all runtime stubs");
 
-  RegisterMap map(_jt, false /* update_map */, false /* process_frames */);
-  const frame& caller = f.sender(&map);
+  // before_unwind() potentially exposes a new frame. The new exposed frame is
+  // always the caller of the top frame.
+  if (!f.is_first_frame()) {
+    f = f.sender(&map);
+    ensure_safe(f);
+  }
+}
+
+inline void StackWatermark::after_unwind() {
+  frame f = _jt->last_frame();
+
+  if (f.is_safepoint_blob_frame() || f.is_runtime_frame()) {
+    // Skip safepoint blob.
+    RegisterMap map(_jt, false /* update_map */, false /* process_frames */);
+    f = f.sender(&map);
+  }
+
+  assert(!f.is_runtime_frame(), "should have skipped all runtime stubs");
 
-  ensure_safe(caller);
+  // after_unwind() potentially exposes the top frame.
+  ensure_safe(f);
 }
 
 inline void StackWatermark::on_iteration(frame f) {

src/hotspot/share/runtime/stackWatermarkSet.cpp

@@ -63,15 +63,29 @@ static void verify_poll_context() {
 #endif
 }
 
-void StackWatermarkSet::on_unwind(JavaThread* jt) {
+void StackWatermarkSet::before_unwind(JavaThread* jt) {
   verify_poll_context();
   if (!jt->has_last_Java_frame()) {
     // Sometimes we throw exceptions and use native transitions on threads that
     // do not have any Java threads. Skip those callsites.
     return;
   }
   for (StackWatermark* current = jt->stack_watermark_set()->_head; current != NULL; current = current->next()) {
-    current->on_unwind();
+    current->before_unwind();
+  }
+  SafepointMechanism::update_poll_values(jt);
+}
+
+
+void StackWatermarkSet::after_unwind(JavaThread* jt) {
+  verify_poll_context();
+  if (!jt->has_last_Java_frame()) {
+    // Sometimes we throw exceptions and use native transitions on threads that
+    // do not have any Java threads. Skip those callsites.
+    return;
+  }
+  for (StackWatermark* current = jt->stack_watermark_set()->_head; current != NULL; current = current->next()) {
+    current->after_unwind();
   }
   SafepointMechanism::update_poll_values(jt);
 }

src/hotspot/share/runtime/stackWatermarkSet.hpp

@@ -42,7 +42,10 @@ class StackWatermarkSet {
 
 public:
   // Called when a thread is about to unwind a frame
-  static void on_unwind(JavaThread* jt);
+  static void before_unwind(JavaThread* jt);
+
+  // Called when a thread just unwinded a frame
+  static void after_unwind(JavaThread* jt);
 
   // Called by stack walkers when walking into a frame
   static void on_iteration(JavaThread* jt, frame fr);

src/hotspot/share/runtime/thread.cpp

@@ -2687,7 +2687,7 @@ void JavaThread::check_safepoint_and_suspend_for_native_trans(JavaThread *thread
   // After returning from native, it could be that the stack frames are not
   // yet safe to use. We catch such situations in the subsequent stack watermark
   // barrier, which will trap unsafe stack frames.
-  StackWatermarkSet::on_unwind(thread);
+  StackWatermarkSet::before_unwind(thread);
 
   JFR_ONLY(SUSPEND_THREAD_CONDITIONAL(thread);)
 }