[vm/concurrency] Add IsolateGroup::RunWithStoppedMutators and use it in various places

When installing new code, we need to ensure the mutator is stopped for a
variety of reasons.

Right now we only need to distuinguish mutator and background compiler
when installing code: The mutator can install code without any
synchronization whereas the bg compiler needs to get the mutator to a safepoint.

Yet once all isolates within one isolate group share a heap, a mutator
might need to stop all other mutators before installing code (since they
operate on pages on which we flip page protection bits)

This CL adds IsolateGroup::RunWithStoppedMutators which will get other
mutators to a safepoint (if there are multiple or we are on a bg
compiler thread).

Along with this we add a read-write lock and use it for the protection
of the IsolateGroup::isolate_: While we make assumptions about the
number of isolates in a group we force all pending additions of new
isolates to wait. Later on this will also be used to allow iterating the
list of isolates during GC and prevent new isolates from being added at
the same time.

Issue #36097

Change-Id: I6e761fa51d36b2f2b4b67995cac954898ce7fd69
Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-linux-product-x64-try,vm-kernel-precomp-linux-release-x64-try,vm-kernel-precomp-linux-release-x64-try,vm-kernel-precomp-android-release-arm-try,vm-dartkb-linux-release-x64-abi-try,vm-kernel-precomp-bare-linux-release-x64-try,vm-kernel-precomp-mac-debug-simarm_x64-try,vm-kernel-precomp-win-release-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/116767
Commit-Queue: Martin Kustermann <[email protected]>
Reviewed-by: Alexander Aprelev <[email protected]>
Reviewed-by: Ryan Macnak <[email protected]>

Author: mkustermann

runtime/vm/code_patcher_ia32.cc

@@ -64,8 +64,10 @@ class NativeCall : public UnoptimizedCall {
   }
 
   void set_native_function(NativeFunction func) const {
-    WritableInstructionsScope writable(start_ + 1, sizeof(func));
-    *reinterpret_cast<NativeFunction*>(start_ + 1) = func;
+    Thread::Current()->isolate_group()->RunWithStoppedMutators([&]() {
+      WritableInstructionsScope writable(start_ + 1, sizeof(func));
+      *reinterpret_cast<NativeFunction*>(start_ + 1) = func;
+    });
   }
 
  private:
