8229212: clear up CHECK_OWNER confusion in objectMonitor.cpp
Reviewed-by: dholmes, coleenp
--- 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.");
+ }
+}