[vm/concurrency] Refactor SafepointRwLock to ensure unlocking never causes safepoint transition.

The previous implementation of SafepointRwLock has an issue (discovered
by Slava, see bug) with the following code pattern:

    {
      SafepointReadRwLocker locker(...);
      return Foo(); // <-- Returns raw ptr.
    }

After the return expression has been evaluated to a raw pointer
the destructor might enter safepoint and cause GC, which leaves
the raw pointer unchanged (since it's not in a handle).

This CL refactors the implementation to ensure we don't perform any
state transition in the destructor, therefore eliminate the
possibility of GC moving the object we return.

Issue https://github.com/dart-lang/sdk/issues/44998

TEST=Relying on existing SafepointRwLock.

Change-Id: Ib7a62b36838edd4b39ad67a8c58f048aa05aa144
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/185062
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Alexander Aprelev <aam@google.com>
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>

runtime/vm/lockers.cc

@@ -86,4 +86,120 @@ Monitor::WaitResult SafepointMonitorLocker::Wait(int64_t millis) {
   }
 }
 
+#if defined(DEBUG)
+bool SafepointRwLock::IsCurrentThreadReader() {
+  ThreadId id = OSThread::GetCurrentThreadId();
+  if (IsCurrentThreadWriter()) {
+    return true;
+  }
+  MonitorLocker ml(&monitor_);
+  for (intptr_t i = readers_ids_.length() - 1; i >= 0; i--) {
+    if (readers_ids_.At(i) == id) {
+      return true;
+    }
+  }
+  return false;
+}
+#endif  // defined(DEBUG)
+
+bool SafepointRwLock::EnterRead() {
+  // No need to safepoint if the current thread is not attached.
+  auto thread = Thread::Current();
+  const bool can_block_without_safepoint = thread == nullptr;
+
+  bool acquired_read_lock = false;
+  if (!TryEnterRead(can_block_without_safepoint, &acquired_read_lock)) {
+    // Important: must never hold monitor_ when blocking for safepoint.
+    TransitionVMToBlocked transition(thread);
+    const bool ok = TryEnterRead(/*can_block=*/true, &acquired_read_lock);
+    RELEASE_ASSERT(ok);
+    RELEASE_ASSERT(acquired_read_lock);
+  }
+  return acquired_read_lock;
+}
+
+bool SafepointRwLock::TryEnterRead(bool can_block, bool* acquired_read_lock) {
+  MonitorLocker ml(&monitor_);
+  if (IsCurrentThreadWriter()) {
+    *acquired_read_lock = false;
+    return true;
+  }
+  if (can_block) {
+    while (state_ < 0) {
+      ml.Wait();
+    }
+  }
+  if (state_ >= 0) {
+    ++state_;
+    DEBUG_ONLY(readers_ids_.Add(OSThread::GetCurrentThreadId()));
+    *acquired_read_lock = true;
+    return true;
+  }
+  return false;
+}
+
+void SafepointRwLock::LeaveRead() {
+  MonitorLocker ml(&monitor_);
+  ASSERT(state_ > 0);
+#if defined(DEBUG)
+  {
+    intptr_t i = readers_ids_.length() - 1;
+    ThreadId id = OSThread::GetCurrentThreadId();
+    while (i >= 0) {
+      if (readers_ids_.At(i) == id) {
+        readers_ids_.RemoveAt(i);
+        break;
+      }
+      i--;
+    }
+    ASSERT(i >= 0);
+  }
+#endif
+  if (--state_ == 0) {
+    ml.NotifyAll();
+  }
+}
+
+void SafepointRwLock::EnterWrite() {
+  // No need to safepoint if the current thread is not attached.
+  auto thread = Thread::Current();
+  const bool can_block_without_safepoint = thread == nullptr;
+
+  if (!TryEnterWrite(can_block_without_safepoint)) {
+    // Important: must never hold monitor_ when blocking for safepoint.
+    TransitionVMToBlocked transition(thread);
+    const bool ok = TryEnterWrite(/*can_block=*/true);
+    RELEASE_ASSERT(ok);
+  }
+}
+
+bool SafepointRwLock::TryEnterWrite(bool can_block) {
+  MonitorLocker ml(&monitor_);
+  if (IsCurrentThreadWriter()) {
+    state_--;
+    return true;
+  }
+  if (can_block) {
+    while (state_ != 0) {
+      ml.Wait();
+    }
+  }
+  if (state_ == 0) {
+    writer_id_ = OSThread::GetCurrentThreadId();
+    state_ = -1;
+    return true;
+  }
+  return false;
+}
+
+void SafepointRwLock::LeaveWrite() {
+  MonitorLocker ml(&monitor_);
+  ASSERT(state_ < 0);
+  if (++state_ < 0) {
+    return;
+  }
+  writer_id_ = OSThread::kInvalidThreadId;
+  ml.NotifyAll();
+}
+
 }  // namespace dart

