llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) <details> <summary>Changes</summary> Depends on: * https://github.com/llvm/llvm-project/pull/174618 If a format entity calls back into `Format` and passes it a format entity type that we're already in the process of parsing, we are likely going to run into infinite recursion and blow the stack. An example of this can be seen in the test-case adjusted by this patch. These seems to be causing actual crashes in the field, so this patch adds basic tracking to `Formatter::Format` that checks whether we're recursively parsing the same entity. This may very well be intended by some entities (e.g., `Root` and `Scope`), so there is an escape hatch for those. Adding a unit-test for this is unfortunately tricky because the only format entity that currently suffers from this is `${function.name-with-args}`, which requires a language plugin and valid target. If we really wanted to we could probably mock all of those, but the shell test provides test coverage for the previously crashing case. rdar://166890120 --- Patch is 43.51 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/174750.diff 11 Files Affected: - (modified) lldb/include/lldb/Core/FormatEntity.h (+44-7) - (modified) lldb/source/Core/Debugger.cpp (+3-2) - (modified) lldb/source/Core/FormatEntity.cpp (+182-179) - (modified) lldb/source/Core/IOHandlerCursesGUI.cpp (+10-8) - (modified) lldb/source/Core/Statusline.cpp (+2-2) - (modified) lldb/source/DataFormatters/FormattersHelpers.cpp (+3-2) - (modified) lldb/source/DataFormatters/TypeSummary.cpp (+4-3) - (modified) lldb/source/Target/StackFrame.cpp (+2-2) - (modified) lldb/source/Target/Thread.cpp (+3-2) - (modified) lldb/test/Shell/Settings/TestCxxFrameFormatRecursive.test (-4) - (modified) lldb/unittests/Core/FormatEntityTest.cpp (+2-2) ``````````diff diff --git a/lldb/include/lldb/Core/FormatEntity.h b/lldb/include/lldb/Core/FormatEntity.h index 107c30a000979..e01009a44aac7 100644 --- a/lldb/include/lldb/Core/FormatEntity.h +++ b/lldb/include/lldb/Core/FormatEntity.h @@ -11,6 +11,7 @@ #include "lldb/lldb-enumerations.h" #include "lldb/lldb-types.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/SmallVector.h" #include <algorithm> #include <cstddef> @@ -229,14 +230,50 @@ struct Entry { bool deref = false; }; -bool Format(const Entry &entry, Stream &s, const SymbolContext *sc, - const ExecutionContext *exe_ctx, const Address *addr, - ValueObject *valobj, bool function_changed, bool initial_function); +class Formatter { +public: + Formatter(const SymbolContext *sc, const ExecutionContext *exe_ctx, + const Address *addr, bool function_changed, bool initial_function) + : m_sc(sc), m_exe_ctx(exe_ctx), m_addr(addr), + m_function_changed(function_changed), + m_initial_function(initial_function) {} + + bool Format(const Entry &entry, Stream &s, ValueObject *valobj = nullptr); + + bool FormatStringRef(const llvm::StringRef &format, Stream &s, + ValueObject *valobj); + +private: + bool DumpValue(Stream &s, const FormatEntity::Entry &entry, + ValueObject *valobj); + + bool FormatFunctionNameForLanguage(Stream &s); + + /// Returns \c true if \a Format has been called for an \a Entry + /// with the specified \c type recusrively. Some types are permitted + /// to be formatted recursively, in which case this function returns + /// \c false. + bool IsInvalidRecursiveFormat(Entry::Type type); + + /// While the returned \a llvm::scope_exit is alive, the specified \c type + /// is tracked by this \c Formatter object for recursion. Once the returned + /// scope guard is destructed, the entry stops being tracked. + auto PushEntryType(Entry::Type type) { + m_entry_type_stack.push_back(type); + return llvm::scope_exit([this] { + assert(!m_entry_type_stack.empty()); + m_entry_type_stack.pop_back(); + }); + } + + const SymbolContext *const m_sc = nullptr; + const ExecutionContext *const m_exe_ctx = nullptr; + const Address *const m_addr = nullptr; + const bool m_function_changed = false; + const bool m_initial_function = false; -bool FormatStringRef(const llvm::StringRef &format, Stream &s, - const SymbolContext *sc, const ExecutionContext *exe_ctx, - const Address *addr, ValueObject *valobj, - bool function_changed, bool initial_function); + llvm::SmallVector<Entry::Type, 1> m_entry_type_stack; +}; Status Parse(const llvm::StringRef &format, Entry &entry); diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp index 99f4a728e3f17..1fae5654beed5 100644 --- a/lldb/source/Core/Debugger.cpp +++ b/lldb/source/Core/Debugger.cpp @@ -1607,8 +1607,9 @@ 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, - function_changed, initial_function); + return FormatEntity::Formatter(sc, exe_ctx, addr, function_changed, + initial_function) + .Format(*format, s); } void Debugger::AssertCallback(llvm::StringRef message, diff --git a/lldb/source/Core/FormatEntity.cpp b/lldb/source/Core/FormatEntity.cpp index faafb5eb6e4f2..244f901ad11c2 100644 --- a/lldb/source/Core/FormatEntity.cpp +++ b/lldb/source/Core/FormatEntity.cpp @@ -754,14 +754,14 @@ static bool DumpValueWithLLVMFormat(Stream &s, llvm::StringRef options, return true; } -static bool DumpValue(Stream &s, const SymbolContext *sc, - const ExecutionContext *exe_ctx, - const FormatEntity::Entry &entry, ValueObject *valobj) { +bool FormatEntity::Formatter::DumpValue(Stream &s, + const FormatEntity::Entry &entry, + ValueObject *valobj) { if (valobj == nullptr) return false; Log *log = GetLog(LLDBLog::DataFormatters); - Format custom_format = eFormatInvalid; + enum Format custom_format = eFormatInvalid; ValueObject::ValueObjectRepresentationStyle val_obj_display = entry.string.empty() ? ValueObject::eValueObjectRepresentationStyleValue @@ -821,7 +821,8 @@ static bool DumpValue(Stream &s, const SymbolContext *sc, ValueObject::eExpressionPathEndResultTypePlain; if (is_script) { - return RunScriptFormatKeyword(s, sc, exe_ctx, valobj, entry.string.c_str()); + return RunScriptFormatKeyword(s, m_sc, m_exe_ctx, valobj, + entry.string.c_str()); } auto split = llvm::StringRef(entry.string).split(':'); @@ -1057,8 +1058,7 @@ static bool DumpValue(Stream &s, const SymbolContext *sc, success &= item->DumpPrintableRepresentation(s, val_obj_display, custom_format); } else { - success &= FormatEntity::FormatStringRef( - special_directions, s, sc, exe_ctx, nullptr, item, false, false); + success &= FormatStringRef(special_directions, s, item); } if (--max_num_children == 0) { @@ -1272,16 +1272,14 @@ static bool HandleFunctionNameWithArgs(Stream &s, return true; } -static bool FormatFunctionNameForLanguage(Stream &s, - const ExecutionContext *exe_ctx, - const SymbolContext *sc) { - assert(sc); +bool FormatEntity::Formatter::FormatFunctionNameForLanguage(Stream &s) { + assert(m_sc); Language *language_plugin = nullptr; - if (sc->function) - language_plugin = Language::FindPlugin(sc->function->GetLanguage()); - else if (sc->symbol) - language_plugin = Language::FindPlugin(sc->symbol->GetLanguage()); + if (m_sc->function) + language_plugin = Language::FindPlugin(m_sc->function->GetLanguage()); + else if (m_sc->symbol) + language_plugin = Language::FindPlugin(m_sc->symbol->GetLanguage()); if (!language_plugin) return false; @@ -1293,38 +1291,36 @@ static bool FormatFunctionNameForLanguage(Stream &s, return false; StreamString name_stream; - const bool success = - FormatEntity::Format(format, name_stream, sc, exe_ctx, /*addr=*/nullptr, - /*valobj=*/nullptr, /*function_changed=*/false, - /*initial_function=*/false); + const bool success = Format(format, name_stream, /*valobj=*/nullptr); if (success) s << name_stream.GetString(); return success; } -bool FormatEntity::FormatStringRef(const llvm::StringRef &format_str, Stream &s, - const SymbolContext *sc, - const ExecutionContext *exe_ctx, - const Address *addr, ValueObject *valobj, - bool function_changed, - bool initial_function) { +bool FormatEntity::Formatter::FormatStringRef(const llvm::StringRef &format_str, + Stream &s, ValueObject *valobj) { if (!format_str.empty()) { FormatEntity::Entry root; Status error = FormatEntity::Parse(format_str, root); if (error.Success()) { - return FormatEntity::Format(root, s, sc, exe_ctx, addr, valobj, - function_changed, initial_function); + return Format(root, s, valobj); } } return false; } -bool FormatEntity::Format(const Entry &entry, Stream &s, - const SymbolContext *sc, - const ExecutionContext *exe_ctx, const Address *addr, - ValueObject *valobj, bool function_changed, - bool initial_function) { +bool FormatEntity::Formatter::Format(const Entry &entry, Stream &s, + ValueObject *valobj) { + if (IsInvalidRecursiveFormat(entry.type)) { + LLDB_LOG(GetLog(LLDBLog::DataFormatters), + "Error: detected recursive format entity: {0}", + FormatEntity::Entry::TypeToCString(entry.type)); + return false; + } + + auto entry_stack_guard = PushEntryType(entry.type); + switch (entry.type) { case Entry::Type::Invalid: case Entry::Type::ParentNumber: // Only used for @@ -1333,7 +1329,7 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, // FormatEntity::Entry::Definition encoding return false; case Entry::Type::EscapeCode: - if (Target *target = Target::GetTargetFromContexts(exe_ctx, sc)) { + if (Target *target = Target::GetTargetFromContexts(m_exe_ctx, m_sc)) { Debugger &debugger = target->GetDebugger(); if (debugger.GetUseColor()) { s.PutCString(entry.string); @@ -1344,8 +1340,7 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, case Entry::Type::Root: for (const auto &child : entry.children_stack[0]) { - if (!Format(child, s, sc, exe_ctx, addr, valobj, function_changed, - initial_function)) { + if (!Format(child, s, valobj)) { return false; // If any item of root fails, then the formatting fails } } @@ -1360,8 +1355,7 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, auto format_children = [&](const std::vector<Entry> &children) { scope_stream.Clear(); for (const auto &child : children) { - if (!Format(child, scope_stream, sc, exe_ctx, addr, valobj, - function_changed, initial_function)) + if (!Format(child, scope_stream, valobj)) return false; } return true; @@ -1382,19 +1376,19 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, case Entry::Type::VariableSynthetic: case Entry::Type::ScriptVariable: case Entry::Type::ScriptVariableSynthetic: - return DumpValue(s, sc, exe_ctx, entry, valobj); + return DumpValue(s, entry, valobj); case Entry::Type::AddressFile: case Entry::Type::AddressLoad: case Entry::Type::AddressLoadOrFile: return ( - addr != nullptr && addr->IsValid() && - DumpAddressAndContent(s, sc, exe_ctx, *addr, + m_addr != nullptr && m_addr->IsValid() && + DumpAddressAndContent(s, m_sc, m_exe_ctx, *m_addr, entry.type == Entry::Type::AddressLoadOrFile)); case Entry::Type::ProcessID: - if (exe_ctx) { - Process *process = exe_ctx->GetProcessPtr(); + if (m_exe_ctx) { + Process *process = m_exe_ctx->GetProcessPtr(); if (process) { const char *format = "%" PRIu64; if (!entry.printf_format.empty()) @@ -1406,8 +1400,8 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::ProcessFile: - if (exe_ctx) { - Process *process = exe_ctx->GetProcessPtr(); + if (m_exe_ctx) { + Process *process = m_exe_ctx->GetProcessPtr(); if (process) { Module *exe_module = process->GetTarget().GetExecutableModulePointer(); if (exe_module) { @@ -1419,17 +1413,17 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::ScriptProcess: - if (exe_ctx) { - Process *process = exe_ctx->GetProcessPtr(); + if (m_exe_ctx) { + Process *process = m_exe_ctx->GetProcessPtr(); if (process) - return RunScriptFormatKeyword(s, sc, exe_ctx, process, + return RunScriptFormatKeyword(s, m_sc, m_exe_ctx, process, entry.string.c_str()); } return false; case Entry::Type::ThreadID: - if (exe_ctx) { - Thread *thread = exe_ctx->GetThreadPtr(); + if (m_exe_ctx) { + Thread *thread = m_exe_ctx->GetThreadPtr(); if (thread) { const char *format = "0x%4.4" PRIx64; if (!entry.printf_format.empty()) { @@ -1460,8 +1454,8 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::ThreadProtocolID: - if (exe_ctx) { - Thread *thread = exe_ctx->GetThreadPtr(); + if (m_exe_ctx) { + Thread *thread = m_exe_ctx->GetThreadPtr(); if (thread) { const char *format = "0x%4.4" PRIx64; if (!entry.printf_format.empty()) @@ -1473,8 +1467,8 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::ThreadIndexID: - if (exe_ctx) { - Thread *thread = exe_ctx->GetThreadPtr(); + if (m_exe_ctx) { + Thread *thread = m_exe_ctx->GetThreadPtr(); if (thread) { const char *format = "%" PRIu32; if (!entry.printf_format.empty()) @@ -1486,8 +1480,8 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::ThreadName: - if (exe_ctx) { - Thread *thread = exe_ctx->GetThreadPtr(); + if (m_exe_ctx) { + Thread *thread = m_exe_ctx->GetThreadPtr(); if (thread) { const char *cstr = thread->GetName(); if (cstr && cstr[0]) { @@ -1499,8 +1493,8 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::ThreadQueue: - if (exe_ctx) { - Thread *thread = exe_ctx->GetThreadPtr(); + if (m_exe_ctx) { + Thread *thread = m_exe_ctx->GetThreadPtr(); if (thread) { const char *cstr = thread->GetQueueName(); if (cstr && cstr[0]) { @@ -1512,8 +1506,8 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::ThreadStopReason: - if (exe_ctx) { - if (Thread *thread = exe_ctx->GetThreadPtr()) { + if (m_exe_ctx) { + if (Thread *thread = m_exe_ctx->GetThreadPtr()) { std::string stop_description = thread->GetStopDescription(); if (!stop_description.empty()) { s.PutCString(stop_description); @@ -1524,8 +1518,8 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::ThreadStopReasonRaw: - if (exe_ctx) { - if (Thread *thread = exe_ctx->GetThreadPtr()) { + if (m_exe_ctx) { + if (Thread *thread = m_exe_ctx->GetThreadPtr()) { std::string stop_description = thread->GetStopDescriptionRaw(); if (!stop_description.empty()) { s.PutCString(stop_description); @@ -1536,8 +1530,8 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::ThreadReturnValue: - if (exe_ctx) { - Thread *thread = exe_ctx->GetThreadPtr(); + if (m_exe_ctx) { + Thread *thread = m_exe_ctx->GetThreadPtr(); if (thread) { StopInfoSP stop_info_sp = thread->GetStopInfo(); if (stop_info_sp && stop_info_sp->IsValid()) { @@ -1556,8 +1550,8 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::ThreadCompletedExpression: - if (exe_ctx) { - Thread *thread = exe_ctx->GetThreadPtr(); + if (m_exe_ctx) { + Thread *thread = m_exe_ctx->GetThreadPtr(); if (thread) { StopInfoSP stop_info_sp = thread->GetStopInfo(); if (stop_info_sp && stop_info_sp->IsValid()) { @@ -1577,22 +1571,23 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::ScriptThread: - if (exe_ctx) { - Thread *thread = exe_ctx->GetThreadPtr(); + if (m_exe_ctx) { + Thread *thread = m_exe_ctx->GetThreadPtr(); if (thread) - return RunScriptFormatKeyword(s, sc, exe_ctx, thread, + return RunScriptFormatKeyword(s, m_sc, m_exe_ctx, thread, entry.string.c_str()); } return false; case Entry::Type::ThreadInfo: - if (exe_ctx) { - Thread *thread = exe_ctx->GetThreadPtr(); + if (m_exe_ctx) { + Thread *thread = m_exe_ctx->GetThreadPtr(); if (thread) { StructuredData::ObjectSP object_sp = thread->GetExtendedInfo(); if (object_sp && object_sp->GetType() == eStructuredDataTypeDictionary) { - if (FormatThreadExtendedInfoRecurse(entry, object_sp, sc, exe_ctx, s)) + if (FormatThreadExtendedInfoRecurse(entry, object_sp, m_sc, m_exe_ctx, + s)) return true; } } @@ -1600,8 +1595,8 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::TargetArch: - if (exe_ctx) { - Target *target = exe_ctx->GetTargetPtr(); + if (m_exe_ctx) { + Target *target = m_exe_ctx->GetTargetPtr(); if (target) { const ArchSpec &arch = target->GetArchitecture(); if (arch.IsValid()) { @@ -1613,8 +1608,8 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::TargetFile: - if (exe_ctx) { - if (Target *target = exe_ctx->GetTargetPtr()) { + if (m_exe_ctx) { + if (Target *target = m_exe_ctx->GetTargetPtr()) { if (Module *exe_module = target->GetExecutableModulePointer()) { if (DumpFile(s, exe_module->GetFileSpec(), (FileKind)entry.number)) return true; @@ -1624,17 +1619,17 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::ScriptTarget: - if (exe_ctx) { - Target *target = exe_ctx->GetTargetPtr(); + if (m_exe_ctx) { + Target *target = m_exe_ctx->GetTargetPtr(); if (target) - return RunScriptFormatKeyword(s, sc, exe_ctx, target, + return RunScriptFormatKeyword(s, m_sc, m_exe_ctx, target, entry.string.c_str()); } return false; case Entry::Type::ModuleFile: - if (sc) { - Module *module = sc->module_sp.get(); + if (m_sc) { + Module *module = m_sc->module_sp.get(); if (module) { if (DumpFile(s, module->GetFileSpec(), (FileKind)entry.number)) return true; @@ -1643,8 +1638,8 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::File: - if (sc) { - CompileUnit *cu = sc->comp_unit; + if (m_sc) { + CompileUnit *cu = m_sc->comp_unit; if (cu) { if (DumpFile(s, cu->GetPrimaryFile(), (FileKind)entry.number)) return true; @@ -1653,8 +1648,8 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::Lang: - if (sc) { - CompileUnit *cu = sc->comp_unit; + if (m_sc) { + CompileUnit *cu = m_sc->comp_unit; if (cu) { const char *lang_name = Language::GetNameForLanguageType(cu->GetLanguage()); @@ -1667,8 +1662,8 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::FrameIndex: - if (exe_ctx) { - StackFrame *frame = exe_ctx->GetFramePtr(); + if (m_exe_ctx) { + StackFrame *frame = m_exe_ctx->GetFramePtr(); if (frame) { const char *format = "%" PRIu32; if (!entry.printf_format.empty()) @@ -1680,20 +1675,20 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::FrameRegisterPC: - if (exe_ctx) { - StackFrame *frame = exe_ctx->GetFramePtr(); + if (m_exe_ctx) { + StackFrame *frame = m_exe_ctx->GetFramePtr(); if (frame) { const Address &pc_addr = frame->GetFrameCodeAddress(); if (pc_addr.IsValid()) - if (DumpAddressAndContent(s, sc, exe_ctx, pc_addr, false)) + if (DumpAddressAndContent(s, m_sc, m_exe_ctx, pc_addr, false)) return true; } } return false; case Entry::Type::FrameRegisterSP: - if (exe_ctx) { - StackFrame *frame = exe_ctx->GetFramePtr(); + if (m_exe_ctx) { + StackFrame *frame = m_exe_ctx->GetFramePtr(); if (frame) { if (DumpRegister(s, frame, eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP, (lldb::Format)entry.number)) @@ -1703,8 +1698,8 @@ bool FormatEntity::Format(const Entry &entry, Stream &s, return false; case Entry::Type::FrameRegisterFP: - if (exe_ctx) { - ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/174750 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
