[lldb] Fix data race in statusline format handling #142489
Merged
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: Jonas Devlieghere (JDevlieghere) ChangesThis fixes a data race between the main thread and the default event handler thread. The statusline format option value was protected by a mutex, but it was returned as a pointer, allowing one thread to access it while another was modifying it. Avoid the data race by returning format values by value instead of by pointer. Full diff: https://github.com/llvm/llvm-project/pull/142489.diff 9 Files Affected:
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index c9e5310cded1a..d73aba1e3ce58 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -243,17 +243,17 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
bool GetAutoConfirm() const;
- const FormatEntity::Entry *GetDisassemblyFormat() const;
+ FormatEntity::Entry GetDisassemblyFormat() const;
- const FormatEntity::Entry *GetFrameFormat() const;
+ FormatEntity::Entry GetFrameFormat() const;
- const FormatEntity::Entry *GetFrameFormatUnique() const;
+ FormatEntity::Entry GetFrameFormatUnique() const;
uint64_t GetStopDisassemblyMaxSize() const;
- const FormatEntity::Entry *GetThreadFormat() const;
+ FormatEntity::Entry GetThreadFormat() const;
- const FormatEntity::Entry *GetThreadStopFormat() const;
+ FormatEntity::Entry GetThreadStopFormat() const;
lldb::ScriptLanguage GetScriptLanguage() const;
@@ -297,7 +297,7 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
bool GetShowStatusline() const;
- const FormatEntity::Entry *GetStatuslineFormat() const;
+ FormatEntity::Entry GetStatuslineFormat() const;
bool SetStatuslineFormat(const FormatEntity::Entry &format);
llvm::StringRef GetSeparator() const;
diff --git a/lldb/include/lldb/Interpreter/OptionValue.h b/lldb/include/lldb/Interpreter/OptionValue.h
index e3c139155b0ef..69f84419416c6 100644
--- a/lldb/include/lldb/Interpreter/OptionValue.h
+++ b/lldb/include/lldb/Interpreter/OptionValue.h
@@ -284,6 +284,8 @@ class OptionValue {
return GetStringValue();
if constexpr (std::is_same_v<T, ArchSpec>)
return GetArchSpecValue();
+ if constexpr (std::is_same_v<T, FormatEntity::Entry>)
+ return GetFormatEntity();
if constexpr (std::is_enum_v<T>)
if (std::optional<int64_t> value = GetEnumerationValue())
return static_cast<T>(*value);
@@ -295,8 +297,6 @@ class OptionValue {
typename std::remove_pointer<T>::type>::type,
std::enable_if_t<std::is_pointer_v<T>, bool> = true>
T GetValueAs() const {
- if constexpr (std::is_same_v<U, FormatEntity::Entry>)
- return GetFormatEntity();
if constexpr (std::is_same_v<U, RegularExpression>)
return GetRegexValue();
return {};
@@ -382,7 +382,7 @@ class OptionValue {
std::optional<UUID> GetUUIDValue() const;
bool SetUUIDValue(const UUID &uuid);
- const FormatEntity::Entry *GetFormatEntity() const;
+ FormatEntity::Entry GetFormatEntity() const;
bool SetFormatEntityValue(const FormatEntity::Entry &entry);
const RegularExpression *GetRegexValue() const;
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 519a2528ca7e0..112ef3572aa98 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -294,19 +294,19 @@ bool Debugger::GetAutoConfirm() const {
idx, g_debugger_properties[idx].default_uint_value != 0);
}
-const FormatEntity::Entry *Debugger::GetDisassemblyFormat() const {
+FormatEntity::Entry Debugger::GetDisassemblyFormat() const {
constexpr uint32_t idx = ePropertyDisassemblyFormat;
- return GetPropertyAtIndexAs<const FormatEntity::Entry *>(idx);
+ return GetPropertyAtIndexAs<FormatEntity::Entry>(idx, {});
}
-const FormatEntity::Entry *Debugger::GetFrameFormat() const {
+FormatEntity::Entry Debugger::GetFrameFormat() const {
constexpr uint32_t idx = ePropertyFrameFormat;
- return GetPropertyAtIndexAs<const FormatEntity::Entry *>(idx);
+ return GetPropertyAtIndexAs<FormatEntity::Entry>(idx, {});
}
-const FormatEntity::Entry *Debugger::GetFrameFormatUnique() const {
+FormatEntity::Entry Debugger::GetFrameFormatUnique() const {
constexpr uint32_t idx = ePropertyFrameFormatUnique;
- return GetPropertyAtIndexAs<const FormatEntity::Entry *>(idx);
+ return GetPropertyAtIndexAs<FormatEntity::Entry>(idx, {});
}
uint64_t Debugger::GetStopDisassemblyMaxSize() const {
@@ -350,14 +350,14 @@ void Debugger::SetPrompt(llvm::StringRef p) {
GetCommandInterpreter().UpdatePrompt(new_prompt);
}
-const FormatEntity::Entry *Debugger::GetThreadFormat() const {
+FormatEntity::Entry Debugger::GetThreadFormat() const {
constexpr uint32_t idx = ePropertyThreadFormat;
- return GetPropertyAtIndexAs<const FormatEntity::Entry *>(idx);
+ return GetPropertyAtIndexAs<FormatEntity::Entry>(idx, {});
}
-const FormatEntity::Entry *Debugger::GetThreadStopFormat() const {
+FormatEntity::Entry Debugger::GetThreadStopFormat() const {
constexpr uint32_t idx = ePropertyThreadStopFormat;
- return GetPropertyAtIndexAs<const FormatEntity::Entry *>(idx);
+ return GetPropertyAtIndexAs<FormatEntity::Entry>(idx, {});
}
lldb::ScriptLanguage Debugger::GetScriptLanguage() const {
@@ -492,9 +492,9 @@ bool Debugger::GetShowStatusline() const {
idx, g_debugger_properties[idx].default_uint_value != 0);
}
-const FormatEntity::Entry *Debugger::GetStatuslineFormat() const {
+FormatEntity::Entry Debugger::GetStatuslineFormat() const {
constexpr uint32_t idx = ePropertyStatuslineFormat;
- return GetPropertyAtIndexAs<const FormatEntity::Entry *>(idx);
+ return GetPropertyAtIndexAs<FormatEntity::Entry>(idx, {});
}
bool Debugger::SetStatuslineFormat(const FormatEntity::Entry &format) {
@@ -1534,15 +1534,13 @@ bool Debugger::FormatDisassemblerAddress(const FormatEntity::Entry *format,
const ExecutionContext *exe_ctx,
const Address *addr, Stream &s) {
FormatEntity::Entry format_entry;
+ if (format)
+ format_entry = *format;
+ else if (exe_ctx != nullptr && exe_ctx->HasTargetScope())
+ format_entry = exe_ctx->GetTargetRef().GetDebugger().GetDisassemblyFormat();
+ else
+ FormatEntity::Parse("${addr}: ", format_entry);
- if (format == nullptr) {
- if (exe_ctx != nullptr && exe_ctx->HasTargetScope())
- format = exe_ctx->GetTargetRef().GetDebugger().GetDisassemblyFormat();
- if (format == nullptr) {
- FormatEntity::Parse("${addr}: ", format_entry);
- format = &format_entry;
- }
- }
bool function_changed = false;
bool initial_function = false;
if (prev_sc && (prev_sc->function || prev_sc->symbol)) {
@@ -1566,7 +1564,7 @@ bool Debugger::FormatDisassemblerAddress(const FormatEntity::Entry *format,
(prev_sc->function == nullptr && prev_sc->symbol == nullptr)) {
initial_function = true;
}
- return FormatEntity::Format(*format, s, sc, exe_ctx, addr, nullptr,
+ return FormatEntity::Format(format_entry, s, sc, exe_ctx, addr, nullptr,
function_changed, initial_function);
}
diff --git a/lldb/source/Core/Disassembler.cpp b/lldb/source/Core/Disassembler.cpp
index dce3d59457c0e..7c1fc8fa1934d 100644
--- a/lldb/source/Core/Disassembler.cpp
+++ b/lldb/source/Core/Disassembler.cpp
@@ -307,14 +307,11 @@ void Disassembler::PrintInstructions(Debugger &debugger, const ArchSpec &arch,
eSymbolContextLineEntry | eSymbolContextFunction | eSymbolContextSymbol;
const bool use_inline_block_range = false;
- const FormatEntity::Entry *disassembly_format = nullptr;
FormatEntity::Entry format;
if (exe_ctx.HasTargetScope()) {
- disassembly_format =
- exe_ctx.GetTargetRef().GetDebugger().GetDisassemblyFormat();
+ format = exe_ctx.GetTargetRef().GetDebugger().GetDisassemblyFormat();
} else {
FormatEntity::Parse("${addr}: ", format);
- disassembly_format = &format;
}
// First pass: step through the list of instructions, find how long the
@@ -342,8 +339,8 @@ void Disassembler::PrintInstructions(Debugger &debugger, const ArchSpec &arch,
module_sp->ResolveSymbolContextForAddress(addr, resolve_mask, sc);
if (resolved_mask) {
StreamString strmstr;
- Debugger::FormatDisassemblerAddress(disassembly_format, &sc, nullptr,
- &exe_ctx, &addr, strmstr);
+ Debugger::FormatDisassemblerAddress(&format, &sc, nullptr, &exe_ctx,
+ &addr, strmstr);
size_t cur_line = strmstr.GetSizeOfLastLine();
if (cur_line > address_text_size)
address_text_size = cur_line;
@@ -1034,14 +1031,11 @@ void InstructionList::Dump(Stream *s, bool show_address, bool show_bytes,
const uint32_t max_opcode_byte_size = GetMaxOpcocdeByteSize();
collection::const_iterator pos, begin, end;
- const FormatEntity::Entry *disassembly_format = nullptr;
FormatEntity::Entry format;
if (exe_ctx && exe_ctx->HasTargetScope()) {
- disassembly_format =
- exe_ctx->GetTargetRef().GetDebugger().GetDisassemblyFormat();
+ format = exe_ctx->GetTargetRef().GetDebugger().GetDisassemblyFormat();
} else {
FormatEntity::Parse("${addr}: ", format);
- disassembly_format = &format;
}
for (begin = m_instructions.begin(), end = m_instructions.end(), pos = begin;
@@ -1049,8 +1043,7 @@ void InstructionList::Dump(Stream *s, bool show_address, bool show_bytes,
if (pos != begin)
s->EOL();
(*pos)->Dump(s, max_opcode_byte_size, show_address, show_bytes,
- show_control_flow_kind, exe_ctx, nullptr, nullptr,
- disassembly_format, 0);
+ show_control_flow_kind, exe_ctx, nullptr, nullptr, &format, 0);
}
}
diff --git a/lldb/source/Core/Statusline.cpp b/lldb/source/Core/Statusline.cpp
index 8b3c8d1ccfa80..4e6cc35033204 100644
--- a/lldb/source/Core/Statusline.cpp
+++ b/lldb/source/Core/Statusline.cpp
@@ -144,9 +144,9 @@ void Statusline::Redraw(bool update) {
}
StreamString stream;
- if (auto *format = m_debugger.GetStatuslineFormat())
- FormatEntity::Format(*format, stream, &symbol_ctx, &exe_ctx, nullptr,
- nullptr, false, false);
+ FormatEntity::Entry format = m_debugger.GetStatuslineFormat();
+ FormatEntity::Format(format, stream, &symbol_ctx, &exe_ctx, nullptr, nullptr,
+ false, false);
Draw(std::string(stream.GetString()));
}
diff --git a/lldb/source/Interpreter/OptionValue.cpp b/lldb/source/Interpreter/OptionValue.cpp
index 28bc57a07ac71..245bc7c83e3f9 100644
--- a/lldb/source/Interpreter/OptionValue.cpp
+++ b/lldb/source/Interpreter/OptionValue.cpp
@@ -380,11 +380,11 @@ bool OptionValue::SetLanguageValue(lldb::LanguageType new_language) {
return false;
}
-const FormatEntity::Entry *OptionValue::GetFormatEntity() const {
+FormatEntity::Entry OptionValue::GetFormatEntity() const {
std::lock_guard<std::mutex> lock(m_mutex);
if (const OptionValueFormatEntity *option_value = GetAsFormatEntity())
- return &option_value->GetCurrentValue();
- return nullptr;
+ return option_value->GetCurrentValue();
+ return {};
}
const RegularExpression *OptionValue::GetRegexValue() const {
diff --git a/lldb/source/Target/StackFrame.cpp b/lldb/source/Target/StackFrame.cpp
index ab5cd0b27c789..fa041d11061ca 100644
--- a/lldb/source/Target/StackFrame.cpp
+++ b/lldb/source/Target/StackFrame.cpp
@@ -1936,7 +1936,7 @@ void StackFrame::DumpUsingSettingsFormat(Stream *strm, bool show_unique,
ExecutionContext exe_ctx(shared_from_this());
- const FormatEntity::Entry *frame_format = nullptr;
+ FormatEntity::Entry frame_format;
Target *target = exe_ctx.GetTargetPtr();
if (target) {
if (show_unique) {
@@ -1945,7 +1945,7 @@ void StackFrame::DumpUsingSettingsFormat(Stream *strm, bool show_unique,
frame_format = target->GetDebugger().GetFrameFormat();
}
}
- if (!DumpUsingFormat(*strm, frame_format, frame_marker)) {
+ if (!DumpUsingFormat(*strm, &frame_format, frame_marker)) {
Dump(strm, true, false);
strm->EOL();
}
diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp
index b0e0f1e67c060..8840ba68e28e6 100644
--- a/lldb/source/Target/Thread.cpp
+++ b/lldb/source/Target/Thread.cpp
@@ -1667,15 +1667,13 @@ void Thread::DumpUsingSettingsFormat(Stream &strm, uint32_t frame_idx,
bool stop_format) {
ExecutionContext exe_ctx(shared_from_this());
- const FormatEntity::Entry *thread_format;
+ FormatEntity::Entry thread_format;
if (stop_format)
thread_format = exe_ctx.GetTargetRef().GetDebugger().GetThreadStopFormat();
else
thread_format = exe_ctx.GetTargetRef().GetDebugger().GetThreadFormat();
- assert(thread_format);
-
- DumpUsingFormat(strm, frame_idx, thread_format);
+ DumpUsingFormat(strm, frame_idx, &thread_format);
}
void Thread::SettingsInitialize() {}
diff --git a/lldb/source/Target/ThreadPlanTracer.cpp b/lldb/source/Target/ThreadPlanTracer.cpp
index ab63cc7f6c223..c5a9c5b806c30 100644
--- a/lldb/source/Target/ThreadPlanTracer.cpp
+++ b/lldb/source/Target/ThreadPlanTracer.cpp
@@ -174,11 +174,11 @@ void ThreadPlanAssemblyTracer::Log() {
const bool show_control_flow_kind = true;
Instruction *instruction =
instruction_list.GetInstructionAtIndex(0).get();
- const FormatEntity::Entry *disassemble_format =
+ FormatEntity::Entry disassemble_format =
m_process.GetTarget().GetDebugger().GetDisassemblyFormat();
instruction->Dump(stream_sp.get(), max_opcode_byte_size, show_address,
show_bytes, show_control_flow_kind, nullptr, nullptr,
- nullptr, disassemble_format, 0);
+ nullptr, &disassemble_format, 0);
}
}
}
|
adrian-prantl
approved these changes
Jun 2, 2025
lldb/source/Core/Debugger.cpp
Outdated
| @@ -1534,15 +1534,13 @@ bool Debugger::FormatDisassemblerAddress(const FormatEntity::Entry *format, | |||
| const ExecutionContext *exe_ctx, | |||
| const Address *addr, Stream &s) { | |||
| FormatEntity::Entry format_entry; | |||
| if (format) | |||
There was a problem hiding this comment.
This is a weird API.
| std::lock_guard<std::mutex> lock(m_mutex); | ||
| if (const OptionValueFormatEntity *option_value = GetAsFormatEntity()) | ||
| return &option_value->GetCurrentValue(); | ||
| return nullptr; | ||
| return option_value->GetCurrentValue(); |
This fixes a data race between the main thread and the default event handler thread. The statusline format option value was protected by a mutex, but it was returned as a pointer, allowing one thread to access it while another was modifying it. Avoid the data race by returning format values by value instead of by pointer.
JDevlieghere
force-pushed
the
statusline-format-data-race
branch
from
8525db1 to
6076f77
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Jun 4, 2025
This fixes a data race between the main thread and the default event handler thread. The statusline format option value was protected by a mutex, but it was returned as a pointer, allowing one thread to access it while another was modifying it. Avoid the data race by returning format values by value instead of by pointer. (cherry picked from commit 6760857)
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.
This fixes a data race between the main thread and the default event handler thread. The statusline format option value was protected by a mutex, but it was returned as a pointer, allowing one thread to access it while another was modifying it.
Avoid the data race by returning format values by value instead of by pointer.