[Lldb-commits] [lldb] [lldb] Fix data race in statusline format handling (PR #142489)
Jonas Devlieghere via lldb-commits
lldb-commits at lists.llvm.org
Mon Jun 2 14:34:59 PDT 2025
https://github.com/JDevlieghere updated https://github.com/llvm/llvm-project/pull/142489
>From bbdaa75ba4baa54d17fb1f8bbf6280b7e4521b84 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Mon, 2 Jun 2025 14:16:23 -0700
Subject: [PATCH 1/2] [lldb] Fix data race in statusline format handling
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.
---
lldb/include/lldb/Core/Debugger.h | 12 +++----
lldb/include/lldb/Interpreter/OptionValue.h | 6 ++--
lldb/source/Core/Debugger.cpp | 40 ++++++++++-----------
lldb/source/Core/Disassembler.cpp | 17 +++------
lldb/source/Core/Statusline.cpp | 6 ++--
lldb/source/Interpreter/OptionValue.cpp | 6 ++--
lldb/source/Target/StackFrame.cpp | 4 +--
lldb/source/Target/Thread.cpp | 6 ++--
lldb/source/Target/ThreadPlanTracer.cpp | 4 +--
9 files changed, 45 insertions(+), 56 deletions(-)
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);
}
}
}
>From 7c6db59f141e811d28cab768c5d5f29c02e45877 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Mon, 2 Jun 2025 14:34:45 -0700
Subject: [PATCH 2/2] Address Adrian's feedback
---
lldb/source/Core/Debugger.cpp | 2 +-
lldb/source/Core/Statusline.cpp | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 112ef3572aa98..52362281a217f 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -1536,7 +1536,7 @@ bool Debugger::FormatDisassemblerAddress(const FormatEntity::Entry *format,
FormatEntity::Entry format_entry;
if (format)
format_entry = *format;
- else if (exe_ctx != nullptr && exe_ctx->HasTargetScope())
+ else if (exe_ctx && exe_ctx->HasTargetScope())
format_entry = exe_ctx->GetTargetRef().GetDebugger().GetDisassemblyFormat();
else
FormatEntity::Parse("${addr}: ", format_entry);
diff --git a/lldb/source/Core/Statusline.cpp b/lldb/source/Core/Statusline.cpp
index 4e6cc35033204..52f6134123b76 100644
--- a/lldb/source/Core/Statusline.cpp
+++ b/lldb/source/Core/Statusline.cpp
@@ -148,5 +148,5 @@ void Statusline::Redraw(bool update) {
FormatEntity::Format(format, stream, &symbol_ctx, &exe_ctx, nullptr, nullptr,
false, false);
- Draw(std::string(stream.GetString()));
+ Draw(stream.GetString().str());
}
More information about the lldb-commits
mailing list