8314225: SIGSEGV in JavaThread::is_lock_owned

Reviewed-by: dlong, dholmes

src/hotspot/share/jfr/leakprofiler/checkpoint/rootResolver.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2024, 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
@@ -248,12 +248,6 @@ bool ReferenceToThreadRootClosure::do_thread_stack_detailed(JavaThread* jt) {
   ReferenceLocateClosure rcl(_callback, OldObjectRoot::_threads, OldObjectRoot::_stack_variable, jt);
 
   if (jt->has_last_Java_frame()) {
-    // Traverse the monitor chunks
-    MonitorChunk* chunk = jt->monitor_chunks();
-    for (; chunk != nullptr; chunk = chunk->next()) {
-      chunk->oops_do(&rcl);
-    }
-
     if (rcl.complete()) {
       return true;
     }

src/hotspot/share/runtime/deoptimization.cpp

@@ -1736,7 +1736,7 @@ void Deoptimization::pop_frames_failed_reallocs(JavaThread* thread, vframeArray*
           ObjectSynchronizer::exit(src->obj(), src->lock(), thread);
         }
       }
-      array->element(i)->free_monitors(thread);
+      array->element(i)->free_monitors();
 #ifdef ASSERT
       array->element(i)->set_removed_monitors();
 #endif

src/hotspot/share/runtime/javaThread.cpp

@@ -430,8 +430,6 @@ JavaThread::JavaThread() :
   _active_handles(nullptr),
   _free_handle_block(nullptr),
 
-  _monitor_chunks(nullptr),
-
   _suspend_flags(0),
 
   _thread_state(_thread_new),
@@ -1050,13 +1048,7 @@ JavaThread* JavaThread::active() {
 
 bool JavaThread::is_lock_owned(address adr) const {
   assert(LockingMode != LM_LIGHTWEIGHT, "should not be called with new lightweight locking");
-  if (Thread::is_lock_owned(adr)) return true;
-
-  for (MonitorChunk* chunk = monitor_chunks(); chunk != nullptr; chunk = chunk->next()) {
-    if (chunk->contains(adr)) return true;
-  }
-
-  return false;
+  return is_in_full_stack(adr);
 }
 
 oop JavaThread::exception_oop() const {
@@ -1067,22 +1059,6 @@ void JavaThread::set_exception_oop(oop o) {
   Atomic::store(&_exception_oop, o);
 }
 
-void JavaThread::add_monitor_chunk(MonitorChunk* chunk) {
-  chunk->set_next(monitor_chunks());
-  set_monitor_chunks(chunk);
-}
-
-void JavaThread::remove_monitor_chunk(MonitorChunk* chunk) {
-  guarantee(monitor_chunks() != nullptr, "must be non empty");
-  if (monitor_chunks() == chunk) {
-    set_monitor_chunks(chunk->next());
-  } else {
-    MonitorChunk* prev = monitor_chunks();
-    while (prev->next() != chunk) prev = prev->next();
-    prev->set_next(chunk->next());
-  }
-}
-
 void JavaThread::handle_special_runtime_exit_condition() {
   if (is_obj_deopt_suspend()) {
     frame_anchor()->make_walkable();
@@ -1408,13 +1384,6 @@ void JavaThread::oops_do_no_frames(OopClosure* f, NMethodClosure* cf) {
 
   DEBUG_ONLY(verify_frame_info();)
 
-  if (has_last_Java_frame()) {
-    // Traverse the monitor chunks
-    for (MonitorChunk* chunk = monitor_chunks(); chunk != nullptr; chunk = chunk->next()) {
-      chunk->oops_do(f);
-    }
-  }
-
   assert(vframe_array_head() == nullptr, "deopt in progress at a safepoint!");
   // If we have deferred set_locals there might be oops waiting to be
   // written

src/hotspot/share/runtime/javaThread.hpp

@@ -193,10 +193,6 @@ class JavaThread: public Thread {
   void pop_jni_handle_block();
 
  private:
-  MonitorChunk* _monitor_chunks;              // Contains the off stack monitors
-                                              // allocated during deoptimization
-                                              // and by JNI_MonitorEnter/Exit
-
   enum SuspendFlags {
     // NOTE: avoid using the sign-bit as cc generates different test code
     //       when the sign-bit is used, and sometimes incorrectly - see CR 6398077
@@ -679,7 +675,7 @@ private:
     return (_suspend_flags & (_obj_deopt JFR_ONLY(| _trace_flag))) != 0;
   }
 
-  // Fast-locking support
+  // Stack-locking support (not for LM_LIGHTWEIGHT)
   bool is_lock_owned(address adr) const;
 
   // Accessors for vframe array top
@@ -881,13 +877,7 @@ private:
   int depth_first_number() { return _depth_first_number; }
   void set_depth_first_number(int dfn) { _depth_first_number = dfn; }
 
- private:
-  void set_monitor_chunks(MonitorChunk* monitor_chunks) { _monitor_chunks = monitor_chunks; }
-
  public:
-  MonitorChunk* monitor_chunks() const           { return _monitor_chunks; }
-  void add_monitor_chunk(MonitorChunk* chunk);
-  void remove_monitor_chunk(MonitorChunk* chunk);
   bool in_deopt_handler() const                  { return _in_deopt_handler > 0; }
   void inc_in_deopt_handler()                    { _in_deopt_handler++; }
   void dec_in_deopt_handler() {

src/hotspot/share/runtime/monitorChunk.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2024, 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
@@ -30,7 +30,6 @@
 MonitorChunk::MonitorChunk(int number_on_monitors) {
   _number_of_monitors = number_on_monitors;
   _monitors           = NEW_C_HEAP_ARRAY(BasicObjectLock, number_on_monitors, mtSynchronizer);
-  _next               = nullptr;
 }
 
 

src/hotspot/share/runtime/monitorChunk.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2024, 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
@@ -38,23 +38,17 @@ class MonitorChunk: public CHeapObj<mtSynchronizer> {
   int              _number_of_monitors;
   BasicObjectLock* _monitors;
   BasicObjectLock* monitors() const { return _monitors; }
-  MonitorChunk*    _next;
  public:
   // Constructor
   MonitorChunk(int number_on_monitors);
   ~MonitorChunk();
 
-  // link operations
-  MonitorChunk* next() const                { return _next; }
-  void set_next(MonitorChunk* next)         { _next = next; }
-
   // Returns the number of monitors
   int number_of_monitors() const { return _number_of_monitors; }
 
   // Returns the index'th monitor
   BasicObjectLock* at(int index)            { assert(index >= 0 && index < number_of_monitors(), "out of bounds check"); return &monitors()[index]; }
 
-
   // Memory management
   void oops_do(OopClosure* f);
 

src/hotspot/share/runtime/synchronizer.cpp

@@ -1056,7 +1056,9 @@ intptr_t ObjectSynchronizer::FastHashCode(Thread* current, oop obj) {
       }
       // Fall thru so we only have one place that installs the hash in
       // the ObjectMonitor.
-    } else if (LockingMode == LM_LEGACY && mark.has_locker() && current->is_lock_owned((address)mark.locker())) {
+    } else if (LockingMode == LM_LEGACY && mark.has_locker()
+               && current->is_Java_thread()
+               && JavaThread::cast(current)->is_lock_owned((address)mark.locker())) {
       // This is a stack-lock owned by the calling thread so fetch the
       // displaced markWord from the BasicLock on the stack.
       temp = mark.displaced_mark_helper();

src/hotspot/share/runtime/thread.cpp

@@ -527,16 +527,6 @@ void Thread::print_owned_locks_on(outputStream* st) const {
 }
 #endif // ASSERT
 
-// We had to move these methods here, because vm threads get into ObjectSynchronizer::enter
-// However, there is a note in JavaThread::is_lock_owned() about the VM threads not being
-// used for compilation in the future. If that change is made, the need for these methods
-// should be revisited, and they should be removed if possible.
-
-bool Thread::is_lock_owned(address adr) const {
-  assert(LockingMode != LM_LIGHTWEIGHT, "should not be called with new lightweight locking");
-  return is_in_full_stack(adr);
-}
-
 bool Thread::set_as_starting_thread() {
   assert(_starting_thread == nullptr, "already initialized: "
          "_starting_thread=" INTPTR_FORMAT, p2i(_starting_thread));

src/hotspot/share/runtime/thread.hpp

@@ -475,9 +475,6 @@ class Thread: public ThreadShadow {
   }
 
  public:
-  // Used by fast lock support
-  virtual bool is_lock_owned(address adr) const;
-
   // Check if address is within the given range of this thread's
   // stack:  stack_base() > adr >= limit
   bool is_in_stack_range_incl(address adr, address limit) const {

src/hotspot/share/runtime/vframeArray.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2024, 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
@@ -37,6 +37,7 @@
 #include "runtime/handles.inline.hpp"
 #include "runtime/monitorChunk.hpp"
 #include "runtime/sharedRuntime.hpp"
+#include "runtime/synchronizer.hpp"
 #include "runtime/vframe.hpp"
 #include "runtime/vframeArray.hpp"
 #include "runtime/vframe_hp.hpp"
@@ -48,11 +49,10 @@
 
 int vframeArrayElement:: bci(void) const { return (_bci == SynchronizationEntryBCI ? 0 : _bci); }
 
-void vframeArrayElement::free_monitors(JavaThread* jt) {
+void vframeArrayElement::free_monitors() {
   if (_monitors != nullptr) {
      MonitorChunk* chunk = _monitors;
      _monitors = nullptr;
-     jt->remove_monitor_chunk(chunk);
      delete chunk;
   }
 }
@@ -72,7 +72,7 @@ void vframeArrayElement::fill_in(compiledVFrame* vf, bool realloc_failures) {
   int index;
 
   {
-    Thread* current_thread = Thread::current();
+    JavaThread* current_thread = JavaThread::current();
     ResourceMark rm(current_thread);
     HandleMark hm(current_thread);
 
@@ -85,7 +85,6 @@ void vframeArrayElement::fill_in(compiledVFrame* vf, bool realloc_failures) {
 
       // Allocate monitor chunk
       _monitors = new MonitorChunk(list->length());
-      vf->thread()->add_monitor_chunk(_monitors);
 
       // Migrate the BasicLocks from the stack to the monitor chunk
       for (index = 0; index < list->length(); index++) {
@@ -95,9 +94,16 @@ void vframeArrayElement::fill_in(compiledVFrame* vf, bool realloc_failures) {
         if (monitor->owner_is_scalar_replaced()) {
           dest->set_obj(nullptr);
         } else {
-          assert(monitor->owner() == nullptr || !monitor->owner()->is_unlocked(), "object must be null or locked");
+          assert(monitor->owner() != nullptr, "monitor owner must not be null");
+          assert(!monitor->owner()->is_unlocked(), "monitor must be locked");
           dest->set_obj(monitor->owner());
+          assert(ObjectSynchronizer::current_thread_holds_lock(current_thread, Handle(current_thread, dest->obj())),
+                 "should be held, before move_to");
+
           monitor->lock()->move_to(monitor->owner(), dest->lock());
+
+          assert(ObjectSynchronizer::current_thread_holds_lock(current_thread, Handle(current_thread, dest->obj())),
+                 "should be held, after move_to");
         }
       }
     }
@@ -308,7 +314,11 @@ void vframeArrayElement::unpack_on_stack(int caller_actual_parameters,
     top = iframe()->previous_monitor_in_interpreter_frame(top);
     BasicObjectLock* src = _monitors->at(index);
     top->set_obj(src->obj());
+    assert(src->obj() != nullptr || ObjectSynchronizer::current_thread_holds_lock(thread, Handle(thread, src->obj())),
+           "should be held, before move_to");
     src->lock()->move_to(src->obj(), top->lock());
+    assert(src->obj() != nullptr || ObjectSynchronizer::current_thread_holds_lock(thread, Handle(thread, src->obj())),
+           "should be held, after move_to");
   }
   if (ProfileInterpreter) {
     iframe()->interpreter_frame_set_mdp(0); // clear out the mdp.
@@ -649,9 +659,8 @@ void vframeArray::unpack_to_stack(frame &unpack_frame, int exec_mode, int caller
 }
 
 void vframeArray::deallocate_monitor_chunks() {
-  JavaThread* jt = JavaThread::current();
   for (int index = 0; index < frames(); index++ ) {
-     element(index)->free_monitors(jt);
+     element(index)->free_monitors();
   }
 }
 

src/hotspot/share/runtime/vframeArray.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2024, 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
@@ -77,7 +77,7 @@ class vframeArrayElement {
 
   MonitorChunk* monitors(void) const { return _monitors; }
 
-  void free_monitors(JavaThread* jt);
+  void free_monitors();
 
   StackValueCollection* locals(void) const             { return _locals; }