Skip to content

Commit

Permalink
[vm/debugger] Use safepoint-safe RwLock for breakpoint locations locks.
Browse files Browse the repository at this point in the history 
Fixes #54650
TEST=DartAPI_BreakpointLockRace

Change-Id: I83b5be80d66ad30c46ea99de08ce7a8d70182414
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/347420
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Alexander Aprelev <aam@google.com>
  • Loading branch information
@aam
aam authored and Commit Queue committed Jan 22, 2024
1 parent eb5c77e commit 84b38ae
Show file tree
Hide file tree
Showing 5 changed files with 222 additions and 44 deletions.
133 changes: 93 additions & 40 deletions runtime/vm/debugger.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand All @@ -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_) {
Expand Down Expand Up @@ -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) {}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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());
Expand All @@ -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.
Expand Down Expand Up @@ -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<const Script>& scripts,
TokenPosition start_pos,
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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());
}
Expand All @@ -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) {
Expand All @@ -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();
Expand All @@ -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(),
Expand All @@ -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;
}
Expand Down Expand Up @@ -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);
}

Expand All @@ -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)) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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
15 changes: 11 additions & 4 deletions runtime/vm/debugger.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);

Expand All @@ -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();
}

Expand All @@ -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);

Expand All @@ -631,13 +637,13 @@ class GroupDebugger {
private:
IsolateGroup* isolate_group_;

std::unique_ptr<RwLock> code_breakpoints_lock_;
std::unique_ptr<SafepointRwLock> 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<RwLock> breakpoint_locations_lock_;
std::unique_ptr<SafepointRwLock> breakpoint_locations_lock_;
MallocGrowableArray<BreakpointLocation*> breakpoint_locations_;

std::unique_ptr<RwLock> single_stepping_set_lock_;
Expand Down Expand Up @@ -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,
Expand Down
9 changes: 9 additions & 0 deletions runtime/vm/debugger_api_impl_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
6 changes: 6 additions & 0 deletions runtime/vm/debugger_api_impl_test.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading

0 comments on commit 84b38ae

Please sign in to comment.