[vm/debugger] Ensure setting breakpoints is lock-safe.

Acquire reload opreation scope when deoptimizing the world to ensure locks can be acquired for compilation.
Set up scope for operations that can be run while the world is deoptimized and stopped to avoid races.
Ensure code stays unoptizimed when single stepping, prevent other isolates to reoptimize it.

TEST=DeoptimizeFramesWhenSettingBreakpoint
BUG=flutter/flutter#140878

Change-Id: Id4c891bd585d42365fd3a60cfb9a4869892c2b03
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/345743
Reviewed-by: Martin Kustermann <[email protected]>
Commit-Queue: Alexander Aprelev <[email protected]>

runtime/vm/debugger.cc

@@ -1506,7 +1506,6 @@ void Debugger::DeoptimizeWorld() {
 #if defined(DART_PRECOMPILED_RUNTIME)
   UNREACHABLE();
 #else
-  NoBackgroundCompilerScope no_bg_compiler(Thread::Current());
   if (FLAG_trace_deoptimization) {
     THR_Print("Deopt for debugger\n");
   }
@@ -1528,8 +1527,6 @@ void Debugger::DeoptimizeWorld() {
 
   const intptr_t num_classes = class_table.NumCids();
   const intptr_t num_tlc_classes = class_table.NumTopLevelCids();
-  // TODO(dartbug.com/36097): Need to stop other mutators running in same IG
-  // before deoptimizing the world.
   SafepointWriteRwLocker ml(thread, isolate_group->program_lock());
   for (intptr_t i = 1; i < num_classes + num_tlc_classes; i++) {
     const intptr_t cid =
@@ -1589,8 +1586,32 @@ void Debugger::DeoptimizeWorld() {
 #endif  // defined(DART_PRECOMPILED_RUNTIME)
 }
 
-void Debugger::NotifySingleStepping(bool value) const {
-  isolate_->set_single_step(value);
+void Debugger::RunWithStoppedDeoptimizedWorld(std::function<void()> fun) {
+#if !defined(DART_PRECOMPILED_RUNTIME)
+  // RELOAD_OPERATION_SCOPE is used here because is is guaranteed that
+  // isolates at reload safepoints hold no safepoint locks.
+  RELOAD_OPERATION_SCOPE(Thread::Current());
+  group_debugger()->isolate_group()->RunWithStoppedMutators([&]() {
+    DeoptimizeWorld();
+    fun();
+  });
+#endif
+}
+
+void Debugger::NotifySingleStepping(bool value) {
+  if (value) {
+    // Setting breakpoint requires unoptimized code, make sure we stop all
+    // isolates to prevent racing reoptimization.
+    RunWithStoppedDeoptimizedWorld([&] {
+      isolate_->set_single_step(value);
+      // Ensure other isolates in the isolate group keep
+      // unoptimized code unoptimized, won't attempt to optimize it.
+      group_debugger()->RegisterSingleSteppingDebugger(Thread::Current(), this);
+    });
+  } else {
+    isolate_->set_single_step(value);
+    group_debugger()->UnregisterSingleSteppingDebugger(Thread::Current(), this);
+  }
 }
 
 static ActivationFrame* CollectDartFrame(uword pc,
@@ -2576,10 +2597,15 @@ BreakpointLocation* Debugger::SetBreakpoint(
             FindExactTokenPosition(script, token_pos, requested_column);
       }
 #endif  // !defined(DART_PRECOMPILED_RUNTIME)
-      DeoptimizeWorld();
-      BreakpointLocation* loc =
-          SetCodeBreakpoints(scripts, token_pos, last_token_pos, requested_line,
-                             requested_column, exact_token_pos, code_functions);
+      BreakpointLocation* loc = nullptr;
+      // Ensure that code stays deoptimized (and background compiler disabled)
+      // until we have installed the breakpoint (at which point the compiler
+      // will not try to optimize it anymore).
+      RunWithStoppedDeoptimizedWorld([&] {
+        loc = SetCodeBreakpoints(scripts, token_pos, last_token_pos,
+                                 requested_line, requested_column,
+                                 exact_token_pos, code_functions);
+      });
       if (loc != nullptr) {
         return loc;
       }
@@ -3015,7 +3041,6 @@ void GroupDebugger::Pause() {
 
 void Debugger::EnterSingleStepMode() {
   ResetSteppingFramePointer();
-  DeoptimizeWorld();
   NotifySingleStepping(true);
 }
 
@@ -3039,14 +3064,12 @@ void Debugger::HandleSteppingRequest(bool skip_next_step /* = false */) {
     // the isolate has been interrupted, but can happen in other cases
     // as well.  We need to deoptimize the world in case we are about
     // to call an optimized function.
-    DeoptimizeWorld();
     NotifySingleStepping(true);
     skip_next_step_ = skip_next_step;
     if (FLAG_verbose_debug) {
       OS::PrintErr("HandleSteppingRequest - kStepInto\n");
     }
   } else if (resume_action_ == kStepOver) {
-    DeoptimizeWorld();
     NotifySingleStepping(true);
     skip_next_step_ = skip_next_step;
     SetSyncSteppingFramePointer(stack_trace_);
@@ -3071,7 +3094,6 @@ void Debugger::HandleSteppingRequest(bool skip_next_step /* = false */) {
     }
 
     // Fall through to synchronous stepping.
-    DeoptimizeWorld();
     NotifySingleStepping(true);
     // Find topmost caller that is debuggable.
     for (intptr_t i = 1; i < stack_trace_->Length(); i++) {
@@ -3352,13 +3374,13 @@ bool Debugger::IsDebuggable(const Function& func) {
 
 void GroupDebugger::RegisterSingleSteppingDebugger(Thread* thread,
                                                    const Debugger* debugger) {
-  ASSERT(single_stepping_set_lock()->IsCurrentThreadWriter());
+  WriteRwLocker sl(Thread::Current(), single_stepping_set_lock());
   single_stepping_set_.Insert(debugger);
 }
 
 void GroupDebugger::UnregisterSingleSteppingDebugger(Thread* thread,
                                                      const Debugger* debugger) {
-  ASSERT(single_stepping_set_lock()->IsCurrentThreadWriter());
+  WriteRwLocker sl(Thread::Current(), single_stepping_set_lock());
   single_stepping_set_.Remove(debugger);
 }
 
@@ -3400,7 +3422,6 @@ bool GroupDebugger::IsDebugging(Thread* thread, const Function& function) {
 
 void Debugger::set_resume_action(ResumeAction resume_action) {
   auto thread = Thread::Current();
-  WriteRwLocker sl(thread, group_debugger()->single_stepping_set_lock());
   if (resume_action == kContinue) {
     group_debugger()->UnregisterSingleSteppingDebugger(thread, this);
   } else {

runtime/vm/debugger.h

@@ -626,6 +626,8 @@ class GroupDebugger {
   bool HasBreakpoint(Thread* thread, const Function& function);
   bool IsDebugging(Thread* thread, const Function& function);
 
+  IsolateGroup* isolate_group() { return isolate_group_; }
+
  private:
   IsolateGroup* isolate_group_;
 
@@ -793,7 +795,8 @@ class Debugger {
                    TokenPosition last_token_pos,
                    Function* best_fit);
   void DeoptimizeWorld();
-  void NotifySingleStepping(bool value) const;
+  void RunWithStoppedDeoptimizedWorld(std::function<void()> fun);
+  void NotifySingleStepping(bool value);
   BreakpointLocation* SetCodeBreakpoints(
       const GrowableHandlePtrArray<const Script>& scripts,
       TokenPosition token_pos,

runtime/vm/heap/safepoint_test.cc

@@ -387,7 +387,7 @@ class CheckinTask : public StateMachineTask {
 
 // Test that mutators will not check-in to "deopt safepoint operations" at
 // at places where the mutator cannot depot (which is indicated by the
-// Thread::runtime_call_kind_ value).
+// [Thread::runtime_call_deopt_ability_] value).
 #if !defined(PRODUCT)
 ISOLATE_UNIT_TEST_CASE(SafepointOperation_SafepointPointTest) {
   auto isolate_group = thread->isolate_group();

runtime/vm/object_test.cc

@@ -5777,6 +5777,99 @@ TEST_CASE(FunctionWithBreakpointNotInlined) {
   }
 }
 
+void SetBreakpoint(Dart_NativeArguments args) {
+  // Refers to the DeoptimizeFramesWhenSettingBreakpoint function below.
+  const int kBreakpointLine = 8;
+
+  // This will force deoptimization of functions on stack.
+  // Function on stack has to be optimized, since we want to trigger debuggers
+  // on-stack deoptimization flow when we set a breakpoint.
+  Dart_Handle result =
+      Dart_SetBreakpoint(NewString(TestCase::url()), kBreakpointLine);
+  EXPECT_VALID(result);
+}
+
+static Dart_NativeFunction SetBreakpointResolver(Dart_Handle name,
+                                                 int argument_count,
+                                                 bool* auto_setup_scope) {
+  ASSERT(auto_setup_scope != nullptr);
+  *auto_setup_scope = true;
+  const char* cstr = nullptr;
+  Dart_Handle result = Dart_StringToCString(name, &cstr);
+  EXPECT_VALID(result);
+  EXPECT_STREQ(cstr, "setBreakpoint");
+  return &SetBreakpoint;
+}
+
+TEST_CASE(DeoptimizeFramesWhenSettingBreakpoint) {
+  const char* kOriginalScript = "test() {}";
+
+  Dart_Handle lib = TestCase::LoadTestScript(kOriginalScript, nullptr);
+  EXPECT_VALID(lib);
+  Dart_SetNativeResolver(lib, &SetBreakpointResolver, nullptr);
+
+  // Get unoptimized code for functions so they can be optimized.
+  Dart_Handle result = Dart_Invoke(lib, NewString("test"), 0, nullptr);
+  EXPECT_VALID(result);
+
+  // Launch second isolate so that running with stopped mutators during
+  // deoptimizattion requests a safepoint.
+  Dart_Isolate parent = Dart_CurrentIsolate();
+  Dart_ExitIsolate();
+  char* error = nullptr;
+  Dart_Isolate child = Dart_CreateIsolateInGroup(parent, "child",
+                                                 /*shutdown_callback=*/nullptr,
+                                                 /*cleanup_callback=*/nullptr,
+                                                 /*peer=*/nullptr, &error);
+  EXPECT_NE(nullptr, child);
+  EXPECT_EQ(nullptr, error);
+  Dart_ExitIsolate();
+  Dart_EnterIsolate(parent);
+
+  const char* kReloadScript =
+      R"(
+      @pragma("vm:external-name", "setBreakpoint")
+      external setBreakpoint();
+      baz() {}
+      test() {
+        if (true) {
+          setBreakpoint();
+        } else {
+          baz();   // this line gets a breakpoint
+        }
+      }
+      )";
+  lib = TestCase::ReloadTestScript(kReloadScript);
+  EXPECT_VALID(lib);
+
+  {
+    TransitionNativeToVM transition(thread);
+    const String& name = String::Handle(String::New(TestCase::url()));
+    const Library& vmlib =
+        Library::Handle(Library::LookupLibrary(thread, name));
+    EXPECT(!vmlib.IsNull());
+    Function& func_test = Function::Handle(GetFunction(vmlib, "test"));
+    Compiler::EnsureUnoptimizedCode(thread, func_test);
+    Compiler::CompileOptimizedFunction(thread, func_test);
+    func_test.set_unoptimized_code(Code::Handle(Code::null()));
+  }
+
+  result = Dart_Invoke(lib, NewString("test"), 0, nullptr);
+  EXPECT_VALID(result);
+
+  // Make sure child isolate finishes.
+  Dart_ExitIsolate();
+  Dart_EnterIsolate(child);
+  {
+    bool result =
+        Dart_RunLoopAsync(/*errors_are_fatal=*/true,
+                          /*on_error_port=*/0, /*on_exit_port=*/0, &error);
+    EXPECT_EQ(true, result);
+  }
+  EXPECT_EQ(nullptr, error);
+  Dart_EnterIsolate(parent);
+}
+
 ISOLATE_UNIT_TEST_CASE(SpecialClassesHaveEmptyArrays) {
   ObjectStore* object_store = IsolateGroup::Current()->object_store();
   Class& cls = Class::Handle();