Skip to content

Commit 6760857

Browse files
JDevlieghereJDevlieghere
authored
[lldb] Fix data race in statusline format handling (llvm#142489)
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.
1 parent 437b160 commit 6760857

File tree

15 files changed

+69
-59
lines changed

15 files changed

+69
-59
lines changed

‎lldb/include/lldb/Core/Debugger.h

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -243,17 +243,17 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
243243

244244
bool GetAutoConfirm() const;
245245

246-
const FormatEntity::Entry *GetDisassemblyFormat() const;
246+
FormatEntity::Entry GetDisassemblyFormat() const;
247247

248-
const FormatEntity::Entry *GetFrameFormat() const;
248+
FormatEntity::Entry GetFrameFormat() const;
249249

250-
const FormatEntity::Entry *GetFrameFormatUnique() const;
250+
FormatEntity::Entry GetFrameFormatUnique() const;
251251

252252
uint64_t GetStopDisassemblyMaxSize() const;
253253

254-
const FormatEntity::Entry *GetThreadFormat() const;
254+
FormatEntity::Entry GetThreadFormat() const;
255255

256-
const FormatEntity::Entry *GetThreadStopFormat() const;
256+
FormatEntity::Entry GetThreadStopFormat() const;
257257

258258
lldb::ScriptLanguage GetScriptLanguage() const;
259259

@@ -297,7 +297,7 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
297297

298298
bool GetShowStatusline() const;
299299

300-
const FormatEntity::Entry *GetStatuslineFormat() const;
300+
FormatEntity::Entry GetStatuslineFormat() const;
301301
bool SetStatuslineFormat(const FormatEntity::Entry &format);
302302

303303
llvm::StringRef GetSeparator() const;

‎lldb/include/lldb/Core/FormatEntity.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,8 @@ struct Entry {
205205
return true;
206206
}
207207

208+
operator bool() const { return type != Type::Invalid; }
209+
208210
std::vector<Entry> &GetChildren();
209211

210212
std::string string;
@@ -217,7 +219,7 @@ struct Entry {
217219
size_t level = 0;
218220
/// @}
219221

220-
Type type;
222+
Type type = Type::Invalid;
221223
lldb::Format fmt = lldb::eFormatDefault;
222224
lldb::addr_t number = 0;
223225
bool deref = false;

‎lldb/include/lldb/Interpreter/OptionValue.h

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,8 @@ class OptionValue {
284284
return GetStringValue();
285285
if constexpr (std::is_same_v<T, ArchSpec>)
286286
return GetArchSpecValue();
287+
if constexpr (std::is_same_v<T, FormatEntity::Entry>)
288+
return GetFormatEntityValue();
287289
if constexpr (std::is_enum_v<T>)
288290
if (std::optional<int64_t> value = GetEnumerationValue())
289291
return static_cast<T>(*value);
@@ -295,11 +297,9 @@ class OptionValue {
295297
typename std::remove_pointer<T>::type>::type,
296298
std::enable_if_t<std::is_pointer_v<T>, bool> = true>
297299
T GetValueAs() const {
298-
if constexpr (std::is_same_v<U, FormatEntity::Entry>)
299-
return GetFormatEntity();
300-
if constexpr (std::is_same_v<U, RegularExpression>)
301-
return GetRegexValue();
302-
return {};
300+
static_assert(std::is_same_v<U, RegularExpression>,
301+
"only for RegularExpression");
302+
return GetRegexValue();
303303
}
304304

305305
bool SetValueAs(bool v) { return SetBooleanValue(v); }
@@ -382,7 +382,7 @@ class OptionValue {
382382
std::optional<UUID> GetUUIDValue() const;
383383
bool SetUUIDValue(const UUID &uuid);
384384

385-
const FormatEntity::Entry *GetFormatEntity() const;
385+
FormatEntity::Entry GetFormatEntityValue() const;
386386
bool SetFormatEntityValue(const FormatEntity::Entry &entry);
387387

388388
const RegularExpression *GetRegexValue() const;

‎lldb/include/lldb/Target/Language.h

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -495,9 +495,7 @@ class Language : public PluginInterface {
495495
/// Python uses \b except. Defaults to \b catch.
496496
virtual llvm::StringRef GetCatchKeyword() const { return "catch"; }
497497

498-
virtual const FormatEntity::Entry *GetFunctionNameFormat() const {
499-
return nullptr;
500-
}
498+
virtual FormatEntity::Entry GetFunctionNameFormat() const { return {}; }
501499

502500
protected:
503501
// Classes that inherit from Language can see and modify these

‎lldb/source/Core/Debugger.cpp

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -294,19 +294,19 @@ bool Debugger::GetAutoConfirm() const {
294294
idx, g_debugger_properties[idx].default_uint_value != 0);
295295
}
296296

297-
const FormatEntity::Entry *Debugger::GetDisassemblyFormat() const {
297+
FormatEntity::Entry Debugger::GetDisassemblyFormat() const {
298298
constexpr uint32_t idx = ePropertyDisassemblyFormat;
299-
return GetPropertyAtIndexAs<const FormatEntity::Entry *>(idx);
299+
return GetPropertyAtIndexAs<FormatEntity::Entry>(idx, {});
300300
}
301301

302-
const FormatEntity::Entry *Debugger::GetFrameFormat() const {
302+
FormatEntity::Entry Debugger::GetFrameFormat() const {
303303
constexpr uint32_t idx = ePropertyFrameFormat;
304-
return GetPropertyAtIndexAs<const FormatEntity::Entry *>(idx);
304+
return GetPropertyAtIndexAs<FormatEntity::Entry>(idx, {});
305305
}
306306

307-
const FormatEntity::Entry *Debugger::GetFrameFormatUnique() const {
307+
FormatEntity::Entry Debugger::GetFrameFormatUnique() const {
308308
constexpr uint32_t idx = ePropertyFrameFormatUnique;
309-
return GetPropertyAtIndexAs<const FormatEntity::Entry *>(idx);
309+
return GetPropertyAtIndexAs<FormatEntity::Entry>(idx, {});
310310
}
311311

312312
uint64_t Debugger::GetStopDisassemblyMaxSize() const {
@@ -350,14 +350,14 @@ void Debugger::SetPrompt(llvm::StringRef p) {
350350
GetCommandInterpreter().UpdatePrompt(new_prompt);
351351
}
352352

353-
const FormatEntity::Entry *Debugger::GetThreadFormat() const {
353+
FormatEntity::Entry Debugger::GetThreadFormat() const {
354354
constexpr uint32_t idx = ePropertyThreadFormat;
355-
return GetPropertyAtIndexAs<const FormatEntity::Entry *>(idx);
355+
return GetPropertyAtIndexAs<FormatEntity::Entry>(idx, {});
356356
}
357357

358-
const FormatEntity::Entry *Debugger::GetThreadStopFormat() const {
358+
FormatEntity::Entry Debugger::GetThreadStopFormat() const {
359359
constexpr uint32_t idx = ePropertyThreadStopFormat;
360-
return GetPropertyAtIndexAs<const FormatEntity::Entry *>(idx);
360+
return GetPropertyAtIndexAs<FormatEntity::Entry>(idx, {});
361361
}
362362

363363
lldb::ScriptLanguage Debugger::GetScriptLanguage() const {
@@ -492,9 +492,9 @@ bool Debugger::GetShowStatusline() const {
492492
idx, g_debugger_properties[idx].default_uint_value != 0);
493493
}
494494

495-
const FormatEntity::Entry *Debugger::GetStatuslineFormat() const {
495+
FormatEntity::Entry Debugger::GetStatuslineFormat() const {
496496
constexpr uint32_t idx = ePropertyStatuslineFormat;
497-
return GetPropertyAtIndexAs<const FormatEntity::Entry *>(idx);
497+
return GetPropertyAtIndexAs<FormatEntity::Entry>(idx, {});
498498
}
499499

500500
bool Debugger::SetStatuslineFormat(const FormatEntity::Entry &format) {
@@ -1536,8 +1536,11 @@ bool Debugger::FormatDisassemblerAddress(const FormatEntity::Entry *format,
15361536
FormatEntity::Entry format_entry;
15371537

15381538
if (format == nullptr) {
1539-
if (exe_ctx != nullptr && exe_ctx->HasTargetScope())
1540-
format = exe_ctx->GetTargetRef().GetDebugger().GetDisassemblyFormat();
1539+
if (exe_ctx != nullptr && exe_ctx->HasTargetScope()) {
1540+
format_entry =
1541+
exe_ctx->GetTargetRef().GetDebugger().GetDisassemblyFormat();
1542+
format = &format_entry;
1543+
}
15411544
if (format == nullptr) {
15421545
FormatEntity::Parse("${addr}: ", format_entry);
15431546
format = &format_entry;

‎lldb/source/Core/Disassembler.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -310,8 +310,8 @@ void Disassembler::PrintInstructions(Debugger &debugger, const ArchSpec &arch,
310310
const FormatEntity::Entry *disassembly_format = nullptr;
311311
FormatEntity::Entry format;
312312
if (exe_ctx.HasTargetScope()) {
313-
disassembly_format =
314-
exe_ctx.GetTargetRef().GetDebugger().GetDisassemblyFormat();
313+
format = exe_ctx.GetTargetRef().GetDebugger().GetDisassemblyFormat();
314+
disassembly_format = &format;
315315
} else {
316316
FormatEntity::Parse("${addr}: ", format);
317317
disassembly_format = &format;
@@ -1037,8 +1037,8 @@ void InstructionList::Dump(Stream *s, bool show_address, bool show_bytes,
10371037
const FormatEntity::Entry *disassembly_format = nullptr;
10381038
FormatEntity::Entry format;
10391039
if (exe_ctx && exe_ctx->HasTargetScope()) {
1040-
disassembly_format =
1041-
exe_ctx->GetTargetRef().GetDebugger().GetDisassemblyFormat();
1040+
format = exe_ctx->GetTargetRef().GetDebugger().GetDisassemblyFormat();
1041+
disassembly_format = &format;
10421042
} else {
10431043
FormatEntity::Parse("${addr}: ", format);
10441044
disassembly_format = &format;

‎lldb/source/Core/FormatEntity.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1279,13 +1279,13 @@ static bool FormatFunctionNameForLanguage(Stream &s,
12791279
if (!language_plugin)
12801280
return false;
12811281

1282-
const auto *format = language_plugin->GetFunctionNameFormat();
1282+
FormatEntity::Entry format = language_plugin->GetFunctionNameFormat();
12831283
if (!format)
12841284
return false;
12851285

12861286
StreamString name_stream;
12871287
const bool success =
1288-
FormatEntity::Format(*format, name_stream, sc, exe_ctx, /*addr=*/nullptr,
1288+
FormatEntity::Format(format, name_stream, sc, exe_ctx, /*addr=*/nullptr,
12891289
/*valobj=*/nullptr, /*function_changed=*/false,
12901290
/*initial_function=*/false);
12911291
if (success)

‎lldb/source/Core/Statusline.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -144,9 +144,9 @@ void Statusline::Redraw(bool update) {
144144
}
145145

146146
StreamString stream;
147-
if (auto *format = m_debugger.GetStatuslineFormat())
148-
FormatEntity::Format(*format, stream, &symbol_ctx, &exe_ctx, nullptr,
149-
nullptr, false, false);
147+
FormatEntity::Entry format = m_debugger.GetStatuslineFormat();
148+
FormatEntity::Format(format, stream, &symbol_ctx, &exe_ctx, nullptr, nullptr,
149+
false, false);
150150

151-
Draw(std::string(stream.GetString()));
151+
Draw(stream.GetString().str());
152152
}

‎lldb/source/Interpreter/OptionValue.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -380,11 +380,11 @@ bool OptionValue::SetLanguageValue(lldb::LanguageType new_language) {
380380
return false;
381381
}
382382

383-
const FormatEntity::Entry *OptionValue::GetFormatEntity() const {
383+
FormatEntity::Entry OptionValue::GetFormatEntityValue() const {
384384
std::lock_guard<std::mutex> lock(m_mutex);
385385
if (const OptionValueFormatEntity *option_value = GetAsFormatEntity())
386-
return &option_value->GetCurrentValue();
387-
return nullptr;
386+
return option_value->GetCurrentValue();
387+
return {};
388388
}
389389

390390
const RegularExpression *OptionValue::GetRegexValue() const {

‎lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2066,9 +2066,9 @@ class PluginProperties : public Properties {
20662066
m_collection_sp->Initialize(g_language_cplusplus_properties);
20672067
}
20682068

2069-
const FormatEntity::Entry *GetFunctionNameFormat() const {
2070-
return GetPropertyAtIndexAs<const FormatEntity::Entry *>(
2071-
ePropertyFunctionNameFormat);
2069+
FormatEntity::Entry GetFunctionNameFormat() const {
2070+
return GetPropertyAtIndexAs<FormatEntity::Entry>(
2071+
ePropertyFunctionNameFormat, {});
20722072
}
20732073
};
20742074
} // namespace
@@ -2078,7 +2078,7 @@ static PluginProperties &GetGlobalPluginProperties() {
20782078
return g_settings;
20792079
}
20802080

2081-
const FormatEntity::Entry *CPlusPlusLanguage::GetFunctionNameFormat() const {
2081+
FormatEntity::Entry CPlusPlusLanguage::GetFunctionNameFormat() const {
20822082
return GetGlobalPluginProperties().GetFunctionNameFormat();
20832083
}
20842084

‎lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,8 @@ class CPlusPlusLanguage : public Language {
9393
static llvm::StringRef GetPluginNameStatic() { return "cplusplus"; }
9494

9595
bool SymbolNameFitsToLanguage(Mangled mangled) const override;
96-
97-
bool DemangledNameContainsPath(llvm::StringRef path,
96+
97+
bool DemangledNameContainsPath(llvm::StringRef path,
9898
ConstString demangled) const override;
9999

100100
ConstString
@@ -136,7 +136,7 @@ class CPlusPlusLanguage : public Language {
136136

137137
llvm::StringRef GetInstanceVariableName() override { return "this"; }
138138

139-
const FormatEntity::Entry *GetFunctionNameFormat() const override;
139+
FormatEntity::Entry GetFunctionNameFormat() const override;
140140

141141
// PluginInterface protocol
142142
llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }

‎lldb/source/Target/StackFrame.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1937,12 +1937,15 @@ void StackFrame::DumpUsingSettingsFormat(Stream *strm, bool show_unique,
19371937
ExecutionContext exe_ctx(shared_from_this());
19381938

19391939
const FormatEntity::Entry *frame_format = nullptr;
1940+
FormatEntity::Entry format_entry;
19401941
Target *target = exe_ctx.GetTargetPtr();
19411942
if (target) {
19421943
if (show_unique) {
1943-
frame_format = target->GetDebugger().GetFrameFormatUnique();
1944+
format_entry = target->GetDebugger().GetFrameFormatUnique();
1945+
frame_format = &format_entry;
19441946
} else {
1945-
frame_format = target->GetDebugger().GetFrameFormat();
1947+
format_entry = target->GetDebugger().GetFrameFormat();
1948+
frame_format = &format_entry;
19461949
}
19471950
}
19481951
if (!DumpUsingFormat(*strm, frame_format, frame_marker)) {

‎lldb/source/Target/Thread.cpp

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1668,10 +1668,14 @@ void Thread::DumpUsingSettingsFormat(Stream &strm, uint32_t frame_idx,
16681668
ExecutionContext exe_ctx(shared_from_this());
16691669

16701670
const FormatEntity::Entry *thread_format;
1671-
if (stop_format)
1672-
thread_format = exe_ctx.GetTargetRef().GetDebugger().GetThreadStopFormat();
1673-
else
1674-
thread_format = exe_ctx.GetTargetRef().GetDebugger().GetThreadFormat();
1671+
FormatEntity::Entry format_entry;
1672+
if (stop_format) {
1673+
format_entry = exe_ctx.GetTargetRef().GetDebugger().GetThreadStopFormat();
1674+
thread_format = &format_entry;
1675+
} else {
1676+
format_entry = exe_ctx.GetTargetRef().GetDebugger().GetThreadFormat();
1677+
thread_format = &format_entry;
1678+
}
16751679

16761680
assert(thread_format);
16771681

‎lldb/source/Target/ThreadPlanTracer.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -174,11 +174,11 @@ void ThreadPlanAssemblyTracer::Log() {
174174
const bool show_control_flow_kind = true;
175175
Instruction *instruction =
176176
instruction_list.GetInstructionAtIndex(0).get();
177-
const FormatEntity::Entry *disassemble_format =
177+
FormatEntity::Entry disassemble_format =
178178
m_process.GetTarget().GetDebugger().GetDisassemblyFormat();
179179
instruction->Dump(stream_sp.get(), max_opcode_byte_size, show_address,
180180
show_bytes, show_control_flow_kind, nullptr, nullptr,
181-
nullptr, disassemble_format, 0);
181+
nullptr, &disassemble_format, 0);
182182
}
183183
}
184184
}

‎lldb/unittests/Core/DebuggerTest.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ TEST_F(DebuggerTest, TestSettings) {
4646

4747
FormatEntity::Entry format("foo");
4848
EXPECT_TRUE(debugger_sp->SetStatuslineFormat(format));
49-
EXPECT_EQ(debugger_sp->GetStatuslineFormat()->string, "foo");
49+
EXPECT_EQ(debugger_sp->GetStatuslineFormat().string, "foo");
5050

5151
Debugger::Destroy(debugger_sp);
5252
}

0 commit comments

Comments
 (0)