Skip to content

Commit bbdaa75

Browse files
JDevlieghereJDevlieghere
committed
[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.
1 parent 832a7bb commit bbdaa75

File tree

9 files changed

+45
-56
lines changed

9 files changed

+45
-56
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/Interpreter/OptionValue.h

Lines changed: 3 additions & 3 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 GetFormatEntity();
287289
if constexpr (std::is_enum_v<T>)
288290
if (std::optional<int64_t> value = GetEnumerationValue())
289291
return static_cast<T>(*value);
@@ -295,8 +297,6 @@ 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();
300300
if constexpr (std::is_same_v<U, RegularExpression>)
301301
return GetRegexValue();
302302
return {};
@@ -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 GetFormatEntity() const;
386386
bool SetFormatEntityValue(const FormatEntity::Entry &entry);
387387

388388
const RegularExpression *GetRegexValue() const;

‎lldb/source/Core/Debugger.cpp

Lines changed: 19 additions & 21 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) {
@@ -1534,15 +1534,13 @@ bool Debugger::FormatDisassemblerAddress(const FormatEntity::Entry *format,
15341534
const ExecutionContext *exe_ctx,
15351535
const Address *addr, Stream &s) {
15361536
FormatEntity::Entry format_entry;
1537+
if (format)
1538+
format_entry = *format;
1539+
else if (exe_ctx != nullptr && exe_ctx->HasTargetScope())
1540+
format_entry = exe_ctx->GetTargetRef().GetDebugger().GetDisassemblyFormat();
1541+
else
1542+
FormatEntity::Parse("${addr}: ", format_entry);
15371543

1538-
if (format == nullptr) {
1539-
if (exe_ctx != nullptr && exe_ctx->HasTargetScope())
1540-
format = exe_ctx->GetTargetRef().GetDebugger().GetDisassemblyFormat();
1541-
if (format == nullptr) {
1542-
FormatEntity::Parse("${addr}: ", format_entry);
1543-
format = &format_entry;
1544-
}
1545-
}
15461544
bool function_changed = false;
15471545
bool initial_function = false;
15481546
if (prev_sc && (prev_sc->function || prev_sc->symbol)) {
@@ -1566,7 +1564,7 @@ bool Debugger::FormatDisassemblerAddress(const FormatEntity::Entry *format,
15661564
(prev_sc->function == nullptr && prev_sc->symbol == nullptr)) {
15671565
initial_function = true;
15681566
}
1569-
return FormatEntity::Format(*format, s, sc, exe_ctx, addr, nullptr,
1567+
return FormatEntity::Format(format_entry, s, sc, exe_ctx, addr, nullptr,
15701568
function_changed, initial_function);
15711569
}
15721570

‎lldb/source/Core/Disassembler.cpp

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -307,14 +307,11 @@ void Disassembler::PrintInstructions(Debugger &debugger, const ArchSpec &arch,
307307
eSymbolContextLineEntry | eSymbolContextFunction | eSymbolContextSymbol;
308308
const bool use_inline_block_range = false;
309309

310-
const FormatEntity::Entry *disassembly_format = nullptr;
311310
FormatEntity::Entry format;
312311
if (exe_ctx.HasTargetScope()) {
313-
disassembly_format =
314-
exe_ctx.GetTargetRef().GetDebugger().GetDisassemblyFormat();
312+
format = exe_ctx.GetTargetRef().GetDebugger().GetDisassemblyFormat();
315313
} else {
316314
FormatEntity::Parse("${addr}: ", format);
317-
disassembly_format = &format;
318315
}
319316

320317
// First pass: step through the list of instructions, find how long the
@@ -342,8 +339,8 @@ void Disassembler::PrintInstructions(Debugger &debugger, const ArchSpec &arch,
342339
module_sp->ResolveSymbolContextForAddress(addr, resolve_mask, sc);
343340
if (resolved_mask) {
344341
StreamString strmstr;
345-
Debugger::FormatDisassemblerAddress(disassembly_format, &sc, nullptr,
346-
&exe_ctx, &addr, strmstr);
342+
Debugger::FormatDisassemblerAddress(&format, &sc, nullptr, &exe_ctx,
343+
&addr, strmstr);
347344
size_t cur_line = strmstr.GetSizeOfLastLine();
348345
if (cur_line > address_text_size)
349346
address_text_size = cur_line;
@@ -1034,23 +1031,19 @@ void InstructionList::Dump(Stream *s, bool show_address, bool show_bytes,
10341031
const uint32_t max_opcode_byte_size = GetMaxOpcocdeByteSize();
10351032
collection::const_iterator pos, begin, end;
10361033

1037-
const FormatEntity::Entry *disassembly_format = nullptr;
10381034
FormatEntity::Entry format;
10391035
if (exe_ctx && exe_ctx->HasTargetScope()) {
1040-
disassembly_format =
1041-
exe_ctx->GetTargetRef().GetDebugger().GetDisassemblyFormat();
1036+
format = exe_ctx->GetTargetRef().GetDebugger().GetDisassemblyFormat();
10421037
} else {
10431038
FormatEntity::Parse("${addr}: ", format);
1044-
disassembly_format = &format;
10451039
}
10461040

10471041
for (begin = m_instructions.begin(), end = m_instructions.end(), pos = begin;
10481042
pos != end; ++pos) {
10491043
if (pos != begin)
10501044
s->EOL();
10511045
(*pos)->Dump(s, max_opcode_byte_size, show_address, show_bytes,
1052-
show_control_flow_kind, exe_ctx, nullptr, nullptr,
1053-
disassembly_format, 0);
1046+
show_control_flow_kind, exe_ctx, nullptr, nullptr, &format, 0);
10541047
}
10551048
}
10561049

‎lldb/source/Core/Statusline.cpp

Lines changed: 3 additions & 3 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

151151
Draw(std::string(stream.GetString()));
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::GetFormatEntity() 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/Target/StackFrame.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1936,7 +1936,7 @@ void StackFrame::DumpUsingSettingsFormat(Stream *strm, bool show_unique,
19361936

19371937
ExecutionContext exe_ctx(shared_from_this());
19381938

1939-
const FormatEntity::Entry *frame_format = nullptr;
1939+
FormatEntity::Entry frame_format;
19401940
Target *target = exe_ctx.GetTargetPtr();
19411941
if (target) {
19421942
if (show_unique) {
@@ -1945,7 +1945,7 @@ void StackFrame::DumpUsingSettingsFormat(Stream *strm, bool show_unique,
19451945
frame_format = target->GetDebugger().GetFrameFormat();
19461946
}
19471947
}
1948-
if (!DumpUsingFormat(*strm, frame_format, frame_marker)) {
1948+
if (!DumpUsingFormat(*strm, &frame_format, frame_marker)) {
19491949
Dump(strm, true, false);
19501950
strm->EOL();
19511951
}

‎lldb/source/Target/Thread.cpp

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1667,15 +1667,13 @@ void Thread::DumpUsingSettingsFormat(Stream &strm, uint32_t frame_idx,
16671667
bool stop_format) {
16681668
ExecutionContext exe_ctx(shared_from_this());
16691669

1670-
const FormatEntity::Entry *thread_format;
1670+
FormatEntity::Entry thread_format;
16711671
if (stop_format)
16721672
thread_format = exe_ctx.GetTargetRef().GetDebugger().GetThreadStopFormat();
16731673
else
16741674
thread_format = exe_ctx.GetTargetRef().GetDebugger().GetThreadFormat();
16751675

1676-
assert(thread_format);
1677-
1678-
DumpUsingFormat(strm, frame_idx, thread_format);
1676+
DumpUsingFormat(strm, frame_idx, &thread_format);
16791677
}
16801678

16811679
void Thread::SettingsInitialize() {}

‎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
}

0 commit comments

Comments
 (0)