[lldb] Guard SBFrame/SBThread methods against running processes #152020
+508
−384
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Felipe de Azevedo Piovezan (felipepiovezan) ChangesPrior to this patch, SBFrame/SBThread methods exhibit racy behavior if called while the process is running, because they do not lock the Some methods attempt to protect against this with the pattern: However, this is also racy: the constructor of With this patch:
This commit exposes many opportunities for early-returns, but these would increase the diff of this patch and distract from the important changes, so we opt not to do it here. Patch is 65.73 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/152020.diff 7 Files Affected:
diff --git a/lldb/include/lldb/API/SBFrame.h b/lldb/include/lldb/API/SBFrame.h
index 3635ee5a537ad..35198068814d0 100644
--- a/lldb/include/lldb/API/SBFrame.h
+++ b/lldb/include/lldb/API/SBFrame.h
@@ -233,6 +233,10 @@ class LLDB_API SBFrame {
void SetFrameSP(const lldb::StackFrameSP &lldb_object_sp);
+ /// Return an SBValue with an error message warning that the process is not
+ /// currently stopped.
+ static SBValue CreateProcessIsRunningExprEvalError();
+
lldb::ExecutionContextRefSP m_opaque_sp;
};
diff --git a/lldb/include/lldb/Host/ProcessRunLock.h b/lldb/include/lldb/Host/ProcessRunLock.h
index 94683673024d5..5efa46154a61e 100644
--- a/lldb/include/lldb/Host/ProcessRunLock.h
+++ b/lldb/include/lldb/Host/ProcessRunLock.h
@@ -41,9 +41,14 @@ class ProcessRunLock {
class ProcessRunLocker {
public:
ProcessRunLocker() = default;
+ ProcessRunLocker(ProcessRunLocker &&other) : m_lock(other.m_lock) {
+ other.m_lock = nullptr;
+ }
~ProcessRunLocker() { Unlock(); }
+ bool IsLocked() const { return m_lock; }
+
// Try to lock the read lock, but only do so if there are no writers.
bool TryLock(ProcessRunLock *lock) {
if (m_lock) {
diff --git a/lldb/include/lldb/Target/ExecutionContext.h b/lldb/include/lldb/Target/ExecutionContext.h
index aebd0d5308e72..ae6051f576fac 100644
--- a/lldb/include/lldb/Target/ExecutionContext.h
+++ b/lldb/include/lldb/Target/ExecutionContext.h
@@ -11,6 +11,7 @@
#include <mutex>
+#include "lldb/Host/ProcessRunLock.h"
#include "lldb/Target/StackID.h"
#include "lldb/lldb-private.h"
@@ -318,11 +319,13 @@ class ExecutionContext {
// These two variants take in a locker, and grab the target, lock the API
// mutex into locker, then fill in the rest of the shared pointers.
ExecutionContext(const ExecutionContextRef &exe_ctx_ref,
- std::unique_lock<std::recursive_mutex> &locker)
- : ExecutionContext(&exe_ctx_ref, locker) {}
+ std::unique_lock<std::recursive_mutex> &locker,
+ ProcessRunLock::ProcessRunLocker &stop_locker)
+ : ExecutionContext(&exe_ctx_ref, locker, stop_locker) {}
ExecutionContext(const ExecutionContextRef *exe_ctx_ref,
- std::unique_lock<std::recursive_mutex> &locker);
+ std::unique_lock<std::recursive_mutex> &locker,
+ ProcessRunLock::ProcessRunLocker &stop_locker);
// Create execution contexts from execution context scopes
ExecutionContext(ExecutionContextScope *exe_scope);
ExecutionContext(ExecutionContextScope &exe_scope);
diff --git a/lldb/source/API/SBFrame.cpp b/lldb/source/API/SBFrame.cpp
index 5b69cf1ee2641..b6fd5e7d3524b 100644
--- a/lldb/source/API/SBFrame.cpp
+++ b/lldb/source/API/SBFrame.cpp
@@ -99,14 +99,15 @@ SBFrame::operator bool() const {
LLDB_INSTRUMENT_VA(this);
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return false;
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock()))
- return GetFrameSP().get() != nullptr;
+ return GetFrameSP().get() != nullptr;
}
// Without a target & process we can't have a valid stack frame.
@@ -118,16 +119,16 @@ SBSymbolContext SBFrame::GetSymbolContext(uint32_t resolve_scope) const {
SBSymbolContext sb_sym_ctx;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return sb_sym_ctx;
SymbolContextItem scope = static_cast<SymbolContextItem>(resolve_scope);
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock())) {
- if (StackFrame *frame = exe_ctx.GetFramePtr())
- sb_sym_ctx = frame->GetSymbolContext(scope);
- }
+ if (StackFrame *frame = exe_ctx.GetFramePtr())
+ sb_sym_ctx = frame->GetSymbolContext(scope);
}
return sb_sym_ctx;
@@ -139,19 +140,19 @@ SBModule SBFrame::GetModule() const {
SBModule sb_module;
ModuleSP module_sp;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return sb_module;
StackFrame *frame = nullptr;
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock())) {
- frame = exe_ctx.GetFramePtr();
- if (frame) {
- module_sp = frame->GetSymbolContext(eSymbolContextModule).module_sp;
- sb_module.SetSP(module_sp);
- }
+ frame = exe_ctx.GetFramePtr();
+ if (frame) {
+ module_sp = frame->GetSymbolContext(eSymbolContextModule).module_sp;
+ sb_module.SetSP(module_sp);
}
}
@@ -163,19 +164,19 @@ SBCompileUnit SBFrame::GetCompileUnit() const {
SBCompileUnit sb_comp_unit;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return sb_comp_unit;
StackFrame *frame = nullptr;
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock())) {
- frame = exe_ctx.GetFramePtr();
- if (frame) {
- sb_comp_unit.reset(
- frame->GetSymbolContext(eSymbolContextCompUnit).comp_unit);
- }
+ frame = exe_ctx.GetFramePtr();
+ if (frame) {
+ sb_comp_unit.reset(
+ frame->GetSymbolContext(eSymbolContextCompUnit).comp_unit);
}
}
@@ -187,19 +188,19 @@ SBFunction SBFrame::GetFunction() const {
SBFunction sb_function;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return sb_function;
StackFrame *frame = nullptr;
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock())) {
- frame = exe_ctx.GetFramePtr();
- if (frame) {
- sb_function.reset(
- frame->GetSymbolContext(eSymbolContextFunction).function);
- }
+ frame = exe_ctx.GetFramePtr();
+ if (frame) {
+ sb_function.reset(
+ frame->GetSymbolContext(eSymbolContextFunction).function);
}
}
@@ -211,18 +212,18 @@ SBSymbol SBFrame::GetSymbol() const {
SBSymbol sb_symbol;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return sb_symbol;
StackFrame *frame = nullptr;
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock())) {
- frame = exe_ctx.GetFramePtr();
- if (frame) {
- sb_symbol.reset(frame->GetSymbolContext(eSymbolContextSymbol).symbol);
- }
+ frame = exe_ctx.GetFramePtr();
+ if (frame) {
+ sb_symbol.reset(frame->GetSymbolContext(eSymbolContextSymbol).symbol);
}
}
@@ -234,18 +235,18 @@ SBBlock SBFrame::GetBlock() const {
SBBlock sb_block;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return sb_block;
StackFrame *frame = nullptr;
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock())) {
- frame = exe_ctx.GetFramePtr();
- if (frame)
- sb_block.SetPtr(frame->GetSymbolContext(eSymbolContextBlock).block);
- }
+ frame = exe_ctx.GetFramePtr();
+ if (frame)
+ sb_block.SetPtr(frame->GetSymbolContext(eSymbolContextBlock).block);
}
return sb_block;
}
@@ -255,18 +256,18 @@ SBBlock SBFrame::GetFrameBlock() const {
SBBlock sb_block;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return sb_block;
StackFrame *frame = nullptr;
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock())) {
- frame = exe_ctx.GetFramePtr();
- if (frame)
- sb_block.SetPtr(frame->GetFrameBlock());
- }
+ frame = exe_ctx.GetFramePtr();
+ if (frame)
+ sb_block.SetPtr(frame->GetFrameBlock());
}
return sb_block;
}
@@ -276,19 +277,19 @@ SBLineEntry SBFrame::GetLineEntry() const {
SBLineEntry sb_line_entry;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return sb_line_entry;
StackFrame *frame = nullptr;
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock())) {
- frame = exe_ctx.GetFramePtr();
- if (frame) {
- sb_line_entry.SetLineEntry(
- frame->GetSymbolContext(eSymbolContextLineEntry).line_entry);
- }
+ frame = exe_ctx.GetFramePtr();
+ if (frame) {
+ sb_line_entry.SetLineEntry(
+ frame->GetSymbolContext(eSymbolContextLineEntry).line_entry);
}
}
return sb_line_entry;
@@ -300,7 +301,10 @@ uint32_t SBFrame::GetFrameID() const {
uint32_t frame_idx = UINT32_MAX;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return frame_idx;
StackFrame *frame = exe_ctx.GetFramePtr();
if (frame)
@@ -313,7 +317,10 @@ lldb::addr_t SBFrame::GetCFA() const {
LLDB_INSTRUMENT_VA(this);
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return LLDB_INVALID_ADDRESS;
StackFrame *frame = exe_ctx.GetFramePtr();
if (frame)
@@ -326,19 +333,19 @@ addr_t SBFrame::GetPC() const {
addr_t addr = LLDB_INVALID_ADDRESS;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return addr;
StackFrame *frame = nullptr;
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock())) {
- frame = exe_ctx.GetFramePtr();
- if (frame) {
- addr = frame->GetFrameCodeAddress().GetOpcodeLoadAddress(
- target, AddressClass::eCode);
- }
+ frame = exe_ctx.GetFramePtr();
+ if (frame) {
+ addr = frame->GetFrameCodeAddress().GetOpcodeLoadAddress(
+ target, AddressClass::eCode);
}
}
@@ -350,17 +357,17 @@ bool SBFrame::SetPC(addr_t new_pc) {
bool ret_val = false;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return ret_val;
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock())) {
- if (StackFrame *frame = exe_ctx.GetFramePtr()) {
- if (RegisterContextSP reg_ctx_sp = frame->GetRegisterContext()) {
- ret_val = reg_ctx_sp->SetPC(new_pc);
- }
+ if (StackFrame *frame = exe_ctx.GetFramePtr()) {
+ if (RegisterContextSP reg_ctx_sp = frame->GetRegisterContext()) {
+ ret_val = reg_ctx_sp->SetPC(new_pc);
}
}
}
@@ -373,17 +380,17 @@ addr_t SBFrame::GetSP() const {
addr_t addr = LLDB_INVALID_ADDRESS;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return addr;
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock())) {
- if (StackFrame *frame = exe_ctx.GetFramePtr()) {
- if (RegisterContextSP reg_ctx_sp = frame->GetRegisterContext()) {
- addr = reg_ctx_sp->GetSP();
- }
+ if (StackFrame *frame = exe_ctx.GetFramePtr()) {
+ if (RegisterContextSP reg_ctx_sp = frame->GetRegisterContext()) {
+ addr = reg_ctx_sp->GetSP();
}
}
}
@@ -396,17 +403,17 @@ addr_t SBFrame::GetFP() const {
addr_t addr = LLDB_INVALID_ADDRESS;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return addr;
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock())) {
- if (StackFrame *frame = exe_ctx.GetFramePtr()) {
- if (RegisterContextSP reg_ctx_sp = frame->GetRegisterContext()) {
- addr = reg_ctx_sp->GetFP();
- }
+ if (StackFrame *frame = exe_ctx.GetFramePtr()) {
+ if (RegisterContextSP reg_ctx_sp = frame->GetRegisterContext()) {
+ addr = reg_ctx_sp->GetFP();
}
}
}
@@ -419,18 +426,18 @@ SBAddress SBFrame::GetPCAddress() const {
SBAddress sb_addr;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return sb_addr;
StackFrame *frame = exe_ctx.GetFramePtr();
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock())) {
- frame = exe_ctx.GetFramePtr();
- if (frame)
- sb_addr.SetAddress(frame->GetFrameCodeAddress());
- }
+ frame = exe_ctx.GetFramePtr();
+ if (frame)
+ sb_addr.SetAddress(frame->GetFrameCodeAddress());
}
return sb_addr;
}
@@ -446,7 +453,10 @@ lldb::SBValue SBFrame::GetValueForVariablePath(const char *var_path) {
SBValue sb_value;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return sb_value;
StackFrame *frame = exe_ctx.GetFramePtr();
Target *target = exe_ctx.GetTargetPtr();
@@ -468,25 +478,25 @@ lldb::SBValue SBFrame::GetValueForVariablePath(const char *var_path,
}
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return sb_value;
StackFrame *frame = nullptr;
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock())) {
- frame = exe_ctx.GetFramePtr();
- if (frame) {
- VariableSP var_sp;
- Status error;
- ValueObjectSP value_sp(frame->GetValueForVariableExpressionPath(
- var_path, eNoDynamicValues,
- StackFrame::eExpressionPathOptionCheckPtrVsMember |
- StackFrame::eExpressionPathOptionsAllowDirectIVarAccess,
- var_sp, error));
- sb_value.SetSP(value_sp, use_dynamic);
- }
+ frame = exe_ctx.GetFramePtr();
+ if (frame) {
+ VariableSP var_sp;
+ Status error;
+ ValueObjectSP value_sp(frame->GetValueForVariableExpressionPath(
+ var_path, eNoDynamicValues,
+ StackFrame::eExpressionPathOptionCheckPtrVsMember |
+ StackFrame::eExpressionPathOptionsAllowDirectIVarAccess,
+ var_sp, error));
+ sb_value.SetSP(value_sp, use_dynamic);
}
}
return sb_value;
@@ -497,7 +507,10 @@ SBValue SBFrame::FindVariable(const char *name) {
SBValue value;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return value;
StackFrame *frame = exe_ctx.GetFramePtr();
Target *target = exe_ctx.GetTargetPtr();
@@ -522,21 +535,21 @@ SBValue SBFrame::FindVariable(const char *name,
ValueObjectSP value_sp;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return sb_value;
StackFrame *frame = nullptr;
Target *target = exe_ctx.GetTargetPtr();
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
- Process::StopLocker stop_locker;
- if (stop_locker.TryLock(&process->GetRunLock())) {
- frame = exe_ctx.GetFramePtr();
- if (frame) {
- value_sp = frame->FindVariable(ConstString(name));
+ frame = exe_ctx.GetFramePtr();
+ if (frame) {
+ value_sp = frame->FindVariable(ConstString(name));
- if (value_sp)
- sb_value.SetSP(value_sp, use_dynamic);
- }
+ if (value_sp)
+ sb_value.SetSP(value_sp, use_dynamic);
}
}
@@ -548,7 +561,10 @@ SBValue SBFrame::FindValue(const char *name, ValueType value_type) {
SBValue value;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker);
+ if (!stop_locker.IsLocked())
+ return value;
StackFrame *frame = exe_ctx.GetFramePtr();
Target *target = exe_ctx.GetTargetPtr();
@@ -572,14 +588,14 @@ SBValue SBFrame::FindValue(const char *name, ValueType value_type,
ValueObjectSP value_sp;
std::unique_lock<std::recursive_mutex> lock;
- ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+ Process::StopLocker stop_locker;
+ Executi...
[truncated]
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Prior to this patch, SBFrame/SBThread methods exhibit racy behavior if
called while the process is running, because they do not lock the
`Process::RetRunLock` mutex. If they did, they would fail, correctly
identifying that the process is not running.
Some methods _attempt_ to protect against this with the pattern:
```
ExecutionContext exe_ctx(m_opaque_sp.get(), lock); // this is a different lock
Process *process = exe_ctx.GetProcessPtr();
if (process) {
Process::StopLocker stop_locker;
if (stop_locker.TryLock(&process->GetRunLock()))
.... do work ...
```
However, this is also racy: the constructor of `ExecutionContext` will
access the frame list, which is something that can only be done once the
process is stopped.
With this patch:
1. The constructor of `ExecutionContext` now expects a `ProcessRunLock`
as an argument. It attempts to lock the run lock, and only fills in
information about frames and threads if the lock can be acquired.
Callers of the constructor are expected to check the lock.
2. All uses of ExecutionContext are adjusted to conform to the above.
3. The SBThread.cpp-defined helper function ResumeNewPlan now expects a
locked ProcessRunLock as _proof_ that the execution is stopped. It
will unlock the mutex prior to resuming the process.
This commit exposes many opportunities for early-returns, but these
would increase the diff of this patch and distract from the important
changes, so we opt not to do it here.
felipepiovezan
force-pushed
the
felipe/fix_racy_sbmethods
branch
from
301d8ac to
9be92f8
Compare
|
This linux test failed, but I'm not yet sure if it is related, as it doesn't fail locally for me: |
jimingham
reviewed
Aug 4, 2025
| std::unique_lock<std::recursive_mutex> &lock) | ||
| ExecutionContext::ExecutionContext( | ||
| const ExecutionContextRef *exe_ctx_ref_ptr, | ||
| std::unique_lock<std::recursive_mutex> &lock, |
There was a problem hiding this comment.
Now that there are two locks, you might want to give this one a more descriptive name?
jimingham
reviewed
Aug 4, 2025
| @@ -99,14 +99,15 @@ SBFrame::operator bool() const { | |||
| LLDB_INSTRUMENT_VA(this); | |||
|
|
|||
| std::unique_lock<std::recursive_mutex> lock; | |||
| ExecutionContext exe_ctx(m_opaque_sp.get(), lock); | |||
| Process::StopLocker stop_locker; | |||
There was a problem hiding this comment.
I'm not sure why this function didn't start off with:
if (!m_opaque_sp) return false;
But given we're doing more work now, it might be good to add that short-circuit.
JDevlieghere
reviewed
Aug 4, 2025
There was a problem hiding this comment.
It looks like this PR is updating all call sites of this ctor. We're always passing a unique_lock and the StopLocker. I understand they do different things and we need both, but if we always need both, should we abstract this away a bit more. Like a UniqueStopLocker or something, which also holds onto the unique_lock RAII style?
Comment on lines
+236
to
+237
| /// Return an SBValue with an error message warning that the process is not | ||
| /// currently stopped. |
There was a problem hiding this comment.
Nit: The "error message warning" is somewhat confusing. How about:
Suggested change
| /// Return an SBValue with an error message warning that the process is not | |
| /// currently stopped. | |
| /// Return an SBValue containing an error message that warns the process is not currently stopped. |
There was a problem hiding this comment.
At the SB API level, if you are getting the ExecutionContext from an ExecutionContextRef represented by a Target, Process, Thread or Frame you should also hold the API mutex. There are some cases where we don't actually require a process. For instance SBTarget::CreateValueFromAddress can work on either a live process that's stopped or to look at static data on a non-running target. But those seem like they are in the minority, and in that case, for instance, you could easily do:
if (!process || stop_locker.IsLocked())
so that wouldn't be particularly awkward.
I can't think of a good reason why you would want to drop the API lock before the stop lock or vice versa. There's the one place where Felipe swaps the stop_locker originally set for another, so long as this doesn't make that particularly awkward, this seems doable.
jimingham
requested changes
Aug 4, 2025
There was a problem hiding this comment.
This is pretty close.
In the SBThread modifications there were a bunch of places where you were returning empty SBError's or not filling in the error you were passed in with an informative message. We should fill in error's we're passed everywhere.
I think this would look nicer if the ExecutionContext constructor was:
ExecutionContext(ExecutionContextRef *, API_Locker, StopLocker, Status *status = nullptr)
Then in all the code that returns or is passed an SBError, you could just pass its opaque pointer to the constructor as well, and you wouldn't either be as likely to forget to do it or have to do it by hand in a whole bunch of places.
Other than that I just had a couple comments about comments.
| @@ -318,11 +319,13 @@ class ExecutionContext { | |||
| // These two variants take in a locker, and grab the target, lock the API | |||
There was a problem hiding this comment.
This comment isn't correct anymore.
You should mention that IF the ExecutionContextRef coming in has a process, it will try to lock the stop locker and won't fill anything in below the process. But if the exe_ctx_ref doesn't have a process, the stop_locker won't get locked. That's fairly obvious, but still worth making clear.
There was a problem hiding this comment.
It's also worth pointing out that IF the stop_locker is locked, the process and target have to be valid, so you don't need to check for that after checking the stop_locker...
| ExecutionContext exe_ctx(m_opaque_sp.get(), lock); | ||
| Process::StopLocker stop_locker; | ||
| ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker); | ||
| if (!stop_locker.IsLocked()) |
There was a problem hiding this comment.
I agree you shouldn't do code simplifications in this pass. But if the stop_locker is locked, then you must have a target and a process. Since this function, for instance, doesn't actually use the target & process pointers except to check for null, that code could be eliminated.
There was a problem hiding this comment.
That should be another cleanup pass...
| @@ -782,27 +809,26 @@ SBValueList SBFrame::GetVariables(const lldb::SBVariablesOptions &options) { | |||
|
|
|||
| SBValueList value_list; | |||
| std::unique_lock<std::recursive_mutex> lock; | |||
| ExecutionContext exe_ctx(m_opaque_sp.get(), lock); | |||
| Process::StopLocker stop_locker; | |||
There was a problem hiding this comment.
Huh, GetVariables didn't take the stop locker originally???
| ExecutionContext exe_ctx(m_opaque_sp.get(), lock); | ||
| Process::StopLocker stop_locker; | ||
| ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker); | ||
| if (stop_locker.IsLocked()) { |
There was a problem hiding this comment.
If you early returned here, there would actually be fewer differences, and it would match the pattern you've set up in all the other methods. I'd do that here.
There was a problem hiding this comment.
Oh, no I see. Lots more code would have to be outdented below if you did that. Okay, that's fine for a second pass. But it does look odd to have mixed early return and not here.
| : m_target_sp(), m_process_sp(), m_thread_sp(), m_frame_sp() { | ||
| if (exe_ctx_ref_ptr) { | ||
| m_target_sp = exe_ctx_ref_ptr->GetTargetSP(); | ||
| if (m_target_sp) { | ||
| lock = std::unique_lock<std::recursive_mutex>(m_target_sp->GetAPIMutex()); | ||
|
|
||
| m_process_sp = exe_ctx_ref_ptr->GetProcessSP(); | ||
| m_thread_sp = exe_ctx_ref_ptr->GetThreadSP(); | ||
| m_frame_sp = exe_ctx_ref_ptr->GetFrameSP(); | ||
| if (m_process_sp && stop_locker.TryLock(&m_process_sp->GetRunLock())) { |
There was a problem hiding this comment.
I think it would be useful to log (to the API log channel) the times that someone called in here and there WAS a process but it was not stopped. If you saw a flurry of those in an API log it would be a strong indication that the client was not doing the right thing.
| Process::StopLocker stop_locker; | ||
| ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker); | ||
| if (!stop_locker.IsLocked()) | ||
| return; |
There was a problem hiding this comment.
And this one.
Maybe we should make the ExecutionContext constructor take an optional Status * object, and if that's not empty we can put a standard error in it? Then you could just pass in these errors, and then you wouldn't have to set the error directly each time.
You could also use that for your SBValue error return; I'm pretty sure there's a way to make a ValueObject just from a Status.
| ExecutionContext exe_ctx(m_opaque_sp.get(), lock); | ||
| Process::StopLocker stop_locker; | ||
| ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker); | ||
| if (!stop_locker.IsLocked()) |
There was a problem hiding this comment.
This one also has an error.
| Process::StopLocker stop_locker; | ||
| ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker); | ||
| if (!stop_locker.IsLocked()) | ||
| return sb_error; |
There was a problem hiding this comment.
We shouldn't return an empty error when we know the error... Again, if the ExecutionContext constructor took an optional error, that would be simple to do.
| Process::StopLocker stop_locker; | ||
| ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker); | ||
| if (!stop_locker.IsLocked()) | ||
| return error; |
There was a problem hiding this comment.
Again, a better error here.
| Process::StopLocker stop_locker; | ||
| ExecutionContext exe_ctx(m_opaque_sp.get(), lock, stop_locker); | ||
| if (!stop_locker.IsLocked()) | ||
| return sb_error; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this patch, SBFrame/SBThread methods exhibit racy behavior if called while the process is running, because they do not lock the
Process::RetRunLockmutex. If they did, they would fail, correctly identifying that the process is not running.Some methods attempt to protect against this with the pattern:
However, this is also racy: the constructor of
ExecutionContextwill access the frame list, which is something that can only be done once the process is stopped.With this patch:
ExecutionContextnow expects aProcessRunLockas an argument. It attempts to lock the run lock, and only fills in information about frames and threads if the lock can be acquired. Callers of the constructor are expected to check the lock.This commit exposes many opportunities for early-returns, but these would increase the diff of this patch and distract from the important changes, so we opt not to do it here.