runtime/vm/lockers.h

@@ -322,21 +322,7 @@ class SafepointRwLock {
   SafepointRwLock() {}
   ~SafepointRwLock() {}
 
-#if defined(DEBUG)
-  bool IsCurrentThreadReader() {
-    ThreadId id = OSThread::GetCurrentThreadId();
-    if (IsCurrentThreadWriter()) {
-      return true;
-    }
-    MutexLocker ml(&reader_ids_mutex_);
-    for (intptr_t i = readers_ids_.length() - 1; i >= 0; i--) {
-      if (readers_ids_.At(i) == id) {
-        return true;
-      }
-    }
-    return false;
-  }
-#endif  // defined(DEBUG)
+  DEBUG_ONLY(bool IsCurrentThreadReader());
 
   bool IsCurrentThreadWriter() {
     return writer_id_ == OSThread::GetCurrentThreadId();
@@ -346,82 +332,33 @@ class SafepointRwLock {
   friend class SafepointReadRwLocker;
   friend class SafepointWriteRwLocker;
 
-  // returns [true] if read lock was acuired,
+  // returns [true] if read lock was acquired,
   // returns [false] if the thread didn't have to acquire read lock due
   // to the thread already holding write lock
-  bool EnterRead() {
-    SafepointMonitorLocker ml(&monitor_);
-    if (IsCurrentThreadWriter()) {
-      return false;
-    }
-    while (state_ < 0) {
-      ml.Wait();
-    }
-#if defined(DEBUG)
-    {
-      MutexLocker ml(&reader_ids_mutex_);
-      readers_ids_.Add(OSThread::GetCurrentThreadId());
-    }
-#endif
-    ++state_;
-    return true;
-  }
-  void LeaveRead() {
-    SafepointMonitorLocker ml(&monitor_);
-    ASSERT(state_ > 0);
-#if defined(DEBUG)
-    {
-      MutexLocker ml(&reader_ids_mutex_);
-      intptr_t i = readers_ids_.length() - 1;
-      ThreadId id = OSThread::GetCurrentThreadId();
-      while (i >= 0) {
-        if (readers_ids_.At(i) == id) {
-          readers_ids_.RemoveAt(i);
-          break;
-        }
-        i--;
-      }
-      ASSERT(i >= 0);
-    }
-#endif
-    if (--state_ == 0) {
-      ml.NotifyAll();
-    }
-  }
+  bool EnterRead();
+  bool TryEnterRead(bool can_block, bool* acquired_read_lock);
+  void LeaveRead();
 
-  void EnterWrite() {
-    SafepointMonitorLocker ml(&monitor_);
-    if (IsCurrentThreadWriter()) {
-      state_--;
-      return;
-    }
-    while (state_ != 0) {
-      ml.Wait();
-    }
-    writer_id_ = OSThread::GetCurrentThreadId();
-    state_ = -1;
-  }
-  void LeaveWrite() {
-    SafepointMonitorLocker ml(&monitor_);
-    ASSERT(state_ < 0);
-    state_++;
-    if (state_ < 0) {
-      return;
-    }
-    writer_id_ = OSThread::kInvalidThreadId;
-    ml.NotifyAll();
-  }
+  void EnterWrite();
+  bool TryEnterWrite(bool can_block);
+  void LeaveWrite();
 
+  // We maintain an invariant that this monitor is never locked for long periods
+  // of time: Any thread that acquired this monitor must always be able to do
+  // it's work and release it (or wait on the monitor which will also release
+  // it).
+  //
+  // In particular we must ensure the monitor is never held and then a potential
+  // safepoint operation is triggered, since another thread could try to acquire
+  // the lock and it would deadlock.
   Monitor monitor_;
+
   // [state_] > 0  : The lock is held by multiple readers.
   // [state_] == 0 : The lock is free (no readers/writers).
   // [state_] < 0  : The lock is held by a single writer (possibly nested).
   intptr_t state_ = 0;
 
-#if defined(DEBUG)
-  Mutex reader_ids_mutex_;
-  MallocGrowableArray<ThreadId> readers_ids_;
-#endif
+  DEBUG_ONLY(MallocGrowableArray<ThreadId> readers_ids_);
   ThreadId writer_id_ = OSThread::kInvalidThreadId;
 };
 

runtime/vm/os_thread.h

@@ -377,6 +377,7 @@ class Monitor {
 
   friend class MonitorLocker;
   friend class SafepointMonitorLocker;
+  friend class SafepointRwLock;
   friend void Dart_TestMonitor();
   DISALLOW_COPY_AND_ASSIGN(Monitor);
 };