From 2f457d1a8a6c8d779a1ade6c378d582271db2a93 Mon Sep 17 00:00:00 2001 From: Alexander Aprelev Date: Thu, 31 Aug 2023 19:52:23 +0000 Subject: [PATCH] [vm/debugger] Use RwLock instead of SafepointRwLock, get rid of RunUnderLockIfNeeded. RwLock, rather that SafepointRwLock, is okay to use for these two breakpoint_locations and single_stepping_set locks because they are short-lived, should not spawn over the safepoint. TEST=ci Change-Id: Iba83291978ba7980d0c6fd0a0a2cf2174c801359 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/323202 Reviewed-by: Ryan Macnak Commit-Queue: Alexander Aprelev --- runtime/vm/debugger.cc | 69 +++++++++++++++++------------------------- runtime/vm/debugger.h | 23 +++----------- 2 files changed, 33 insertions(+), 59 deletions(-) diff --git a/runtime/vm/debugger.cc b/runtime/vm/debugger.cc index ed44e366bc6..779992ba5fc 100644 --- a/runtime/vm/debugger.cc +++ b/runtime/vm/debugger.cc @@ -1374,8 +1374,8 @@ GroupDebugger::GroupDebugger(IsolateGroup* isolate_group) : isolate_group_(isolate_group), code_breakpoints_lock_(new RwLock()), code_breakpoints_(nullptr), - breakpoint_locations_lock_(new SafepointRwLock()), - single_stepping_set_lock_(new SafepointRwLock()), + breakpoint_locations_lock_(new RwLock()), + single_stepping_set_lock_(new RwLock()), needs_breakpoint_cleanup_(false) {} GroupDebugger::~GroupDebugger() { @@ -1420,8 +1420,8 @@ void Debugger::Shutdown() { return; } { - SafepointWriteRwLocker sl(Thread::Current(), - group_debugger()->breakpoint_locations_lock()); + WriteRwLocker sl(Thread::Current(), + group_debugger()->breakpoint_locations_lock()); while (breakpoint_locations_ != nullptr) { BreakpointLocation* loc = breakpoint_locations_; group_debugger()->UnlinkCodeBreakpoints(loc); @@ -3328,32 +3328,19 @@ void GroupDebugger::UnregisterSingleSteppingDebugger(Thread* thread, single_stepping_set_.Remove(debugger); } -bool GroupDebugger::RunUnderReadLockIfNeededCallable(Thread* thread, - SafepointRwLock* rw_lock, - BoolCallable* callable) { - if (thread->IsInStoppedMutatorsScope()) { - return callable->Call(); - } - - SafepointReadRwLocker sl(thread, rw_lock); - return callable->Call(); -} - bool GroupDebugger::HasBreakpoint(Thread* thread, const Function& function) { - if (RunUnderReadLockIfNeeded(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; - } - } - return false; - })) { - return true; + { + 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; + } + } } // TODO(aam): do we have to iterate over both code breakpoints and @@ -3368,18 +3355,18 @@ bool GroupDebugger::HasBreakpoint(Thread* thread, const Function& function) { } bool GroupDebugger::IsDebugging(Thread* thread, const Function& function) { - if (!RunUnderReadLockIfNeeded(thread, single_stepping_set_lock(), [&]() { - return single_stepping_set_.IsEmpty(); - })) { - return true; + { + ReadRwLocker ml(thread, single_stepping_set_lock()); + if (!single_stepping_set_.IsEmpty()) { + return true; + } } return HasBreakpoint(thread, function); } void Debugger::set_resume_action(ResumeAction resume_action) { auto thread = Thread::Current(); - SafepointWriteRwLocker sl(thread, - group_debugger()->single_stepping_set_lock()); + WriteRwLocker sl(thread, group_debugger()->single_stepping_set_lock()); if (resume_action == kContinue) { group_debugger()->UnregisterSingleSteppingDebugger(thread, this); } else { @@ -3831,8 +3818,8 @@ CodePtr GroupDebugger::GetPatchedStubAddress(uword breakpoint_address) { } bool Debugger::SetBreakpointState(Breakpoint* bpt, bool enable) { - SafepointWriteRwLocker sl(Thread::Current(), - group_debugger()->breakpoint_locations_lock()); + WriteRwLocker 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(), @@ -3848,8 +3835,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) { - SafepointWriteRwLocker sl(Thread::Current(), - group_debugger()->breakpoint_locations_lock()); + WriteRwLocker sl(Thread::Current(), + group_debugger()->breakpoint_locations_lock()); if (RemoveBreakpointFromTheList(bp_id, &breakpoint_locations_)) { return; } @@ -4084,8 +4071,8 @@ BreakpointLocation* Debugger::GetLatentBreakpoint(const String& url, } void Debugger::RegisterBreakpointLocation(BreakpointLocation* loc) { - SafepointWriteRwLocker sl(Thread::Current(), - group_debugger()->breakpoint_locations_lock()); + WriteRwLocker sl(Thread::Current(), + group_debugger()->breakpoint_locations_lock()); ASSERT(loc->next() == nullptr); loc->set_next(breakpoint_locations_); breakpoint_locations_ = loc; diff --git a/runtime/vm/debugger.h b/runtime/vm/debugger.h index 50205fc02e3..355b55c2280 100644 --- a/runtime/vm/debugger.h +++ b/runtime/vm/debugger.h @@ -611,29 +611,16 @@ class GroupDebugger { RwLock* code_breakpoints_lock() { return code_breakpoints_lock_.get(); } - SafepointRwLock* breakpoint_locations_lock() { + RwLock* breakpoint_locations_lock() { return breakpoint_locations_lock_.get(); } - SafepointRwLock* single_stepping_set_lock() { - return single_stepping_set_lock_.get(); - } + RwLock* single_stepping_set_lock() { return single_stepping_set_lock_.get(); } + void RegisterSingleSteppingDebugger(Thread* thread, const Debugger* debugger); void UnregisterSingleSteppingDebugger(Thread* thread, const Debugger* debugger); - bool RunUnderReadLockIfNeededCallable(Thread* thread, - SafepointRwLock* rw_lock, - BoolCallable* callable); - - template - bool RunUnderReadLockIfNeeded(Thread* thread, - SafepointRwLock* rw_lock, - T function) { - LambdaBoolCallable callable(function); - return RunUnderReadLockIfNeededCallable(thread, rw_lock, &callable); - } - // Returns [true] if there is at least one breakpoint set in function or code. // Checks for both user-defined and internal temporary breakpoints. bool HasBreakpoint(Thread* thread, const Function& function); @@ -648,10 +635,10 @@ class GroupDebugger { // 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_; + std::unique_ptr single_stepping_set_lock_; DebuggerSet single_stepping_set_; void RemoveUnlinkedCodeBreakpoints();