[vm] Don't block OOB messages or reload during field initialization; make interrupts lock-free.

- Account for initialization-in-progress sentinel when checking static field types for reload.
 - Don't read the true stack limit when setting or clearing interrupts.

TEST=ci
Bug: https://github.com/dart-lang/sdk/issues/46596
Change-Id: I80adb4d7d69f01125b7eae8215b5da4d2e467bda
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206662
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>

runtime/platform/atomic.h

@@ -21,9 +21,16 @@ class RelaxedAtomic {
   T load(std::memory_order order = std::memory_order_relaxed) const {
     return value_.load(order);
   }
+  T load(std::memory_order order = std::memory_order_relaxed) const volatile {
+    return value_.load(order);
+  }
   void store(T arg, std::memory_order order = std::memory_order_relaxed) {
     value_.store(arg, order);
   }
+  void store(T arg,
+             std::memory_order order = std::memory_order_relaxed) volatile {
+    value_.store(arg, order);
+  }
 
   T fetch_add(T arg, std::memory_order order = std::memory_order_relaxed) {
     return value_.fetch_add(arg, order);
@@ -44,6 +51,12 @@ class RelaxedAtomic {
       std::memory_order order = std::memory_order_relaxed) {
     return value_.compare_exchange_weak(expected, desired, order, order);
   }
+  bool compare_exchange_weak(
+      T& expected,  // NOLINT
+      T desired,
+      std::memory_order order = std::memory_order_relaxed) volatile {
+    return value_.compare_exchange_weak(expected, desired, order, order);
+  }
   bool compare_exchange_strong(
       T& expected,  // NOLINT
       T desired,

runtime/vm/heap/safepoint.cc

@@ -155,7 +155,7 @@ void SafepointHandler::LevelHandler::NotifyThreadsToGetToSafepointLevel(
       if (!Thread::IsAtSafepoint(level_, state)) {
         // Send OOB message to get it to safepoint.
         if (current->IsMutatorThread()) {
-          current->ScheduleInterruptsLocked(Thread::kVMInterrupt);
+          current->ScheduleInterrupts(Thread::kVMInterrupt);
         }
         MonitorLocker sl(&parked_lock_);
         num_threads_not_parked_++;

runtime/vm/isolate.cc

@@ -867,19 +867,6 @@ bool IsolateVisitor::IsSystemIsolate(Isolate* isolate) const {
   return Isolate::IsSystemIsolate(isolate);
 }
 
-NoOOBMessageScope::NoOOBMessageScope(Thread* thread)
-    : ThreadStackResource(thread) {
-  if (thread->isolate() != nullptr) {
-    thread->DeferOOBMessageInterrupts();
-  }
-}
-
-NoOOBMessageScope::~NoOOBMessageScope() {
-  if (thread()->isolate() != nullptr) {
-    thread()->RestoreOOBMessageInterrupts();
-  }
-}
-
 Bequest::~Bequest() {
   if (handle_ == nullptr) {
     return;

runtime/vm/isolate.h

@@ -134,16 +134,6 @@ class LambdaCallable : public Callable {
   DISALLOW_COPY_AND_ASSIGN(LambdaCallable);
 };
 
-// Disallow OOB message handling within this scope.
-class NoOOBMessageScope : public ThreadStackResource {
- public:
-  explicit NoOOBMessageScope(Thread* thread);
-  ~NoOOBMessageScope();
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(NoOOBMessageScope);
-};
-
 // Fixed cache for exception handler lookup.
 typedef FixedCache<intptr_t, ExceptionHandlerInfo, 16> HandlerInfoCache;
 // Fixed cache for catch entry state lookup.

runtime/vm/isolate_reload.cc

@@ -2147,7 +2147,8 @@ class FieldInvalidator {
         // At that point it doesn't have the field table setup yet.
         if (field_table->IsReadyToUse()) {
           value_ = field_table->At(field_id);
-          if (value_.ptr() != Object::sentinel().ptr()) {
+          if ((value_.ptr() != Object::sentinel().ptr()) &&
+              (value_.ptr() != Object::transition_sentinel().ptr())) {
             CheckValueType(null_safety, value_, field);
           }
         }

runtime/vm/isolate_test.cc

@@ -145,106 +145,4 @@ TEST_CASE(StackLimitInterrupts) {
   barrier.Exit();
 }
 
-class IsolateTestHelper {
- public:
-  static uword GetStackLimit(Thread* thread) { return thread->stack_limit_; }
-  static uword GetSavedStackLimit(Thread* thread) {
-    return thread->saved_stack_limit_;
-  }
-  static uword GetDeferredInterruptsMask(Thread* thread) {
-    return thread->deferred_interrupts_mask_;
-  }
-  static uword GetDeferredInterrupts(Thread* thread) {
-    return thread->deferred_interrupts_;
-  }
-};
-
-TEST_CASE(NoOOBMessageScope) {
-  // Finish any GC in progress so that no kVMInterrupt is added for GC reasons.
-  {
-    TransitionNativeToVM transition(thread);
-    GCTestHelper::CollectAllGarbage();
-    const Error& error = Error::Handle(thread->HandleInterrupts());
-    RELEASE_ASSERT(error.IsNull());
-  }
-
-  // EXPECT_EQ is picky about type agreement for its arguments.
-  const uword kZero = 0;
-  const uword kMessageInterrupt = Thread::kMessageInterrupt;
-  const uword kVMInterrupt = Thread::kVMInterrupt;
-  uword stack_limit;
-  uword interrupt_bits;
-
-  // Initially no interrupts are scheduled or deferred.
-  EXPECT_EQ(IsolateTestHelper::GetStackLimit(thread),
-            IsolateTestHelper::GetSavedStackLimit(thread));
-  EXPECT_EQ(kZero, IsolateTestHelper::GetDeferredInterruptsMask(thread));
-  EXPECT_EQ(kZero, IsolateTestHelper::GetDeferredInterrupts(thread));
-
-  {
-    // Defer message interrupts.
-    NoOOBMessageScope no_msg_scope(thread);
-    EXPECT_EQ(IsolateTestHelper::GetStackLimit(thread),
-              IsolateTestHelper::GetSavedStackLimit(thread));
-    EXPECT_EQ(kMessageInterrupt,
-              IsolateTestHelper::GetDeferredInterruptsMask(thread));
-    EXPECT_EQ(kZero, IsolateTestHelper::GetDeferredInterrupts(thread));
-
-    // Schedule a message, it is deferred.
-    thread->ScheduleInterrupts(Thread::kMessageInterrupt);
-    EXPECT_EQ(IsolateTestHelper::GetStackLimit(thread),
-              IsolateTestHelper::GetSavedStackLimit(thread));
-    EXPECT_EQ(kMessageInterrupt,
-              IsolateTestHelper::GetDeferredInterruptsMask(thread));
-    EXPECT_EQ(kMessageInterrupt,
-              IsolateTestHelper::GetDeferredInterrupts(thread));
-
-    // Schedule a vm interrupt, it is not deferred.
-    thread->ScheduleInterrupts(Thread::kVMInterrupt);
-    stack_limit = IsolateTestHelper::GetStackLimit(thread);
-    EXPECT_NE(stack_limit, IsolateTestHelper::GetSavedStackLimit(thread));
-    EXPECT((stack_limit & Thread::kVMInterrupt) != 0);
-    EXPECT_EQ(kMessageInterrupt,
-              IsolateTestHelper::GetDeferredInterruptsMask(thread));
-    EXPECT_EQ(kMessageInterrupt,
-              IsolateTestHelper::GetDeferredInterrupts(thread));
-
-    // Clear the vm interrupt.  Message is still deferred.
-    interrupt_bits = thread->GetAndClearInterrupts();
-    EXPECT_EQ(kVMInterrupt, interrupt_bits);
-    EXPECT_EQ(IsolateTestHelper::GetStackLimit(thread),
-              IsolateTestHelper::GetSavedStackLimit(thread));
-    EXPECT_EQ(kMessageInterrupt,
-              IsolateTestHelper::GetDeferredInterruptsMask(thread));
-    EXPECT_EQ(kMessageInterrupt,
-              IsolateTestHelper::GetDeferredInterrupts(thread));
-  }
-
-  // Restore message interrupts.  Message is now pending.
-  stack_limit = IsolateTestHelper::GetStackLimit(thread);
-  EXPECT_NE(stack_limit, IsolateTestHelper::GetSavedStackLimit(thread));
-  EXPECT((stack_limit & Thread::kMessageInterrupt) != 0);
-  EXPECT_EQ(kZero, IsolateTestHelper::GetDeferredInterruptsMask(thread));
-  EXPECT_EQ(kZero, IsolateTestHelper::GetDeferredInterrupts(thread));
-
-  {
-    // Defer message interrupts, again.  The pending interrupt is deferred.
-    NoOOBMessageScope no_msg_scope(thread);
-    EXPECT_EQ(IsolateTestHelper::GetStackLimit(thread),
-              IsolateTestHelper::GetSavedStackLimit(thread));
-    EXPECT_EQ(kMessageInterrupt,
-              IsolateTestHelper::GetDeferredInterruptsMask(thread));
-    EXPECT_EQ(kMessageInterrupt,
-              IsolateTestHelper::GetDeferredInterrupts(thread));
-  }
-
-  // Restore, then clear interrupts.  The world is as it was.
-  interrupt_bits = thread->GetAndClearInterrupts();
-  EXPECT_EQ(kMessageInterrupt, interrupt_bits);
-  EXPECT_EQ(IsolateTestHelper::GetStackLimit(thread),
-            IsolateTestHelper::GetSavedStackLimit(thread));
-  EXPECT_EQ(kZero, IsolateTestHelper::GetDeferredInterruptsMask(thread));
-  EXPECT_EQ(kZero, IsolateTestHelper::GetDeferredInterrupts(thread));
-}
-
 }  // namespace dart

runtime/vm/object.cc

@@ -11156,8 +11156,7 @@ void Field::SetStaticConstFieldValue(const Instance& value,
 }
 
 ObjectPtr Field::EvaluateInitializer() const {
-  Thread* const thread = Thread::Current();
-  ASSERT(thread->IsMutatorThread());
+  ASSERT(Thread::Current()->IsMutatorThread());
 
 #if !defined(DART_PRECOMPILED_RUNTIME)
   if (is_static() && is_const()) {
@@ -11165,8 +11164,6 @@ ObjectPtr Field::EvaluateInitializer() const {
   }
 #endif  // !defined(DART_PRECOMPILED_RUNTIME)
 
-  NoOOBMessageScope no_msg_scope(thread);
-  NoReloadScope no_reload_scope(thread);
   const Function& initializer = Function::Handle(EnsureInitializerFunction());
   return DartEntry::InvokeFunction(initializer, Object::empty_array());
 }

runtime/vm/thread.cc

@@ -91,9 +91,6 @@ Thread::Thread(bool is_vm_isolate)
       no_safepoint_scope_depth_(0),
 #endif
       reusable_handles_(),
-      defer_oob_messages_count_(0),
-      deferred_interrupts_mask_(0),
-      deferred_interrupts_(0),
       stack_overflow_count_(0),
       hierarchy_info_(NULL),
       type_usage_info_(NULL),
@@ -398,7 +395,7 @@ void Thread::SetStackLimit(uword limit) {
   MonitorLocker ml(&thread_lock_);
   if (!HasScheduledInterrupts()) {
     // No interrupt pending, set stack_limit_ too.
-    stack_limit_ = limit;
+    stack_limit_.store(limit);
   }
   saved_stack_limit_ = limit;
 }
@@ -407,96 +404,40 @@ void Thread::ClearStackLimit() {
   SetStackLimit(~static_cast<uword>(0));
 }
 
-void Thread::ScheduleInterrupts(uword interrupt_bits) {
-  MonitorLocker ml(&thread_lock_);
-  ScheduleInterruptsLocked(interrupt_bits);
+static bool IsInterruptLimit(uword limit) {
+  return (limit & ~Thread::kInterruptsMask) ==
+         (kInterruptStackLimit & ~Thread::kInterruptsMask);
 }
 
-void Thread::ScheduleInterruptsLocked(uword interrupt_bits) {
-  ASSERT(thread_lock_.IsOwnedByCurrentThread());
+void Thread::ScheduleInterrupts(uword interrupt_bits) {
   ASSERT((interrupt_bits & ~kInterruptsMask) == 0);  // Must fit in mask.
 
-  // Check to see if any of the requested interrupts should be deferred.
-  uword defer_bits = interrupt_bits & deferred_interrupts_mask_;
-  if (defer_bits != 0) {
-    deferred_interrupts_ |= defer_bits;
-    interrupt_bits &= ~deferred_interrupts_mask_;
-    if (interrupt_bits == 0) {
-      return;
+  uword old_limit = stack_limit_.load();
+  uword new_limit;
+  do {
+    if (IsInterruptLimit(old_limit)) {
+      new_limit = old_limit | interrupt_bits;
+    } else {
+      new_limit = (kInterruptStackLimit & ~kInterruptsMask) | interrupt_bits;
     }
-  }
-
-  if (stack_limit_ == saved_stack_limit_) {
-    stack_limit_ = (kInterruptStackLimit & ~kInterruptsMask) | interrupt_bits;
-  } else {
-    stack_limit_ = stack_limit_ | interrupt_bits;
-  }
+  } while (!stack_limit_.compare_exchange_weak(old_limit, new_limit));
 }
 
 uword Thread::GetAndClearInterrupts() {
-  MonitorLocker ml(&thread_lock_);
-  if (stack_limit_ == saved_stack_limit_) {
-    return 0;  // No interrupt was requested.
-  }
-  uword interrupt_bits = stack_limit_ & kInterruptsMask;
-  stack_limit_ = saved_stack_limit_;
+  uword interrupt_bits = 0;
+  uword old_limit = stack_limit_.load();
+  uword new_limit = saved_stack_limit_;
+  do {
+    if (IsInterruptLimit(old_limit)) {
+      interrupt_bits = interrupt_bits | (old_limit & kInterruptsMask);
+    } else {
+      return interrupt_bits;
+    }
+  } while (!stack_limit_.compare_exchange_weak(old_limit, new_limit));
+
   return interrupt_bits;
 }
 
-void Thread::DeferOOBMessageInterrupts() {
-  MonitorLocker ml(&thread_lock_);
-  defer_oob_messages_count_++;
-  if (defer_oob_messages_count_ > 1) {
-    // OOB message interrupts are already deferred.
-    return;
-  }
-  ASSERT(deferred_interrupts_mask_ == 0);
-  deferred_interrupts_mask_ = kMessageInterrupt;
-
-  if (stack_limit_ != saved_stack_limit_) {
-    // Defer any interrupts which are currently pending.
-    deferred_interrupts_ = stack_limit_ & deferred_interrupts_mask_;
-
-    // Clear deferrable interrupts, if present.
-    stack_limit_ = stack_limit_ & ~deferred_interrupts_mask_;
-
-    if ((stack_limit_ & kInterruptsMask) == 0) {
-      // No other pending interrupts.  Restore normal stack limit.
-      stack_limit_ = saved_stack_limit_;
-    }
-  }
-#if !defined(PRODUCT)
-  if (FLAG_trace_service && FLAG_trace_service_verbose) {
-    OS::PrintErr("[+%" Pd64 "ms] Isolate %s deferring OOB interrupts\n",
-                 Dart::UptimeMillis(), isolate()->name());
-  }
-#endif  // !defined(PRODUCT)
-}
-
-void Thread::RestoreOOBMessageInterrupts() {
-  MonitorLocker ml(&thread_lock_);
-  defer_oob_messages_count_--;
-  if (defer_oob_messages_count_ > 0) {
-    return;
-  }
-  ASSERT(defer_oob_messages_count_ == 0);
-  ASSERT(deferred_interrupts_mask_ == kMessageInterrupt);
-  deferred_interrupts_mask_ = 0;
-  if (deferred_interrupts_ != 0) {
-    if (stack_limit_ == saved_stack_limit_) {
-      stack_limit_ = kInterruptStackLimit & ~kInterruptsMask;
-    }
-    stack_limit_ = stack_limit_ | deferred_interrupts_;
-    deferred_interrupts_ = 0;
-  }
-#if !defined(PRODUCT)
-  if (FLAG_trace_service && FLAG_trace_service_verbose) {
-    OS::PrintErr("[+%" Pd64 "ms] Isolate %s restoring OOB interrupts\n",
-                 Dart::UptimeMillis(), isolate()->name());
-  }
-#endif  // !defined(PRODUCT)
-}
-
 ErrorPtr Thread::HandleInterrupts() {
   uword interrupt_bits = GetAndClearInterrupts();
   if ((interrupt_bits & kVMInterrupt) != 0) {

runtime/vm/thread.h

@@ -417,11 +417,10 @@ class Thread : public ThreadState {
   };
 
   void ScheduleInterrupts(uword interrupt_bits);
-  void ScheduleInterruptsLocked(uword interrupt_bits);
   ErrorPtr HandleInterrupts();
   uword GetAndClearInterrupts();
   bool HasScheduledInterrupts() const {
-    return (stack_limit_ & kInterruptsMask) != 0;
+    return (stack_limit_.load() & kInterruptsMask) != 0;
   }
 
   // Monitor corresponding to this thread.
@@ -1031,7 +1030,7 @@ class Thread : public ThreadState {
   // in SIMARM(IA32) and ARM, and the same offsets in SIMARM64(X64) and ARM64.
   // We use only word-sized fields to avoid differences in struct packing on the
   // different architectures. See also CheckOffsets in dart.cc.
-  RelaxedAtomic<uword> stack_limit_;
+  volatile RelaxedAtomic<uword> stack_limit_;
   uword write_barrier_mask_;
   uword heap_base_;
   Isolate* isolate_;
@@ -1107,9 +1106,6 @@ class Thread : public ThreadState {
   int32_t no_safepoint_scope_depth_;
 #endif
   VMHandles reusable_handles_;
-  intptr_t defer_oob_messages_count_;
-  uint16_t deferred_interrupts_mask_;
-  uint16_t deferred_interrupts_;
   int32_t stack_overflow_count_;
   uint32_t runtime_call_count_ = 0;
 
@@ -1205,9 +1201,6 @@ class Thread : public ThreadState {
 
   static void SetCurrent(Thread* current) { OSThread::SetCurrentTLS(current); }
 
-  void DeferOOBMessageInterrupts();
-  void RestoreOOBMessageInterrupts();
-
 #define REUSABLE_FRIEND_DECLARATION(name)                                      \
   friend class Reusable##name##HandleScope;
   REUSABLE_HANDLE_LIST(REUSABLE_FRIEND_DECLARATION)
@@ -1218,9 +1211,7 @@ class Thread : public ThreadState {
   friend class InterruptChecker;
   friend class Isolate;
   friend class IsolateGroup;
-  friend class IsolateTestHelper;
   friend class NoActiveIsolateScope;
-  friend class NoOOBMessageScope;
   friend class NoReloadScope;
   friend class Simulator;
   friend class StackZone;

tests/lib/isolate/kill_infinite_loop_in_initializer_test.dart

@@ -0,0 +1,44 @@
+// Copyright (c) 2021, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// VMOptions=--enable-isolate-groups --experimental-enable-isolate-groups-jit
+// VMOptions=--no-enable-isolate-groups
+
+// Regression test against out-of-band messages being blocked during lazy
+// static field initialization.
+
+import "dart:isolate";
+import "dart:async";
+import "package:expect/expect.dart";
+import "package:async_helper/async_helper.dart";
+
+dynamic staticFieldWithBadInitializer = badInitializer();
+
+badInitializer() {
+  print("badInitializer");
+  for (;;) {}
+  return 42; // Unreachable.
+}
+
+child(message) {
+  print("child");
+  RawReceivePort port = new RawReceivePort();
+  print(staticFieldWithBadInitializer);
+  port.close(); // Unreachable.
+}
+
+void main() {
+  asyncStart();
+  Isolate.spawn(child, null).then((Isolate isolate) {
+    print("spawned");
+    late RawReceivePort exitSignal;
+    exitSignal = new RawReceivePort((_) {
+      print("onExit");
+      exitSignal.close();
+      asyncEnd();
+    });
+    isolate.addOnExitListener(exitSignal.sendPort);
+    isolate.kill(priority: Isolate.immediate);
+  });
+}

tests/lib_2/isolate/kill_infinite_loop_in_initializer_test.dart

@@ -0,0 +1,46 @@
+// Copyright (c) 2021, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// @dart = 2.9
+
+// VMOptions=--enable-isolate-groups --experimental-enable-isolate-groups-jit
+// VMOptions=--no-enable-isolate-groups
+
+// Regression test against out-of-band messages being blocked during lazy
+// static field initialization.
+
+import "dart:isolate";
+import "dart:async";
+import "package:expect/expect.dart";
+import "package:async_helper/async_helper.dart";
+
+dynamic staticFieldWithBadInitializer = badInitializer();
+
+badInitializer() {
+  print("badInitializer");
+  for (;;) {}
+  return 42; // Unreachable.
+}
+
+child(message) {
+  print("child");
+  RawReceivePort port = new RawReceivePort();
+  print(staticFieldWithBadInitializer);
+  port.close(); // Unreachable.
+}
+
+void main() {
+  asyncStart();
+  Isolate.spawn(child, null).then((Isolate isolate) {
+    print("spawned");
+    RawReceivePort exitSignal;
+    exitSignal = new RawReceivePort((_) {
+      print("onExit");
+      exitSignal.close();
+      asyncEnd();
+    });
+    isolate.addOnExitListener(exitSignal.sendPort);
+    isolate.kill(priority: Isolate.immediate);
+  });
+}