6699669: Hotspot server leaves synchronized block with monitor in bad state

Summary: Remove usage of _highest_lock field in Thread so that is_lock_owned won't depend on the correct update of that field.
Reviewed-by: never, dice, acorn

agent/src/share/classes/sun/jvm/hotspot/runtime/JavaThread.java

@@ -48,6 +48,8 @@ public class JavaThread extends Thread {
   private static AddressField  lastJavaPCField;
   private static CIntegerField threadStateField;
   private static AddressField  osThreadField;
+  private static AddressField  stackBaseField;
+  private static CIntegerField stackSizeField;
 
   private static JavaThreadPDAccess access;
 
@@ -83,6 +85,8 @@ public class JavaThread extends Thread {
     lastJavaPCField   = anchorType.getAddressField("_last_Java_pc");
     threadStateField  = type.getCIntegerField("_thread_state");
     osThreadField     = type.getAddressField("_osthread");
+    stackBaseField    = type.getAddressField("_stack_base");
+    stackSizeField    = type.getCIntegerField("_stack_size");
 
     UNINITIALIZED     = db.lookupIntConstant("_thread_uninitialized").intValue();
     NEW               = db.lookupIntConstant("_thread_new").intValue();
@@ -312,6 +316,14 @@ public class JavaThread extends Thread {
     return (OSThread) VMObjectFactory.newObject(OSThread.class, osThreadField.getValue(addr));
   }
 
+  public Address getStackBase() {
+    return stackBaseField.getValue();
+  }
+
+  public long getStackSize() {
+    return stackSizeField.getValue();
+  }
+
   /** Gets the Java-side thread object for this JavaThread */
   public Oop getThreadObj() {
     return VM.getVM().getObjectHeap().newOop(threadObjField.getValue(addr));
@@ -345,11 +357,18 @@ public class JavaThread extends Thread {
     if (Assert.ASSERTS_ENABLED) {
       Assert.that(VM.getVM().isDebugging(), "Not yet implemented for non-debugging system");
     }
-    Address highest = highestLock();
     Address sp      = lastSPDbg();
+    Address stackBase = getStackBase();
     // Be robust
-    if ((highest == null) || (sp == null)) return false;
-    return (highest.greaterThanOrEqual(a) && sp.lessThanOrEqual(a));
+    if (sp == null) return false;
+    return stackBase.greaterThanOrEqual(a) && sp.lessThanOrEqual(a);
+  }
+
+  public boolean isLockOwned(Address a) {
+    Address stackBase = getStackBase();
+    Address stackLimit = stackBase.addOffsetTo(-getStackSize());
+
+    return stackBase.greaterThanOrEqual(a) && stackLimit.lessThanOrEqual(a);
 
     // FIXME: should traverse MonitorArray/MonitorChunks as in VM
   }

agent/src/share/classes/sun/jvm/hotspot/runtime/Thread.java

@@ -38,7 +38,6 @@ public class Thread extends VMObject {
   private static int HAS_ASYNC_EXCEPTION;
 
   private static AddressField activeHandlesField;
-  private static AddressField highestLockField;
   private static AddressField currentPendingMonitorField;
   private static AddressField currentWaitingMonitorField;
 
@@ -60,7 +59,6 @@ public class Thread extends VMObject {
 
     tlabFieldOffset    = type.getField("_tlab").getOffset();
     activeHandlesField = type.getAddressField("_active_handles");
-    highestLockField   = type.getAddressField("_highest_lock");
     currentPendingMonitorField = type.getAddressField("_current_pending_monitor");
     currentWaitingMonitorField = type.getAddressField("_current_waiting_monitor");
   }
@@ -121,10 +119,6 @@ public class Thread extends VMObject {
     // pending exception
   }
 
-  public Address highestLock() {
-    return highestLockField.getValue(addr);
-  }
-
   public ObjectMonitor getCurrentPendingMonitor() {
     Address monitorAddr = currentPendingMonitorField.getValue(addr);
     if (monitorAddr == null) {

agent/src/share/classes/sun/jvm/hotspot/runtime/Threads.java

@@ -164,20 +164,11 @@ public class Threads {
             }
         }
 
-        long leastDiff = 0;
-        boolean leastDiffInitialized = false;
-        JavaThread theOwner = null;
         for (JavaThread thread = first(); thread != null; thread = thread.next()) {
-            Address addr = thread.highestLock();
-            if (addr == null || addr.lessThan(o)) continue;
-            long diff = addr.minus(o);
-            if (!leastDiffInitialized || diff < leastDiff) {
-                leastDiffInitialized = true;
-                leastDiff = diff;
-                theOwner = thread;
-            }
+          if (thread.isLockOwned(o))
+            return thread;
         }
-        return theOwner;
+        return null;
     }
 
     public JavaThread owningThreadFromMonitor(ObjectMonitor monitor) {

src/share/vm/runtime/javaCalls.cpp

@@ -37,11 +37,6 @@ JavaCallWrapper::JavaCallWrapper(methodHandle callee_method, Handle receiver, Ja
   guarantee(!thread->is_Compiler_thread(), "cannot make java calls from the compiler");
   _result   = result;
 
-  // Make sure that that the value of the  higest_lock is at least the same as the current stackpointer,
-  // since, the Java code is highly likely to use locks.
-  // Use '(address)this' to guarantee that highest_lock address is conservative and inside our thread
-  thread->update_highest_lock((address)this);
-
   // Allocate handle block for Java code. This must be done before we change thread_state to _thread_in_Java_or_stub,
   // since it can potentially block.
   JNIHandleBlock* new_handles = JNIHandleBlock::allocate_block(thread);

src/share/vm/runtime/synchronizer.cpp

@@ -1117,10 +1117,10 @@ ObjectMonitor * ATTR ObjectSynchronizer::inflate (Thread * Self, oop object) {
 
           // Optimization: if the mark->locker stack address is associated
           // with this thread we could simply set m->_owner = Self and
-          // m->OwnerIsThread = 1.  Note that a thread can inflate an object
+          // m->OwnerIsThread = 1. Note that a thread can inflate an object
           // that it has stack-locked -- as might happen in wait() -- directly
           // with CAS.  That is, we can avoid the xchg-NULL .... ST idiom.
-          m->set_owner (mark->locker());
+          m->set_owner(mark->locker());
           m->set_object(object);
           // TODO-FIXME: assert BasicLock->dhw != 0.
 
@@ -1214,10 +1214,9 @@ void ObjectSynchronizer::fast_enter(Handle obj, BasicLock* lock, bool attempt_re
       BiasedLocking::revoke_at_safepoint(obj);
     }
     assert(!obj->mark()->has_bias_pattern(), "biases should be revoked by now");
-  }
+ }
 
-  THREAD->update_highest_lock((address)lock);
-  slow_enter (obj, lock, THREAD) ;
+ slow_enter (obj, lock, THREAD) ;
 }
 
 void ObjectSynchronizer::fast_exit(oop object, BasicLock* lock, TRAPS) {

src/share/vm/runtime/thread.cpp

@@ -128,7 +128,6 @@ Thread::Thread() {
   debug_only(_allow_allocation_count = 0;)
   NOT_PRODUCT(_allow_safepoint_count = 0;)
   CHECK_UNHANDLED_OOPS_ONLY(_gc_locked_out_count = 0;)
-  _highest_lock = NULL;
   _jvmti_env_iteration_count = 0;
   _vm_operation_started_count = 0;
   _vm_operation_completed_count = 0;
@@ -790,19 +789,6 @@ void Thread::check_for_valid_safepoint_state(bool potential_vm_operation) {
 }
 #endif
 
-bool Thread::lock_is_in_stack(address adr) const {
-  assert(Thread::current() == this, "lock_is_in_stack can only be called from current thread");
-  // High limit: highest_lock is set during thread execution
-  // Low  limit: address of the local variable dummy, rounded to 4K boundary.
-  // (The rounding helps finding threads in unsafe mode, even if the particular stack
-  // frame has been popped already.  Correct as long as stacks are at least 4K long and aligned.)
-  address end = os::current_stack_pointer();
-  if (_highest_lock >= adr && adr >= end) return true;
-
-  return false;
-}
-
-
 bool Thread::is_in_stack(address adr) const {
   assert(Thread::current() == this, "is_in_stack can only be called from current thread");
   address end = os::current_stack_pointer();
@@ -818,8 +804,7 @@ bool Thread::is_in_stack(address adr) const {
 // should be revisited, and they should be removed if possible.
 
 bool Thread::is_lock_owned(address adr) const {
-  if (lock_is_in_stack(adr) ) return true;
-  return false;
+  return (_stack_base >= adr && adr >= (_stack_base - _stack_size));
 }
 
 bool Thread::set_as_starting_thread() {
@@ -1664,7 +1649,7 @@ JavaThread* JavaThread::active() {
 }
 
 bool JavaThread::is_lock_owned(address adr) const {
-  if (lock_is_in_stack(adr)) return true;
+  if (Thread::is_lock_owned(adr)) return true;
 
   for (MonitorChunk* chunk = monitor_chunks(); chunk != NULL; chunk = chunk->next()) {
     if (chunk->contains(adr)) return true;
@@ -2443,7 +2428,7 @@ void JavaThread::print_on(outputStream *st) const {
   if (thread_oop != NULL && java_lang_Thread::is_daemon(thread_oop))  st->print("daemon ");
   Thread::print_on(st);
   // print guess for valid stack memory region (assume 4K pages); helps lock debugging
-  st->print_cr("[" INTPTR_FORMAT ".." INTPTR_FORMAT "]", (intptr_t)last_Java_sp() & ~right_n_bits(12), highest_lock());
+  st->print_cr("[" INTPTR_FORMAT "]", (intptr_t)last_Java_sp() & ~right_n_bits(12));
   if (thread_oop != NULL && JDK_Version::is_gte_jdk15x_version()) {
     st->print_cr("   java.lang.Thread.State: %s", java_lang_Thread::thread_status_name(thread_oop));
   }
@@ -3733,25 +3718,13 @@ JavaThread *Threads::owning_thread_from_monitor_owner(address owner, bool doLock
   // heavyweight monitors, then the owner is the stack address of the
   // Lock Word in the owning Java thread's stack.
   //
-  // We can't use Thread::is_lock_owned() or Thread::lock_is_in_stack() because
-  // those routines rely on the "current" stack pointer. That would be our
-  // stack pointer which is not relevant to the question. Instead we use the
-  // highest lock ever entered by the thread and find the thread that is
-  // higher than and closest to our target stack address.
-  //
-  address    least_diff = 0;
-  bool       least_diff_initialized = false;
   JavaThread* the_owner = NULL;
   {
     MutexLockerEx ml(doLock ? Threads_lock : NULL);
     ALL_JAVA_THREADS(q) {
-      address addr = q->highest_lock();
-      if (addr == NULL || addr < owner) continue;  // thread has entered no monitors or is too low
-      address diff = (address)(addr - owner);
-      if (!least_diff_initialized || diff < least_diff) {
-        least_diff_initialized = true;
-        least_diff = diff;
+      if (q->is_lock_owned(owner)) {
         the_owner = q;
+        break;
       }
     }
   }

src/share/vm/runtime/thread.hpp

@@ -200,14 +200,6 @@ class Thread: public ThreadShadow {
   friend class ThreadLocalStorage;
   friend class GC_locker;
 
-  // In order for all threads to be able to use fast locking, we need to know the highest stack
-  // address of where a lock is on the stack (stacks normally grow towards lower addresses). This
-  // variable is initially set to NULL, indicating no locks are used by the thread. During the thread's
-  // execution, it will be set whenever locking can happen, i.e., when we call out to Java code or use
-  // an ObjectLocker. The value is never decreased, hence, it will over the lifetime of a thread
-  // approximate the real stackbase.
-  address _highest_lock;                         // Highest stack address where a JavaLock exist
-
   ThreadLocalAllocBuffer _tlab;                  // Thread-local eden
 
   int   _vm_operation_started_count;             // VM_Operation support
@@ -400,18 +392,14 @@ public:
   // Sweeper support
   void nmethods_do();
 
-  // Fast-locking support
-  address highest_lock() const                   { return _highest_lock; }
-  void update_highest_lock(address base)         { if (base > _highest_lock) _highest_lock = base; }
-
   // Tells if adr belong to this thread. This is used
   // for checking if a lock is owned by the running thread.
-  // Warning: the method can only be used on the running thread
-  // Fast lock support uses these methods
-  virtual bool lock_is_in_stack(address adr) const;
+
+  // Used by fast lock support
   virtual bool is_lock_owned(address adr) const;
 
   // Check if address is in the stack of the thread (not just for locks).
+  // Warning: the method can only be used on the running thread
   bool is_in_stack(address adr) const;
 
   // Sets this thread as starting thread. Returns failure if thread

src/share/vm/runtime/vmStructs.cpp

@@ -656,7 +656,6 @@ static inline uint64_t cast_uint64_t(size_t x)
                                                                                                                                      \
    volatile_nonstatic_field(Thread,            _suspend_flags,                                uint32_t)                              \
   nonstatic_field(Thread,                      _active_handles,                               JNIHandleBlock*)                       \
-  nonstatic_field(Thread,                      _highest_lock,                                 address)                               \
   nonstatic_field(Thread,                      _tlab,                                         ThreadLocalAllocBuffer)                \
   nonstatic_field(Thread,                      _current_pending_monitor,                      ObjectMonitor*)                        \
   nonstatic_field(Thread,                      _current_pending_monitor_is_from_java,         bool)                                  \