diff --git a/runtime/vm/debugger.cc b/runtime/vm/debugger.cc index c05046ba8977..40ae71a74d20 100644 --- a/runtime/vm/debugger.cc +++ b/runtime/vm/debugger.cc @@ -411,9 +411,9 @@ static bool IsImplicitFunction(const Function& func) { return false; } -bool GroupDebugger::HasCodeBreakpointInFunction(const Function& func) { - auto thread = Thread::Current(); - ReadRwLocker sl(thread, code_breakpoints_lock()); +bool GroupDebugger::HasCodeBreakpointInFunctionUnsafe(const Function& func) { + DEBUG_ASSERT(code_breakpoints_lock()->IsCurrentThreadReader() || + Thread::Current()->IsInStoppedMutatorsScope()); CodeBreakpoint* cbpt = code_breakpoints_; while (cbpt != nullptr) { if (func.ptr() == cbpt->function()) { @@ -424,9 +424,20 @@ bool GroupDebugger::HasCodeBreakpointInFunction(const Function& func) { return false; } +bool GroupDebugger::HasCodeBreakpointInFunction(const Function& func) { + auto thread = Thread::Current(); + // Don't need to worry about the lock if mutators are stopped. + if (thread->IsInStoppedMutatorsScope()) { + return HasCodeBreakpointInFunctionUnsafe(func); + } else { + SafepointReadRwLocker sl(thread, code_breakpoints_lock()); + return HasCodeBreakpointInFunctionUnsafe(func); + } +} + bool GroupDebugger::HasBreakpointInCode(const Code& code) { auto thread = Thread::Current(); - ReadRwLocker sl(thread, code_breakpoints_lock()); + SafepointReadRwLocker sl(thread, code_breakpoints_lock()); CodeBreakpoint* cbpt = code_breakpoints_; while (cbpt != nullptr) { if (code.ptr() == cbpt->code_) { @@ -1376,9 +1387,9 @@ BreakpointLocation* CodeBreakpoint::FindBreakpointForDebugger( GroupDebugger::GroupDebugger(IsolateGroup* isolate_group) : isolate_group_(isolate_group), - code_breakpoints_lock_(new RwLock()), + code_breakpoints_lock_(new SafepointRwLock()), code_breakpoints_(nullptr), - breakpoint_locations_lock_(new RwLock()), + breakpoint_locations_lock_(new SafepointRwLock()), single_stepping_set_lock_(new RwLock()), needs_breakpoint_cleanup_(false) {} @@ -1424,8 +1435,8 @@ void Debugger::Shutdown() { return; } { - WriteRwLocker sl(Thread::Current(), - group_debugger()->breakpoint_locations_lock()); + SafepointWriteRwLocker sl(Thread::Current(), + group_debugger()->breakpoint_locations_lock()); while (breakpoint_locations_ != nullptr) { BreakpointLocation* loc = breakpoint_locations_; group_debugger()->UnlinkCodeBreakpoints(loc); @@ -2184,8 +2195,11 @@ bool BreakpointLocation::EnsureIsResolved(const Function& target_function, return true; } -void GroupDebugger::MakeCodeBreakpointAt(const Function& func, - BreakpointLocation* loc) { +void GroupDebugger::MakeCodeBreakpointAtUnsafe(const Function& func, + BreakpointLocation* loc) { + DEBUG_ASSERT(Thread::Current()->IsInStoppedMutatorsScope() || + code_breakpoints_lock()->IsCurrentThreadWriter()); + ASSERT(loc->token_pos().IsReal()); ASSERT((loc != nullptr) && loc->IsResolved()); ASSERT(!func.HasOptimizedCode()); @@ -2211,7 +2225,6 @@ void GroupDebugger::MakeCodeBreakpointAt(const Function& func, } uword lowest_pc = code.PayloadStart() + lowest_pc_offset; - WriteRwLocker sl(Thread::Current(), code_breakpoints_lock()); CodeBreakpoint* code_bpt = GetCodeBreakpoint(lowest_pc); if (code_bpt == nullptr) { // No code breakpoint for this code exists; create one. @@ -2240,6 +2253,17 @@ void GroupDebugger::MakeCodeBreakpointAt(const Function& func, } } +void GroupDebugger::MakeCodeBreakpointAt(const Function& func, + BreakpointLocation* loc) { + auto thread = Thread::Current(); + if (thread->IsInStoppedMutatorsScope()) { + MakeCodeBreakpointAtUnsafe(func, loc); + } else { + SafepointWriteRwLocker sl(thread, code_breakpoints_lock()); + MakeCodeBreakpointAtUnsafe(func, loc); + } +} + void Debugger::FindCompiledFunctions( const GrowableHandlePtrArray& scripts, TokenPosition start_pos, @@ -2645,7 +2669,7 @@ BreakpointLocation* Debugger::SetBreakpoint( // associated with the breakpoint location loc. void GroupDebugger::SyncBreakpointLocation(BreakpointLocation* loc) { bool any_enabled = loc->AnyEnabled(); - WriteRwLocker sl(Thread::Current(), code_breakpoints_lock()); + SafepointWriteRwLocker sl(Thread::Current(), code_breakpoints_lock()); CodeBreakpoint* cbpt = code_breakpoints_; while (cbpt != nullptr) { if (cbpt->HasBreakpointLocation(loc)) { @@ -3033,7 +3057,7 @@ void Debugger::Pause(ServiceEvent* event) { } void GroupDebugger::Pause() { - WriteRwLocker sl(Thread::Current(), code_breakpoints_lock()); + SafepointWriteRwLocker sl(Thread::Current(), code_breakpoints_lock()); if (needs_breakpoint_cleanup_) { RemoveUnlinkedCodeBreakpoints(); } @@ -3384,20 +3408,35 @@ void GroupDebugger::UnregisterSingleSteppingDebugger(Thread* thread, single_stepping_set_.Remove(debugger); } -bool GroupDebugger::HasBreakpoint(Thread* thread, const Function& function) { - { - ReadRwLocker(thread, breakpoint_locations_lock()); - // Check if function has any breakpoints. - String& url = String::Handle(thread->zone()); - for (intptr_t i = 0; i < breakpoint_locations_.length(); i++) { - BreakpointLocation* location = breakpoint_locations_.At(i); - url = location->url(); - if (FunctionOverlaps(function, url, location->token_pos(), - location->end_token_pos())) { - return true; - } +bool GroupDebugger::HasBreakpointUnsafe(Thread* thread, + const Function& function) { + DEBUG_ASSERT(thread->IsInStoppedMutatorsScope() || + breakpoint_locations_lock()->IsCurrentThreadReader()); + // Check if function has any breakpoints. + String& url = String::Handle(thread->zone()); + for (intptr_t i = 0; i < breakpoint_locations_.length(); i++) { + BreakpointLocation* location = breakpoint_locations_.At(i); + url = location->url(); + if (FunctionOverlaps(function, url, location->token_pos(), + location->end_token_pos())) { + return true; } } + return false; +} + +bool GroupDebugger::HasBreakpoint(Thread* thread, const Function& function) { + bool hasBreakpoint = false; + // Don't need to worry about the lock if mutators are stopped. + if (thread->IsInStoppedMutatorsScope()) { + hasBreakpoint = HasBreakpointUnsafe(thread, function); + } else { + SafepointReadRwLocker sl(thread, breakpoint_locations_lock()); + hasBreakpoint = HasBreakpointUnsafe(thread, function); + } + if (hasBreakpoint) { + return true; + } // TODO(aam): do we have to iterate over both code breakpoints and // breakpoint locations? Wouldn't be sufficient to iterate over only @@ -3573,8 +3612,8 @@ ErrorPtr Debugger::PauseBreakpoint() { BreakpointLocation* bpt_location = nullptr; const char* cbpt_tostring = nullptr; { - ReadRwLocker cbl(Thread::Current(), - group_debugger()->code_breakpoints_lock()); + SafepointReadRwLocker cbl(Thread::Current(), + group_debugger()->code_breakpoints_lock()); CodeBreakpoint* cbpt = nullptr; bpt_location = group_debugger()->GetBreakpointLocationFor( this, top_frame->pc(), &cbpt); @@ -3822,7 +3861,7 @@ void Debugger::NotifyDoneLoading() { // TODO(hausner): Could potentially make this faster by checking // whether the call target at pc is a debugger stub. bool GroupDebugger::HasActiveBreakpoint(uword pc) { - ReadRwLocker sl(Thread::Current(), code_breakpoints_lock()); + SafepointReadRwLocker sl(Thread::Current(), code_breakpoints_lock()); CodeBreakpoint* cbpt = GetCodeBreakpoint(pc); return (cbpt != nullptr) && (cbpt->IsEnabled()); } @@ -3843,7 +3882,7 @@ BreakpointLocation* GroupDebugger::GetBreakpointLocationFor( uword breakpoint_address, CodeBreakpoint** pcbpt) { ASSERT(pcbpt != nullptr); - ReadRwLocker sl(Thread::Current(), code_breakpoints_lock()); + SafepointReadRwLocker sl(Thread::Current(), code_breakpoints_lock()); *pcbpt = code_breakpoints_; while (*pcbpt != nullptr) { if ((*pcbpt)->pc() == breakpoint_address) { @@ -3863,7 +3902,7 @@ void GroupDebugger::RegisterCodeBreakpoint(CodeBreakpoint* cbpt) { } CodePtr GroupDebugger::GetPatchedStubAddress(uword breakpoint_address) { - ReadRwLocker sl(Thread::Current(), code_breakpoints_lock()); + SafepointReadRwLocker sl(Thread::Current(), code_breakpoints_lock()); CodeBreakpoint* cbpt = GetCodeBreakpoint(breakpoint_address); if (cbpt != nullptr) { return cbpt->OrigStubAddress(); @@ -3873,8 +3912,8 @@ CodePtr GroupDebugger::GetPatchedStubAddress(uword breakpoint_address) { } bool Debugger::SetBreakpointState(Breakpoint* bpt, bool enable) { - WriteRwLocker sl(Thread::Current(), - group_debugger()->breakpoint_locations_lock()); + SafepointWriteRwLocker sl(Thread::Current(), + group_debugger()->breakpoint_locations_lock()); if (bpt->is_enabled() != enable) { if (FLAG_verbose_debug) { OS::PrintErr("Setting breakpoint %" Pd " to state: %s\n", bpt->id(), @@ -3890,8 +3929,8 @@ bool Debugger::SetBreakpointState(Breakpoint* bpt, bool enable) { // Remove and delete the source breakpoint bpt and its associated // code breakpoints. void Debugger::RemoveBreakpoint(intptr_t bp_id) { - WriteRwLocker sl(Thread::Current(), - group_debugger()->breakpoint_locations_lock()); + SafepointWriteRwLocker sl(Thread::Current(), + group_debugger()->breakpoint_locations_lock()); if (RemoveBreakpointFromTheList(bp_id, &breakpoint_locations_)) { return; } @@ -3966,7 +4005,8 @@ bool Debugger::RemoveBreakpointFromTheList(intptr_t bp_id, } void GroupDebugger::RegisterBreakpointLocation(BreakpointLocation* location) { - ASSERT(breakpoint_locations_lock()->IsCurrentThreadWriter()); + DEBUG_ASSERT(breakpoint_locations_lock()->IsCurrentThreadWriter() || + Thread::Current()->IsInStoppedMutatorsScope()); breakpoint_locations_.Add(location); } @@ -3986,7 +4026,7 @@ void GroupDebugger::UnregisterBreakpointLocation(BreakpointLocation* location) { // should be hit before it gets deleted. void GroupDebugger::UnlinkCodeBreakpoints(BreakpointLocation* bpt_location) { ASSERT(bpt_location != nullptr); - WriteRwLocker sl(Thread::Current(), code_breakpoints_lock()); + SafepointWriteRwLocker sl(Thread::Current(), code_breakpoints_lock()); CodeBreakpoint* curr_bpt = code_breakpoints_; while (curr_bpt != nullptr) { if (curr_bpt->FindAndDeleteBreakpointLocation(bpt_location)) { @@ -4000,7 +4040,8 @@ void GroupDebugger::UnlinkCodeBreakpoints(BreakpointLocation* bpt_location) { // Remove and delete unlinked code breakpoints, i.e. breakpoints that // are not associated with a breakpoint location. void GroupDebugger::RemoveUnlinkedCodeBreakpoints() { - ASSERT(code_breakpoints_lock()->IsCurrentThreadWriter()); + DEBUG_ASSERT(code_breakpoints_lock()->IsCurrentThreadWriter() || + Thread::Current()->IsInStoppedMutatorsScope()); CodeBreakpoint* prev_bpt = nullptr; CodeBreakpoint* curr_bpt = code_breakpoints_; while (curr_bpt != nullptr) { @@ -4125,15 +4166,27 @@ BreakpointLocation* Debugger::GetLatentBreakpoint(const String& url, return loc; } -void Debugger::RegisterBreakpointLocation(BreakpointLocation* loc) { - WriteRwLocker sl(Thread::Current(), - group_debugger()->breakpoint_locations_lock()); +void Debugger::RegisterBreakpointLocationUnsafe(BreakpointLocation* loc) { + DEBUG_ASSERT( + group_debugger()->breakpoint_locations_lock()->IsCurrentThreadWriter() || + Thread::Current()->IsInStoppedMutatorsScope()); ASSERT(loc->next() == nullptr); loc->set_next(breakpoint_locations_); breakpoint_locations_ = loc; group_debugger()->RegisterBreakpointLocation(loc); } +void Debugger::RegisterBreakpointLocation(BreakpointLocation* loc) { + auto thread = Thread::Current(); + if (thread->IsInStoppedMutatorsScope()) { + RegisterBreakpointLocationUnsafe(loc); + } else { + SafepointWriteRwLocker sl(thread, + group_debugger()->breakpoint_locations_lock()); + RegisterBreakpointLocationUnsafe(loc); + } +} + #endif // !PRODUCT } // namespace dart diff --git a/runtime/vm/debugger.h b/runtime/vm/debugger.h index 16fb3aa7bd6a..02c76faca328 100644 --- a/runtime/vm/debugger.h +++ b/runtime/vm/debugger.h @@ -574,6 +574,8 @@ class GroupDebugger { explicit GroupDebugger(IsolateGroup* isolate_group); ~GroupDebugger(); + void MakeCodeBreakpointAtUnsafe(const Function& func, + BreakpointLocation* bpt); void MakeCodeBreakpointAt(const Function& func, BreakpointLocation* bpt); // Returns [nullptr] if no breakpoint exists for the given address. @@ -592,6 +594,7 @@ class GroupDebugger { // Returns true if the call at address pc is patched to point to // a debugger stub. bool HasActiveBreakpoint(uword pc); + bool HasCodeBreakpointInFunctionUnsafe(const Function& func); bool HasCodeBreakpointInFunction(const Function& func); bool HasCodeBreakpointInCode(const Code& code); @@ -609,9 +612,11 @@ class GroupDebugger { void VisitObjectPointers(ObjectPointerVisitor* visitor); - RwLock* code_breakpoints_lock() { return code_breakpoints_lock_.get(); } + SafepointRwLock* code_breakpoints_lock() { + return code_breakpoints_lock_.get(); + } - RwLock* breakpoint_locations_lock() { + SafepointRwLock* breakpoint_locations_lock() { return breakpoint_locations_lock_.get(); } @@ -623,6 +628,7 @@ class GroupDebugger { // Returns [true] if there is at least one breakpoint set in function or code. // Checks for both user-defined and internal temporary breakpoints. + bool HasBreakpointUnsafe(Thread* thread, const Function& function); bool HasBreakpoint(Thread* thread, const Function& function); bool IsDebugging(Thread* thread, const Function& function); @@ -631,13 +637,13 @@ class GroupDebugger { private: IsolateGroup* isolate_group_; - std::unique_ptr code_breakpoints_lock_; + std::unique_ptr code_breakpoints_lock_; CodeBreakpoint* code_breakpoints_; // Secondary list of all breakpoint_locations_(primary is in Debugger class). // This list is kept in sync with all the lists in Isolate Debuggers and is // used to quickly scan BreakpointLocations when new Function is compiled. - std::unique_ptr breakpoint_locations_lock_; + std::unique_ptr breakpoint_locations_lock_; MallocGrowableArray breakpoint_locations_; std::unique_ptr single_stepping_set_lock_; @@ -823,6 +829,7 @@ class Debugger { BreakpointLocation* GetLatentBreakpoint(const String& url, intptr_t line, intptr_t column); + void RegisterBreakpointLocationUnsafe(BreakpointLocation* loc); void RegisterBreakpointLocation(BreakpointLocation* bpt); BreakpointLocation* GetResolvedBreakpointLocation( const String& script_url, diff --git a/runtime/vm/debugger_api_impl_test.cc b/runtime/vm/debugger_api_impl_test.cc index 4f45f49ce3fa..98ab51206f54 100644 --- a/runtime/vm/debugger_api_impl_test.cc +++ b/runtime/vm/debugger_api_impl_test.cc @@ -157,6 +157,15 @@ Dart_Handle Dart_SetBreakpoint(Dart_Handle script_url_in, return Dart_NewInteger(bpt->id()); } +Dart_Handle Dart_RemoveBreakpoint(Dart_Handle breakpoint_id_in) { + DARTSCOPE(Thread::Current()); + Isolate* I = T->isolate(); + CHECK_DEBUGGER(I); + UNWRAP_AND_CHECK_PARAM(Integer, breakpoint_id, breakpoint_id_in); + I->debugger()->RemoveBreakpoint(breakpoint_id.AsInt64Value()); + return Api::Success(); +} + Dart_Handle Dart_EvaluateStaticExpr(Dart_Handle lib_handle, Dart_Handle expr_in) { DARTSCOPE(Thread::Current()); diff --git a/runtime/vm/debugger_api_impl_test.h b/runtime/vm/debugger_api_impl_test.h index 66ca912cb445..18eff22f4b4f 100644 --- a/runtime/vm/debugger_api_impl_test.h +++ b/runtime/vm/debugger_api_impl_test.h @@ -82,6 +82,12 @@ Dart_Handle Dart_GetLibraryDebuggable(intptr_t library_id, bool* is_debuggable); */ Dart_Handle Dart_SetLibraryDebuggable(intptr_t library_id, bool is_debuggable); +/** + * Remove breakpoint with provided id. + * + * Requires there to be a current isolate. + */ +Dart_Handle Dart_RemoveBreakpoint(Dart_Handle breakpoint_id); /** * Sets a breakpoint at line \line_number in \script_url, or the closest * following line (within the same function) where a breakpoint can be set. diff --git a/runtime/vm/object_test.cc b/runtime/vm/object_test.cc index 4a5a938f139c..7ce157ddef5b 100644 --- a/runtime/vm/object_test.cc +++ b/runtime/vm/object_test.cc @@ -5870,6 +5870,109 @@ TEST_CASE(DeoptimizeFramesWhenSettingBreakpoint) { Dart_EnterIsolate(parent); } +class ToggleBreakpointTask : public ThreadPool::Task { + public: + ToggleBreakpointTask(IsolateGroup* isolate_group, + Dart_Isolate isolate, + std::atomic* done) + : isolate_group_(isolate_group), isolate_(isolate), done_(done) {} + virtual void Run() { + Dart_EnterIsolate(isolate_); + Dart_EnterScope(); + const int kBreakpointLine = 5; // in the dart script below + Thread* t = Thread::Current(); + for (intptr_t i = 0; i < 1000; i++) { + Dart_Handle result = + Dart_SetBreakpoint(NewString(TestCase::url()), kBreakpointLine); + EXPECT_VALID(result); + int64_t breakpoint_id; + { + TransitionNativeToVM transition(t); + Integer& breakpoint_id_handle = Integer::Handle(); + breakpoint_id_handle ^= Api::UnwrapHandle(result); + breakpoint_id = breakpoint_id_handle.AsInt64Value(); + } + result = Dart_RemoveBreakpoint(Dart_NewInteger(breakpoint_id)); + EXPECT_VALID(result); + } + Dart_ExitScope(); + Dart_ExitIsolate(); + *done_ = true; + } + + private: + IsolateGroup* isolate_group_; + Dart_Isolate isolate_; + std::atomic* done_; +}; + +TEST_CASE(DartAPI_BreakpointLockRace) { + const char* kScriptChars = + "class A {\n" + " a() {\n" + " }\n" + " b() {\n" + " a();\n" // This is line 5. + " }\n" + "}\n" + "test() {\n" + " new A().b();\n" + "}"; + // Create a test library and Load up a test script in it. + Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, nullptr); + EXPECT_VALID(lib); + + // Run function A.b one time. + 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); + + // Run function A.b one time. + std::atomic done = false; + Dart::thread_pool()->Run(IsolateGroup::Current(), child, + &done); + + while (!done) { + ReloadParticipationScope allow_reload(thread); + { + TransitionNativeToVM transition(thread); + const String& name = String::Handle(String::New(TestCase::url())); + const Library& vmlib = + Library::Handle(Library::LookupLibrary(thread, name)); + EXPECT(!vmlib.IsNull()); + const Class& class_a = Class::Handle( + vmlib.LookupClass(String::Handle(Symbols::New(thread, "A")))); + Function& func_b = Function::Handle(GetFunction(class_a, "b")); + func_b.CanBeInlined(); + } + } + + // 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();