changeset 55977:18f4d3d46d54

8229212: clear up CHECK_OWNER confusion in objectMonitor.cpp Reviewed-by: dholmes, coleenp
author dcubed
date Tue, 13 Aug 2019 16:13:22 -0400
parents be8c11fc16bb
children 7ba5e49258de
files src/hotspot/share/runtime/objectMonitor.cpp src/hotspot/share/runtime/objectMonitor.hpp src/hotspot/share/runtime/objectMonitor.inline.hpp src/hotspot/share/runtime/synchronizer.cpp test/hotspot/jtreg/runtime/Monitor/NonOwnerOps.java
diffstat 5 files changed, 119 insertions(+), 45 deletions(-) [+]
line wrap: on
line diff
--- a/src/hotspot/share/runtime/objectMonitor.cpp	Tue Aug 13 10:10:42 2019 -0700
+++ b/src/hotspot/share/runtime/objectMonitor.cpp	Tue Aug 13 16:13:22 2019 -0400
@@ -1143,31 +1143,34 @@
   return;
 }
 
-
-// -----------------------------------------------------------------------------
-// A macro is used below because there may already be a pending
-// exception which should not abort the execution of the routines
-// which use this (which is why we don't put this into check_slow and
-// call it with a CHECK argument).
-
-#define CHECK_OWNER()                                                       \
-  do {                                                                      \
-    if (THREAD != _owner) {                                                 \
-      if (THREAD->is_lock_owned((address) _owner)) {                        \
-        _owner = THREAD;  /* Convert from basiclock addr to Thread addr */  \
-        _recursions = 0;                                                    \
-      } else {                                                              \
-        THROW(vmSymbols::java_lang_IllegalMonitorStateException());         \
-      }                                                                     \
-    }                                                                       \
+// Checks that the current THREAD owns this monitor and causes an
+// immediate return if it doesn't. We don't use the CHECK macro
+// because we want the IMSE to be the only exception that is thrown
+// from the call site when false is returned. Any other pending
+// exception is ignored.
+#define CHECK_OWNER()                                                  \
+  do {                                                                 \
+    if (!check_owner(THREAD)) {                                        \
+       assert(HAS_PENDING_EXCEPTION, "expected a pending IMSE here."); \
+       return;                                                         \
+     }                                                                 \
   } while (false)
 
-// check_slow() is a misnomer.  It's called to simply to throw an IMSX exception.
-// TODO-FIXME: remove check_slow() -- it's likely dead.
-
-void ObjectMonitor::check_slow(TRAPS) {
-  assert(THREAD != _owner && !THREAD->is_lock_owned((address) _owner), "must not be owner");
-  THROW_MSG(vmSymbols::java_lang_IllegalMonitorStateException(), "current thread not owner");
+// Returns true if the specified thread owns the ObjectMonitor.
+// Otherwise returns false and throws IllegalMonitorStateException
+// (IMSE). If there is a pending exception and the specified thread
+// is not the owner, that exception will be replaced by the IMSE.
+bool ObjectMonitor::check_owner(Thread* THREAD) {
+  if (_owner == THREAD) {
+    return true;
+  }
+  if (THREAD->is_lock_owned((address)_owner)) {
+    _owner = THREAD;  // convert from BasicLock addr to Thread addr
+    _recursions = 0;
+    return true;
+  }
+  THROW_MSG_(vmSymbols::java_lang_IllegalMonitorStateException(),
+             "current thread is not owner", false);
 }
 
 static void post_monitor_wait_event(EventJavaMonitorWait* event,
@@ -1197,8 +1200,7 @@
 
   assert(InitDone, "Unexpectedly not initialized");
 
-  // Throw IMSX or IEX.
-  CHECK_OWNER();
+  CHECK_OWNER();  // Throws IMSE if not owner.
 
   EventJavaMonitorWait event;
 
@@ -1477,7 +1479,7 @@
 // that suggests a lost wakeup bug.
 
 void ObjectMonitor::notify(TRAPS) {
-  CHECK_OWNER();
+  CHECK_OWNER();  // Throws IMSE if not owner.
   if (_WaitSet == NULL) {
     return;
   }
@@ -1495,7 +1497,7 @@
 // mode the waitset will be empty and the EntryList will be "DCBAXYZ".
 
 void ObjectMonitor::notifyAll(TRAPS) {
-  CHECK_OWNER();
+  CHECK_OWNER();  // Throws IMSE if not owner.
   if (_WaitSet == NULL) {
     return;
   }
--- a/src/hotspot/share/runtime/objectMonitor.hpp	Tue Aug 13 10:10:42 2019 -0700
+++ b/src/hotspot/share/runtime/objectMonitor.hpp	Tue Aug 13 16:13:22 2019 -0400
@@ -285,8 +285,9 @@
   void*     object_addr();
   void      set_object(void* obj);
 
-  bool      check(TRAPS);       // true if the thread owns the monitor.
-  void      check_slow(TRAPS);
+  // Returns true if the specified thread owns the ObjectMonitor. Otherwise
+  // returns false and throws IllegalMonitorStateException (IMSE).
+  bool      check_owner(Thread* THREAD);
   void      clear();
 
   void      enter(TRAPS);
--- a/src/hotspot/share/runtime/objectMonitor.inline.hpp	Tue Aug 13 10:10:42 2019 -0700
+++ b/src/hotspot/share/runtime/objectMonitor.inline.hpp	Tue Aug 13 16:13:22 2019 -0400
@@ -77,19 +77,6 @@
   _object = obj;
 }
 
-inline bool ObjectMonitor::check(TRAPS) {
-  if (THREAD != _owner) {
-    if (THREAD->is_lock_owned((address) _owner)) {
-      _owner = THREAD;  // regain ownership of inflated monitor
-      assert (_recursions == 0, "invariant") ;
-    } else {
-      check_slow(THREAD);
-      return false;
-    }
-  }
-  return true;
-}
-
 // return number of threads contending for this monitor
 inline jint ObjectMonitor::contentions() const {
   return _contentions;
--- a/src/hotspot/share/runtime/synchronizer.cpp	Tue Aug 13 10:10:42 2019 -0700
+++ b/src/hotspot/share/runtime/synchronizer.cpp	Tue Aug 13 16:13:22 2019 -0400
@@ -429,9 +429,10 @@
   assert(!obj->mark()->has_bias_pattern(), "biases should be revoked by now");
 
   ObjectMonitor* monitor = inflate(THREAD, obj, inflate_cause_jni_exit);
-  // If this thread has locked the object, exit the monitor.  Note:  can't use
-  // monitor->check(CHECK); must exit even if an exception is pending.
-  if (monitor->check(THREAD)) {
+  // If this thread has locked the object, exit the monitor. We
+  // intentionally do not use CHECK here because we must exit the
+  // monitor even if an exception is pending.
+  if (monitor->check_owner(THREAD)) {
     monitor->exit(true, THREAD);
   }
 }
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/hotspot/jtreg/runtime/Monitor/NonOwnerOps.java	Tue Aug 13 16:13:22 2019 -0400
@@ -0,0 +1,83 @@
+/*
+ * Copyright (c) 2019, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * @test
+ * @bug 8229212
+ * @summary Verify that monitor operations by a non-owner thread throw
+ *          IllegalMonitorStateException.
+ * @run main NonOwnerOps
+ */
+
+public class NonOwnerOps {
+    public static void main(String[] args) {
+        int error_count = 0;
+        Object obj;
+
+        obj = new Object();
+        try {
+            obj.wait();
+            System.err.println("ERROR: wait() by non-owner thread did not " +
+                               "throw IllegalMonitorStateException.");
+            error_count++;
+        } catch (InterruptedException ie) {
+            System.err.println("ERROR: wait() by non-owner thread threw " +
+                               "InterruptedException which is not expected.");
+            error_count++;
+        } catch (IllegalMonitorStateException imse) {
+            System.out.println("wait() by non-owner thread threw the " +
+                               "expected IllegalMonitorStateException:");
+            System.out.println("    " + imse);
+        }
+
+        obj = new Object();
+        try {
+            obj.notify();
+            System.err.println("ERROR: notify() by non-owner thread did not " +
+                               "throw IllegalMonitorStateException.");
+            error_count++;
+        } catch (IllegalMonitorStateException imse) {
+            System.out.println("notify() by non-owner thread threw the " +
+                               "expected IllegalMonitorStateException:");
+            System.out.println("    " + imse);
+        }
+
+        obj = new Object();
+        try {
+            obj.notifyAll();
+            System.err.println("ERROR: notifyAll() by non-owner thread did " +
+                               "not throw IllegalMonitorStateException.");
+            error_count++;
+        } catch (IllegalMonitorStateException imse) {
+            System.out.println("notifyAll() by non-owner thread threw the " +
+                               "expected IllegalMonitorStateException:");
+            System.out.println("    " + imse);
+        }
+
+        if (error_count != 0) {
+            throw new RuntimeException("Test failed with " + error_count +
+                                       " errors.");
+        }
+        System.out.println("Test PASSED.");
+    }
+}