8256383: PlatformMutex::try_lock has different semantics on Windows a…

…nd Posix

Reviewed-by: stuefe, dcubed

src/hotspot/os/posix/os_posix.hpp

@@ -211,7 +211,10 @@ class PlatformParker : public CHeapObj<mtSynchronizer> {
 #define PLATFORM_MONITOR_IMPL_INDIRECT 0
 #endif
 
-// Platform specific implementations that underpin VM Mutex/Monitor classes
+// Platform specific implementations that underpin VM Mutex/Monitor classes.
+// Note that we use "normal" pthread_mutex_t attributes so that recursive
+// locking is not supported, which matches the expected semantics of the
+// VM Mutex class.
 
 class PlatformMutex : public CHeapObj<mtSynchronizer> {
 #if PLATFORM_MONITOR_IMPL_INDIRECT

src/hotspot/os/windows/os_windows.hpp

@@ -187,7 +187,10 @@ class PlatformParker : public CHeapObj<mtSynchronizer> {
 
 } ;
 
-// Platform specific implementations that underpin VM Mutex/Monitor classes
+// Platform specific implementations that underpin VM Mutex/Monitor classes.
+// Note that CRITICAL_SECTION supports recursive locking, while the semantics
+// of the VM Mutex class does not. It is up to the Mutex class to hide this
+// difference in behaviour.
 
 class PlatformMutex : public CHeapObj<mtSynchronizer> {
   NONCOPYABLE(PlatformMutex);

src/hotspot/share/runtime/mutex.cpp

@@ -137,13 +137,14 @@ void Mutex::lock_without_safepoint_check() {
 
 
 // Returns true if thread succeeds in grabbing the lock, otherwise false.
-
 bool Mutex::try_lock() {
   Thread * const self = Thread::current();
   // Some safepoint_check_always locks use try_lock, so cannot check
   // safepoint state, but can check blocking state.
   check_block_state(self);
-  if (_lock.try_lock()) {
+  // Checking the owner hides the potential difference in recursive locking behaviour
+  // on some platforms.
+  if (_owner != self && _lock.try_lock()) {
     assert_owner(NULL);
     set_owner(self);
     return true;

src/hotspot/share/runtime/mutex.hpp

@@ -32,6 +32,12 @@
 // variable that supports lock ownership tracking, lock ranking for deadlock
 // detection and coordinates with the safepoint protocol.
 
+// Locking is non-recursive: if you try to lock a mutex you already own then you
+// will get an assertion failure in a debug build (which should suffice to expose
+// usage bugs). If you call try_lock on a mutex you already own it will return false.
+// The underlying PlatformMutex may support recursive locking but this is not exposed
+// and we account for that possibility in try_lock.
+
 // The default length of mutex name was originally chosen to be 64 to avoid
 // false sharing. Now, PaddedMutex and PaddedMonitor are available for this purpose.
 // TODO: Check if _name[MUTEX_NAME_LEN] should better get replaced by const char*.