=?utf-8?q?José?= L. Junior <jljunio...@gmail.com> Message-ID: In-Reply-To: <llvm.org/llvm/llvm-project/pull/76...@github.com>
llvmbot wrote: <!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: José Lira Junior (junior-jl) <details> <summary>Changes</summary> Follow-up to #<!-- -->69422. This commit builds upon the previous contribution that introduced symbol colorization in the image lookup command when using regex. In response to feedback from reviewers, this follow-up refines the colorization mechanism based on their recommendations. Changes: - Refactors symbol colorization logic to incorporate feedback from the previous commit. - Extends colorization to functions in addition to symbols for a more comprehensive visual representation. Co-authored-by: Talha Tahir <talha.tahir@<!-- -->10xengineers.ai> --- Full diff: https://github.com/llvm/llvm-project/pull/76112.diff 9 Files Affected: - (modified) lldb/include/lldb/Core/Address.h (+3-1) - (modified) lldb/include/lldb/Symbol/Symbol.h (+2-1) - (modified) lldb/include/lldb/Symbol/SymbolContext.h (+3-2) - (modified) lldb/include/lldb/Utility/Stream.h (+11-3) - (modified) lldb/source/Commands/CommandObjectTarget.cpp (+24-14) - (modified) lldb/source/Core/Address.cpp (+9-9) - (modified) lldb/source/Symbol/Symbol.cpp (+3-5) - (modified) lldb/source/Symbol/SymbolContext.cpp (+7-7) - (modified) lldb/source/Utility/Stream.cpp (+4-6) ``````````diff diff --git a/lldb/include/lldb/Core/Address.h b/lldb/include/lldb/Core/Address.h index 725b5d9f91d3d5..c5a47321c774e7 100644 --- a/lldb/include/lldb/Core/Address.h +++ b/lldb/include/lldb/Core/Address.h @@ -13,6 +13,7 @@ #include "lldb/lldb-forward.h" #include "lldb/lldb-private-enumerations.h" #include "lldb/lldb-types.h" +#include "lldb/Utility/Stream.h" #include "llvm/ADT/StringRef.h" @@ -255,7 +256,8 @@ class Address { bool Dump(Stream *s, ExecutionContextScope *exe_scope, DumpStyle style, DumpStyle fallback_style = DumpStyleInvalid, uint32_t addr_byte_size = UINT32_MAX, bool all_ranges = false, - llvm::StringRef pattern = "") const; + std::optional<Information> pattern_info = std::nullopt) const; + AddressClass GetAddressClass() const; diff --git a/lldb/include/lldb/Symbol/Symbol.h b/lldb/include/lldb/Symbol/Symbol.h index e6c0b495bcf28c..96ba7ba282a01c 100644 --- a/lldb/include/lldb/Symbol/Symbol.h +++ b/lldb/include/lldb/Symbol/Symbol.h @@ -16,6 +16,7 @@ #include "lldb/Utility/UserID.h" #include "lldb/lldb-private.h" #include "llvm/Support/JSON.h" +#include "lldb/Utility/Stream.h" namespace lldb_private { @@ -175,7 +176,7 @@ class Symbol : public SymbolContextScope { void SetFlags(uint32_t flags) { m_flags = flags; } void GetDescription(Stream *s, lldb::DescriptionLevel level, Target *target, - llvm::StringRef pattern = "") const; + std::optional<Information> pattern_info = std::nullopt) const; bool IsSynthetic() const { return m_is_synthetic; } diff --git a/lldb/include/lldb/Symbol/SymbolContext.h b/lldb/include/lldb/Symbol/SymbolContext.h index 26f3bac09a9626..529dc9630840b7 100644 --- a/lldb/include/lldb/Symbol/SymbolContext.h +++ b/lldb/include/lldb/Symbol/SymbolContext.h @@ -17,6 +17,7 @@ #include "lldb/Core/Mangled.h" #include "lldb/Symbol/LineEntry.h" #include "lldb/Utility/Iterable.h" +#include "lldb/Utility/Stream.h" #include "lldb/lldb-private.h" namespace lldb_private { @@ -157,7 +158,7 @@ class SymbolContext { const Address &so_addr, bool show_fullpaths, bool show_module, bool show_inlined_frames, bool show_function_arguments, bool show_function_name, - llvm::StringRef pattern = "") const; + std::optional<Information> pattern_info = std::nullopt) const; /// Get the address range contained within a symbol context. /// @@ -224,7 +225,7 @@ class SymbolContext { const Symbol *FindBestGlobalDataSymbol(ConstString name, Status &error); void GetDescription(Stream *s, lldb::DescriptionLevel level, Target *target, - llvm::StringRef pattern = "") const; + std::optional<Information> pattern_info = std::nullopt) const; uint32_t GetResolvedMask() const; diff --git a/lldb/include/lldb/Utility/Stream.h b/lldb/include/lldb/Utility/Stream.h index 20c55ac4597ae6..7aed0a831631bd 100644 --- a/lldb/include/lldb/Utility/Stream.h +++ b/lldb/include/lldb/Utility/Stream.h @@ -23,6 +23,16 @@ namespace lldb_private { +struct Information { + llvm::StringRef pattern; + llvm::StringRef prefix; + llvm::StringRef suffix; + + // Constructor + Information(llvm::StringRef p, llvm::StringRef pre, llvm::StringRef suf) + : pattern(p), prefix(pre), suffix(suf) {} +}; + /// \class Stream Stream.h "lldb/Utility/Stream.h" /// A stream class that can stream formatted output to a file. class Stream { @@ -261,9 +271,7 @@ class Stream { /// environment-dependent. void PutCStringColorHighlighted(llvm::StringRef text, - llvm::StringRef pattern = "", - llvm::StringRef prefix = "", - llvm::StringRef suffix = ""); + std::optional<Information> pattern_info = std::nullopt); /// Output and End of Line character to the stream. size_t EOL(); diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp index bc8bc51356c8ca..ed2e3fee191b57 100644 --- a/lldb/source/Commands/CommandObjectTarget.cpp +++ b/lldb/source/Commands/CommandObjectTarget.cpp @@ -55,6 +55,7 @@ #include "lldb/Utility/State.h" #include "lldb/Utility/StructuredData.h" #include "lldb/Utility/Timer.h" +#include "lldb/Utility/Stream.h" #include "lldb/lldb-enumerations.h" #include "lldb/lldb-private-enumerations.h" @@ -1533,7 +1534,7 @@ static void DumpOsoFilesTable(Stream &strm, static void DumpAddress(ExecutionContextScope *exe_scope, const Address &so_addr, bool verbose, bool all_ranges, - Stream &strm, llvm::StringRef pattern = "") { + Stream &strm, std::optional<Information> pattern_info = std::nullopt) { strm.IndentMore(); strm.Indent(" Address: "); so_addr.Dump(&strm, exe_scope, Address::DumpStyleModuleWithFileAddress); @@ -1544,13 +1545,14 @@ static void DumpAddress(ExecutionContextScope *exe_scope, const uint32_t save_indent = strm.GetIndentLevel(); strm.SetIndentLevel(save_indent + 13); so_addr.Dump(&strm, exe_scope, Address::DumpStyleResolvedDescription, - Address::DumpStyleInvalid, UINT32_MAX, false, pattern); + Address::DumpStyleInvalid, UINT32_MAX, false, pattern_info); strm.SetIndentLevel(save_indent); // Print out detailed address information when verbose is enabled if (verbose) { strm.EOL(); so_addr.Dump(&strm, exe_scope, Address::DumpStyleDetailedSymbolContext, - Address::DumpStyleInvalid, UINT32_MAX, all_ranges, pattern); + Address::DumpStyleInvalid, UINT32_MAX, all_ranges, + pattern_info); } strm.IndentLess(); } @@ -1609,6 +1611,11 @@ static uint32_t LookupSymbolInModule(CommandInterpreter &interpreter, } if (num_matches > 0) { + llvm::StringRef ansi_prefix = + interpreter.GetDebugger().GetRegexMatchAnsiPrefix(); + llvm::StringRef ansi_suffix = + interpreter.GetDebugger().GetRegexMatchAnsiSuffix(); + Information info(name, ansi_prefix, ansi_suffix); strm.Indent(); strm.Printf("%u symbols match %s'%s' in ", num_matches, name_is_regex ? "the regular expression " : "", name); @@ -1622,18 +1629,14 @@ static uint32_t LookupSymbolInModule(CommandInterpreter &interpreter, DumpAddress( interpreter.GetExecutionContext().GetBestExecutionContextScope(), symbol->GetAddressRef(), verbose, all_ranges, strm, - use_color && name_is_regex ? name : nullptr); + use_color && name_is_regex ? std::optional<Information>{info} : std::nullopt); strm.EOL(); } else { strm.IndentMore(); strm.Indent(" Name: "); - llvm::StringRef ansi_prefix = - interpreter.GetDebugger().GetRegexMatchAnsiPrefix(); - llvm::StringRef ansi_suffix = - interpreter.GetDebugger().GetRegexMatchAnsiSuffix(); strm.PutCStringColorHighlighted( symbol->GetDisplayName().GetStringRef(), - use_color ? name : nullptr, ansi_prefix, ansi_suffix); + use_color ? std::optional<Information>{info} : std::nullopt); strm.EOL(); strm.Indent(" Value: "); strm.Printf("0x%16.16" PRIx64 "\n", symbol->GetRawValue()); @@ -1653,7 +1656,8 @@ static uint32_t LookupSymbolInModule(CommandInterpreter &interpreter, static void DumpSymbolContextList(ExecutionContextScope *exe_scope, Stream &strm, const SymbolContextList &sc_list, - bool verbose, bool all_ranges) { + bool verbose, bool all_ranges, + std::optional<Information> pattern_info = std::nullopt) { strm.IndentMore(); bool first_module = true; for (const SymbolContext &sc : sc_list) { @@ -1661,10 +1665,9 @@ static void DumpSymbolContextList(ExecutionContextScope *exe_scope, strm.EOL(); AddressRange range; - sc.GetAddressRange(eSymbolContextEverything, 0, true, range); - - DumpAddress(exe_scope, range.GetBaseAddress(), verbose, all_ranges, strm); + DumpAddress(exe_scope, range.GetBaseAddress(), verbose, all_ranges, strm, + pattern_info); first_module = false; } strm.IndentLess(); @@ -1675,6 +1678,7 @@ static size_t LookupFunctionInModule(CommandInterpreter &interpreter, const char *name, bool name_is_regex, const ModuleFunctionSearchOptions &options, bool verbose, bool all_ranges) { + const bool use_color = interpreter.GetDebugger().GetUseColor(); if (module && name && name[0]) { SymbolContextList sc_list; size_t num_matches = 0; @@ -1688,6 +1692,11 @@ static size_t LookupFunctionInModule(CommandInterpreter &interpreter, } num_matches = sc_list.GetSize(); if (num_matches) { + llvm::StringRef ansi_prefix = + interpreter.GetDebugger().GetRegexMatchAnsiPrefix(); + llvm::StringRef ansi_suffix = + interpreter.GetDebugger().GetRegexMatchAnsiSuffix(); + Information info(name, ansi_prefix, ansi_suffix); strm.Indent(); strm.Printf("%" PRIu64 " match%s found in ", (uint64_t)num_matches, num_matches > 1 ? "es" : ""); @@ -1695,7 +1704,8 @@ static size_t LookupFunctionInModule(CommandInterpreter &interpreter, strm.PutCString(":\n"); DumpSymbolContextList( interpreter.GetExecutionContext().GetBestExecutionContextScope(), - strm, sc_list, verbose, all_ranges); + strm, sc_list, verbose, all_ranges, + use_color && name_is_regex ? std::optional<Information>{info} : std::nullopt); } return num_matches; } diff --git a/lldb/source/Core/Address.cpp b/lldb/source/Core/Address.cpp index 19d34db44ea55c..fe6ec00ad2b955 100644 --- a/lldb/source/Core/Address.cpp +++ b/lldb/source/Core/Address.cpp @@ -407,7 +407,7 @@ bool Address::GetDescription(Stream &s, Target &target, bool Address::Dump(Stream *s, ExecutionContextScope *exe_scope, DumpStyle style, DumpStyle fallback_style, uint32_t addr_size, - bool all_ranges, llvm::StringRef pattern) const { + bool all_ranges, std::optional<Information> pattern_info) const { // If the section was nullptr, only load address is going to work unless we // are trying to deref a pointer SectionSP section_sp(GetSection()); @@ -524,8 +524,7 @@ bool Address::Dump(Stream *s, ExecutionContextScope *exe_scope, DumpStyle style, ansi_suffix = target->GetDebugger().GetRegexMatchAnsiSuffix(); } - s->PutCStringColorHighlighted(symbol_name, pattern, - ansi_prefix, ansi_suffix); + s->PutCStringColorHighlighted(symbol_name, pattern_info); addr_t delta = file_Addr - symbol->GetAddressRef().GetFileAddress(); if (delta) @@ -653,7 +652,7 @@ bool Address::Dump(Stream *s, ExecutionContextScope *exe_scope, DumpStyle style, pointer_sc.symbol != nullptr) { s->PutCString(": "); pointer_sc.DumpStopContext(s, exe_scope, so_addr, true, false, - false, true, true, pattern); + false, true, true, pattern_info); } } } @@ -693,13 +692,13 @@ bool Address::Dump(Stream *s, ExecutionContextScope *exe_scope, DumpStyle style, sc.DumpStopContext(s, exe_scope, *this, show_fullpaths, show_module, show_inlined_frames, show_function_arguments, show_function_name, - pattern); + pattern_info); } else { // We found a symbol but it was in a different section so it // isn't the symbol we should be showing, just show the section // name + offset Dump(s, exe_scope, DumpStyleSectionNameOffset, DumpStyleInvalid, - UINT32_MAX, false, pattern); + UINT32_MAX, false, pattern_info); } } } @@ -707,7 +706,7 @@ bool Address::Dump(Stream *s, ExecutionContextScope *exe_scope, DumpStyle style, } else { if (fallback_style != DumpStyleInvalid) return Dump(s, exe_scope, fallback_style, DumpStyleInvalid, addr_size, - false, pattern); + false, pattern_info); return false; } break; @@ -728,7 +727,8 @@ bool Address::Dump(Stream *s, ExecutionContextScope *exe_scope, DumpStyle style, sc.symbol->GetAddressRef().GetSection() != GetSection()) sc.symbol = nullptr; } - sc.GetDescription(s, eDescriptionLevelBrief, target, pattern); + + sc.GetDescription(s, eDescriptionLevelBrief, target, pattern_info); if (sc.block) { bool can_create = true; @@ -777,7 +777,7 @@ bool Address::Dump(Stream *s, ExecutionContextScope *exe_scope, DumpStyle style, } else { if (fallback_style != DumpStyleInvalid) return Dump(s, exe_scope, fallback_style, DumpStyleInvalid, addr_size, - false, pattern); + false, pattern_info); return false; } break; diff --git a/lldb/source/Symbol/Symbol.cpp b/lldb/source/Symbol/Symbol.cpp index fcc45f861c2255..60763f878b8368 100644 --- a/lldb/source/Symbol/Symbol.cpp +++ b/lldb/source/Symbol/Symbol.cpp @@ -227,7 +227,7 @@ bool Symbol::IsTrampoline() const { return m_type == eSymbolTypeTrampoline; } bool Symbol::IsIndirect() const { return m_type == eSymbolTypeResolver; } void Symbol::GetDescription(Stream *s, lldb::DescriptionLevel level, - Target *target, llvm::StringRef pattern) const { + Target *target, std::optional<Information> pattern_info) const { s->Printf("id = {0x%8.8x}", m_uid); if (m_addr_range.GetBaseAddress().GetSection()) { @@ -262,14 +262,12 @@ void Symbol::GetDescription(Stream *s, lldb::DescriptionLevel level, } if (ConstString demangled = m_mangled.GetDemangledName()) { s->PutCString(", name=\""); - s->PutCStringColorHighlighted(demangled.GetStringRef(), pattern, - ansi_prefix, ansi_suffix); + s->PutCStringColorHighlighted(demangled.GetStringRef(), pattern_info); s->PutCString("\""); } if (ConstString mangled_name = m_mangled.GetMangledName()) { s->PutCString(", mangled=\""); - s->PutCStringColorHighlighted(mangled_name.GetStringRef(), pattern, - ansi_prefix, ansi_suffix); + s->PutCStringColorHighlighted(mangled_name.GetStringRef(), pattern_info); s->PutCString("\""); } } diff --git a/lldb/source/Symbol/SymbolContext.cpp b/lldb/source/Symbol/SymbolContext.cpp index 9fd40b5ca567f8..10dc266c008487 100644 --- a/lldb/source/Symbol/SymbolContext.cpp +++ b/lldb/source/Symbol/SymbolContext.cpp @@ -25,6 +25,7 @@ #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" #include "lldb/Utility/StreamString.h" +#include "lldb/Utility/Stream.h" #include "lldb/lldb-enumerations.h" using namespace lldb; @@ -73,7 +74,7 @@ bool SymbolContext::DumpStopContext(Stream *s, ExecutionContextScope *exe_scope, bool show_module, bool show_inlined_frames, bool show_function_arguments, bool show_function_name, - llvm::StringRef pattern) const { + std::optional<Information> pattern_info) const { bool dumped_something = false; if (show_module && module_sp) { if (show_fullpaths) @@ -102,8 +103,7 @@ bool SymbolContext::DumpStopContext(Stream *s, ExecutionContextScope *exe_scope, ansi_prefix = target_sp->GetDebugger().GetRegexMatchAnsiPrefix(); ansi_suffix = target_sp->GetDebugger().GetRegexMatchAnsiSuffix(); } - s->PutCStringColorHighlighted(name.GetStringRef(), pattern, ansi_prefix, - ansi_suffix); + s->PutCStringColorHighlighted(name.GetStringRef(), pattern_info); } } @@ -178,8 +178,8 @@ bool SymbolContext::DumpStopContext(Stream *s, ExecutionContextScope *exe_scope, ansi_prefix = target_sp->GetDebugger().GetRegexMatchAnsiPrefix(); ansi_suffix = target_sp->GetDebugger().GetRegexMatchAnsiSuffix(); } - s->PutCStringColorHighlighted(symbol->GetName().GetStringRef(), pattern, - ansi_prefix, ansi_suffix); + s->PutCStringColorHighlighted(symbol->GetName().GetStringRef(), + pattern_info); } if (addr.IsValid() && symbol->ValueIsAddress()) { @@ -203,7 +203,7 @@ bool SymbolContext::DumpStopContext(Stream *s, ExecutionContextScope *exe_scope, void SymbolContext::GetDescription(Stream *s, lldb::DescriptionLevel level, Target *target, - llvm::StringRef pattern) const { + std::optional<Information> pattern_info) const { if (module_sp) { s->Indent(" Module: file = \""); module_sp->GetFileSpec().Dump(s->AsRawOstream()); @@ -263,7 +263,7 @@ void SymbolContext::GetDescription(Stream *s, lldb::DescriptionLevel level, if (symbol != nullptr) { s->Indent(" Symbol: "); - symbol->GetDescription(s, level, target, pattern); + symbol->GetDescription(s, level, target, pattern_info); s->EOL(); } diff --git a/lldb/source/Utility/Stream.cpp b/lldb/source/Utility/Stream.cpp index 62e061e9d09c07..9a670ea9b54d8b 100644 --- a/lldb/source/Utility/Stream.cpp +++ b/lldb/source/Utility/Stream.cpp @@ -73,22 +73,20 @@ size_t Stream::PutCString(llvm::StringRef str) { } void Stream::PutCStringColorHighlighted(llvm::StringRef text, - llvm::StringRef pattern, - llvm::StringRef prefix, - llvm::StringRef suffix) { + std::optional<Information> pattern_info) { // Only apply color formatting when a pattern is present and both prefix and // suffix are specified. In the absence of these conditions, output the text // without color formatting. - if (pattern.empty() || (prefix.empty() && suffix.empty())) { + if (!pattern_info.has_value()) { PutCString(text); return; } - llvm::Regex reg_pattern(pattern); + llvm::Regex reg_pattern(pattern_info.value().pattern); llvm::SmallVector<llvm::StringRef, 1> matches; llvm::StringRef remaining = text; std::string format_str = lldb_private::ansi::FormatAnsiTerminalCodes( - prefix.str() + "%.*s" + suffix.str()); + pattern_info.value().prefix.str() + "%.*s" + pattern_info.value().suffix.str()); while (reg_pattern.match(remaining, &matches)) { llvm::StringRef match = matches[0]; size_t match_start_pos = match.data() - remaining.data(); `````````` </details> https://github.com/llvm/llvm-project/pull/76112 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
