llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/142489.diff 9 Files Affected: - (modified) lldb/include/lldb/Core/Debugger.h (+6-6) - (modified) lldb/include/lldb/Interpreter/OptionValue.h (+3-3) - (modified) lldb/source/Core/Debugger.cpp (+19-21) - (modified) lldb/source/Core/Disassembler.cpp (+5-12) - (modified) lldb/source/Core/Statusline.cpp (+3-3) - (modified) lldb/source/Interpreter/OptionValue.cpp (+3-3) - (modified) lldb/source/Target/StackFrame.cpp (+2-2) - (modified) lldb/source/Target/Thread.cpp (+2-4) - (modified) lldb/source/Target/ThreadPlanTracer.cpp (+2-2) ``````````diff 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); } } } `````````` </details> https://github.com/llvm/llvm-project/pull/142489 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
