diff --git a/runtime/vm/debugger.cc b/runtime/vm/debugger.cc index dd80306f16e..c05046ba897 100644 --- a/runtime/vm/debugger.cc +++ b/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 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 { diff --git a/runtime/vm/debugger.h b/runtime/vm/debugger.h index 355b55c2280..16fb3aa7bd6 100644 --- a/runtime/vm/debugger.h +++ b/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 fun); + void NotifySingleStepping(bool value); BreakpointLocation* SetCodeBreakpoints( const GrowableHandlePtrArray& scripts, TokenPosition token_pos, diff --git a/runtime/vm/heap/safepoint_test.cc b/runtime/vm/heap/safepoint_test.cc index 0f08b45166f..e432c4957bd 100644 --- a/runtime/vm/heap/safepoint_test.cc +++ b/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(); diff --git a/runtime/vm/object_test.cc b/runtime/vm/object_test.cc index 664bdebbef5..4a5a938f139 100644 --- a/runtime/vm/object_test.cc +++ b/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();