@@ -179,11 +181,15 @@ RawCode* CodePatcher::GetStaticCallTargetAt(uword return_address,
 void CodePatcher::PatchStaticCallAt(uword return_address,
                                     const Code& code,
                                     const Code& new_target) {
-  const Instructions& instrs = Instructions::Handle(code.instructions());
-  WritableInstructionsScope writable(instrs.PayloadStart(), instrs.Size());
-  ASSERT(code.ContainsInstructionAt(return_address));
-  StaticCall call(return_address);
-  call.set_target(new_target);
+  auto thread = Thread::Current();
+  auto zone = thread->zone();
+  const Instructions& instrs = Instructions::Handle(zone, code.instructions());
+  thread->isolate_group()->RunWithStoppedMutators([&]() {
+    WritableInstructionsScope writable(instrs.PayloadStart(), instrs.Size());
+    ASSERT(code.ContainsInstructionAt(return_address));
+    StaticCall call(return_address);
+    call.set_target(new_target);
+  });
 }
 
 void CodePatcher::InsertDeoptimizationCallAt(uword start) {
@@ -205,12 +211,17 @@ void CodePatcher::PatchInstanceCallAt(uword return_address,
                                       const Code& caller_code,
                                       const Object& data,
                                       const Code& target) {
+  auto thread = Thread::Current();
+  auto zone = thread->zone();
   ASSERT(caller_code.ContainsInstructionAt(return_address));
-  const Instructions& instrs = Instructions::Handle(caller_code.instructions());
-  WritableInstructionsScope writable(instrs.PayloadStart(), instrs.Size());
-  InstanceCall call(return_address);
-  call.set_data(data);
-  call.set_target(target);
+  const Instructions& instrs =
+      Instructions::Handle(zone, caller_code.instructions());
+  thread->isolate_group()->RunWithStoppedMutators([&]() {
+    WritableInstructionsScope writable(instrs.PayloadStart(), instrs.Size());
+    InstanceCall call(return_address);
+    call.set_data(data);
+    call.set_target(target);
+  });
 }
 
 RawFunction* CodePatcher::GetUnoptimizedStaticCallAt(uword return_address,

runtime/vm/compiler/jit/compiler.cc

@@ -646,25 +646,34 @@ RawCode* CompileParsedFunctionHelper::Compile(CompilationPipeline* pipeline) {
       }
       {
         TIMELINE_DURATION(thread(), CompilerVerbose, "FinalizeCompilation");
-        if (thread()->IsMutatorThread()) {
+
+        auto mutator_fun = [&]() {
           *result =
               FinalizeCompilation(&assembler, &graph_compiler, flow_graph);
-        } else {
-          // This part of compilation must be at a safepoint.
-          // Stop mutator thread before creating the instruction object and
-          // installing code.
-          // Mutator thread may not run code while we are creating the
-          // instruction object, since the creation of instruction object
-          // changes code page access permissions (makes them temporary not
-          // executable).
-          {
-            CheckIfBackgroundCompilerIsBeingStopped(optimized());
-            ForceGrowthSafepointOperationScope safepoint_scope(thread());
+        };
+        auto bg_compiler_fun = [&]() {
+          if (Compiler::IsBackgroundCompilation()) {
             CheckIfBackgroundCompilerIsBeingStopped(optimized());
-            *result =
-                FinalizeCompilation(&assembler, &graph_compiler, flow_graph);
           }
-        }
+          *result =
+              FinalizeCompilation(&assembler, &graph_compiler, flow_graph);
+        };
+
+        // We have to ensure no mutators are running, because:
+        //
+        //   a) We allocate an instructions object, which might cause us to
+        //      temporarily flip page protections (RX -> RW -> RX).
+        //
+        //   b) We have to ensure the code generated does not violate
+        //      assumptions (e.g. CHA, field guards), the validation has to
+        //      happen while mutator is stopped.
+        //
+        //   b) We update the [Function] object with a new [Code] which
+        //      requires updating several pointers: We have to ensure all of
+        //      those writes are observed atomically.
+        //
+        thread()->isolate_group()->RunWithStoppedMutators(
+            mutator_fun, bg_compiler_fun, /*use_force_growth=*/true);
 
         // We notify code observers after finalizing the code in order to be
         // outside a [SafepointOperationScope].

runtime/vm/debugger_dbc.cc

@@ -29,9 +29,11 @@ static Instr* FastSmiInstructionFromReturnAddress(uword pc) {
 
 void CodeBreakpoint::PatchCode() {
   ASSERT(!is_enabled_);
-  const Code& code = Code::Handle(code_);
-  const Instructions& instrs = Instructions::Handle(code.instructions());
-  {
+  auto thread = Thread::Current();
+  auto zone = thread->zone();
+  const Code& code = Code::Handle(zone, code_);
+  const Instructions& instrs = Instructions::Handle(zone, code.instructions());
+  thread->isolate_group()->RunWithStoppedMutators([&]() {
     WritableInstructionsScope writable(instrs.PayloadStart(), instrs.Size());
     saved_value_ = *CallInstructionFromReturnAddress(pc_);
     switch (breakpoint_kind_) {
@@ -67,15 +69,17 @@ void CodeBreakpoint::PatchCode() {
     } else {
       saved_value_fastsmi_ = SimulatorBytecode::kTrap;
     }
-  }
+  });
   is_enabled_ = true;
 }
 
 void CodeBreakpoint::RestoreCode() {
   ASSERT(is_enabled_);
-  const Code& code = Code::Handle(code_);
-  const Instructions& instrs = Instructions::Handle(code.instructions());
-  {
+  auto thread = Thread::Current();
+  auto zone = thread->zone();
+  const Code& code = Code::Handle(zone, code_);
+  const Instructions& instrs = Instructions::Handle(zone, code.instructions());
+  thread->isolate_group()->RunWithStoppedMutators([&]() {
     WritableInstructionsScope writable(instrs.PayloadStart(), instrs.Size());
     switch (breakpoint_kind_) {
       case RawPcDescriptors::kIcCall:
@@ -94,7 +98,7 @@ void CodeBreakpoint::RestoreCode() {
              SimulatorBytecode::kNop);
       *FastSmiInstructionFromReturnAddress(pc_) = saved_value_fastsmi_;
     }
-  }
+  });
   is_enabled_ = false;
 }
 

runtime/vm/debugger_ia32.cc

@@ -25,10 +25,12 @@ RawCode* CodeBreakpoint::OrigStubAddress() const {
 
 void CodeBreakpoint::PatchCode() {
   ASSERT(!is_enabled_);
-  const Code& code = Code::Handle(code_);
-  const Instructions& instrs = Instructions::Handle(code.instructions());
-  Code& stub_target = Code::Handle();
-  {
+  auto thread = Thread::Current();
+  auto zone = thread->zone();
+  const Code& code = Code::Handle(zone, code_);
+  const Instructions& instrs = Instructions::Handle(zone, code.instructions());
+  Code& stub_target = Code::Handle(zone);
+  thread->isolate_group()->RunWithStoppedMutators([&]() {
     WritableInstructionsScope writable(instrs.PayloadStart(), instrs.Size());
     switch (breakpoint_kind_) {
       case RawPcDescriptors::kIcCall: {
@@ -49,15 +51,17 @@ void CodeBreakpoint::PatchCode() {
     }
     saved_value_ = CodePatcher::GetStaticCallTargetAt(pc_, code);
     CodePatcher::PatchStaticCallAt(pc_, code, stub_target);
-  }
+  });
   is_enabled_ = true;
 }
 
 void CodeBreakpoint::RestoreCode() {
   ASSERT(is_enabled_);
-  const Code& code = Code::Handle(code_);
-  const Instructions& instrs = Instructions::Handle(code.instructions());
-  {
+  auto thread = Thread::Current();
+  auto zone = thread->zone();
+  const Code& code = Code::Handle(zone, code_);
+  const Instructions& instrs = Instructions::Handle(zone, code.instructions());
+  thread->isolate_group()->RunWithStoppedMutators([&]() {
     WritableInstructionsScope writable(instrs.PayloadStart(), instrs.Size());
     switch (breakpoint_kind_) {
       case RawPcDescriptors::kIcCall:
@@ -69,7 +73,7 @@ void CodeBreakpoint::RestoreCode() {
       default:
         UNREACHABLE();
     }
-  }
+  });
   is_enabled_ = false;
 }
 

runtime/vm/isolate.cc

@@ -132,25 +132,25 @@ static RawInstance* DeserializeMessage(Thread* thread, Message* message) {
 
 IsolateGroup::IsolateGroup(std::unique_ptr<IsolateGroupSource> source,
                            void* embedder_data)
-    : source_(std::move(source)),
-      embedder_data_(embedder_data),
+    : embedder_data_(embedder_data),
+      isolates_rwlock_(new RwLock()),
+      isolates_(),
+      source_(std::move(source)),
       thread_registry_(new ThreadRegistry()),
-      safepoint_handler_(new SafepointHandler(this)),
-      isolates_monitor_(new Monitor()),
-      isolates_() {}
+      safepoint_handler_(new SafepointHandler(this)) {}
 
 IsolateGroup::~IsolateGroup() {}
 
 void IsolateGroup::RegisterIsolate(Isolate* isolate) {
-  MonitorLocker ml(isolates_monitor_.get());
+  WriteRwLocker wl(ThreadState::Current(), isolates_rwlock_.get());
   isolates_.Append(isolate);
   isolate_count_++;
 }
 
 void IsolateGroup::UnregisterIsolate(Isolate* isolate) {
   bool is_last_isolate = false;
   {
-    MonitorLocker ml(isolates_monitor_.get());
+    WriteRwLocker wl(ThreadState::Current(), isolates_rwlock_.get());
     isolates_.Remove(isolate);
     isolate_count_--;
     is_last_isolate = isolate_count_ == 0;
@@ -2226,6 +2226,30 @@ void Isolate::DisableIncrementalBarrier() {
   ASSERT(!Thread::Current()->is_marking());
 }
 
+void IsolateGroup::RunWithStoppedMutators(
+    std::function<void()> single_current_mutator,
+    std::function<void()> otherwise,
+    bool use_force_growth_in_otherwise) {
+  auto thread = Thread::Current();
+
+  ReadRwLocker wl(thread, isolates_rwlock_.get());
+  const bool only_one_isolate = isolates_.First() == isolates_.Last();
+  if (thread->IsMutatorThread() && only_one_isolate) {
+    single_current_mutator();
+  } else {
+    // We use the more strict safepoint operation scope here (which ensures that
+    // all other threads, including auxiliary threads are at a safepoint), even
+    // though we only need to ensure that the mutator threads are stopped.
+    if (use_force_growth_in_otherwise) {
+      ForceGrowthSafepointOperationScope safepoint_scope(thread);
+      otherwise();
+    } else {
+      SafepointOperationScope safepoint_scope(thread);
+      otherwise();
+    }
+  }
+}
+
 RawClass* Isolate::GetClassForHeapWalkAt(intptr_t cid) {
   RawClass* raw_class = nullptr;
 #if !defined(PRODUCT) && !defined(DART_PRECOMPILED_RUNTIME)

runtime/vm/isolate.h

@@ -9,6 +9,7 @@
 #error "Should not include runtime"
 #endif
 
+#include <functional>
 #include <memory>
 #include <utility>
 
@@ -78,6 +79,7 @@ class RawFloat32x4;
 class RawInt32x4;
 class RawUserTag;
 class ReversePcLookupCache;
+class RwLock;
 class SafepointHandler;
 class SampleBuffer;
 class SendPort;
@@ -262,16 +264,36 @@ class IsolateGroup {
     library_tag_handler_ = handler;
   }
 
+  // Ensures mutators are stopped during execution of the provided function.
+  //
+  // If the current thread is the only mutator in the isolate group,
+  // [single_current_mutator] will be called. Otherwise [otherwise] will be
+  // called inside a [SafepointOperationsScope] (or
+  // [ForceGrowthSafepointOperationScope] if [use_force_growth_in_otherwise]
+  // is set).
+  //
+  // During the duration of this function, no new isolates can be added to the
+  // isolate group.
+  void RunWithStoppedMutators(std::function<void()> single_current_mutator,
+                              std::function<void()> otherwise,
+                              bool use_force_growth_in_otherwise = false);
+
+  void RunWithStoppedMutators(std::function<void()> function) {
+    RunWithStoppedMutators(function, function);
+  }
+
  private:
-  std::unique_ptr<IsolateGroupSource> source_;
   void* embedder_data_ = nullptr;
-  std::unique_ptr<ThreadRegistry> thread_registry_;
-  std::unique_ptr<SafepointHandler> safepoint_handler_;
-  std::unique_ptr<Monitor> isolates_monitor_;
+
+  std::unique_ptr<RwLock> isolates_rwlock_;
   IntrusiveDList<Isolate> isolates_;
   intptr_t isolate_count_ = 0;
   bool initial_spawn_successful_ = false;
   Dart_LibraryTagHandler library_tag_handler_ = nullptr;
+
+  std::unique_ptr<IsolateGroupSource> source_;
+  std::unique_ptr<ThreadRegistry> thread_registry_;
+  std::unique_ptr<SafepointHandler> safepoint_handler_;
 };
 
 class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {

runtime/vm/lockers.cc

@@ -101,4 +101,22 @@ Monitor::WaitResult SafepointMonitorLocker::Wait(int64_t millis) {
   }
 }
 
+ReadRwLocker::ReadRwLocker(ThreadState* thread_state, RwLock* rw_lock)
+    : StackResource(thread_state), rw_lock_(rw_lock) {
+  rw_lock_->EnterRead();
+}
+
+ReadRwLocker::~ReadRwLocker() {
+  rw_lock_->LeaveRead();
+}
+
+WriteRwLocker::WriteRwLocker(ThreadState* thread_state, RwLock* rw_lock)
+    : StackResource(thread_state), rw_lock_(rw_lock) {
+  rw_lock_->EnterWrite();
+}
+
+WriteRwLocker::~WriteRwLocker() {
+  rw_lock_->LeaveWrite();
+}
+
 }  // namespace dart