[Lldb-commits] [lldb] [lldb][breakpoint] Grey out disabled breakpoints (PR #91404)
https://github.com/chelcassanova updated https://github.com/llvm/llvm-project/pull/91404 >From 5724d6c77d29ad80e9ca03ce7ac1c3e6ed33afc0 Mon Sep 17 00:00:00 2001 From: Chelsea Cassanova Date: Thu, 9 May 2024 11:08:29 -0700 Subject: [PATCH] [lldb][breakpoint] Grey out disabled breakpoints This commit adds colour settings to the list of breakpoints in order to grey out breakpoints that have been disabled. --- lldb/include/lldb/API/SBStream.h | 5 + lldb/include/lldb/Utility/Stream.h| 8 lldb/source/API/SBStream.cpp | 10 ++ lldb/source/Breakpoint/Breakpoint.cpp | 9 + lldb/source/Utility/Stream.cpp| 8 5 files changed, 40 insertions(+) diff --git a/lldb/include/lldb/API/SBStream.h b/lldb/include/lldb/API/SBStream.h index 0e33f05b69916..108ddc38b4028 100644 --- a/lldb/include/lldb/API/SBStream.h +++ b/lldb/include/lldb/API/SBStream.h @@ -12,6 +12,7 @@ #include #include "lldb/API/SBDefines.h" +#include "llvm/ADT/StringRef.h" namespace lldb { @@ -43,6 +44,10 @@ class LLDB_API SBStream { void Print(const char *str); + bool HasColor(); + + void FormatAnsiTerminalCodes(llvm::StringRef format); + void RedirectToFile(const char *path, bool append); void RedirectToFile(lldb::SBFile file); diff --git a/lldb/include/lldb/Utility/Stream.h b/lldb/include/lldb/Utility/Stream.h index 37bcdc9924171..1ab590202cd69 100644 --- a/lldb/include/lldb/Utility/Stream.h +++ b/lldb/include/lldb/Utility/Stream.h @@ -309,6 +309,12 @@ class Stream { /// The current indentation level. unsigned GetIndentLevel() const; + /// Whether or not the stream is using color. + /// + /// \return + /// The color setting of the stream. + bool HasColor(); + /// Indent the current line in the stream. /// /// Indent the current line using the current indentation level and print an @@ -366,6 +372,8 @@ class Stream { /// The optional C string format that can be overridden. void QuotedCString(const char *cstr, const char *format = "\"%s\""); + void FormatAnsiTerminalCodes(llvm::StringRef format); + /// Set the address size in bytes. /// /// \param[in] addr_size diff --git a/lldb/source/API/SBStream.cpp b/lldb/source/API/SBStream.cpp index fc8f09a7bb9ae..bc0f3356d4753 100644 --- a/lldb/source/API/SBStream.cpp +++ b/lldb/source/API/SBStream.cpp @@ -11,6 +11,7 @@ #include "lldb/API/SBFile.h" #include "lldb/Host/FileSystem.h" #include "lldb/Host/StreamFile.h" +#include "lldb/Utility/AnsiTerminal.h" #include "lldb/Utility/Instrumentation.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Status.h" @@ -77,6 +78,15 @@ void SBStream::Printf(const char *format, ...) { va_end(args); } +bool SBStream::HasColor() { + return m_opaque_up->AsRawOstream().colors_enabled(); +} + +void SBStream::FormatAnsiTerminalCodes(llvm::StringRef format) { + if (HasColor()) +Printf("%s", ansi::FormatAnsiTerminalCodes(format).c_str()); +} + void SBStream::RedirectToFile(const char *path, bool append) { LLDB_INSTRUMENT_VA(this, path, append); diff --git a/lldb/source/Breakpoint/Breakpoint.cpp b/lldb/source/Breakpoint/Breakpoint.cpp index ae845e92762b9..95624f4ae3ad5 100644 --- a/lldb/source/Breakpoint/Breakpoint.cpp +++ b/lldb/source/Breakpoint/Breakpoint.cpp @@ -15,6 +15,7 @@ #include "lldb/Breakpoint/BreakpointResolver.h" #include "lldb/Breakpoint/BreakpointResolverFileLine.h" #include "lldb/Core/Address.h" +#include "lldb/Core/Debugger.h" #include "lldb/Core/Module.h" #include "lldb/Core/ModuleList.h" #include "lldb/Core/SearchFilter.h" @@ -26,6 +27,7 @@ #include "lldb/Target/SectionLoadList.h" #include "lldb/Target/Target.h" #include "lldb/Target/ThreadSpec.h" +#include "lldb/Utility/AnsiTerminal.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" #include "lldb/Utility/Stream.h" @@ -837,6 +839,10 @@ void Breakpoint::GetDescription(Stream *s, lldb::DescriptionLevel level, bool show_locations) { assert(s != nullptr); + // Grey out any disabled breakpoints in the list of breakpoints. + if (!IsEnabled()) +s->FormatAnsiTerminalCodes("${ansi.faint}"); + if (!m_kind_description.empty()) { if (level == eDescriptionLevelBrief) { s->PutCString(GetBreakpointKind()); @@ -933,6 +939,9 @@ void Breakpoint::GetDescription(Stream *s, lldb::DescriptionLevel level, } s->IndentLess(); } + + // Reset the colors back to normal if they were previously greyed out. + s->FormatAnsiTerminalCodes("${ansi.normal}"); } void Breakpoint::GetResolverDescription(Stream *s) { diff --git a/lldb/source/Utility/Stream.cpp b/lldb/source/Utility/Stream.cpp index 89dce9fb0e1f7..e4ca9ad5a1f14 100644 --- a/lldb/source/Utility/Stream.cpp +++ b/lldb/source/Utility/Stream.cpp @@ -103,6 +103,11 @@ void Stream::QuotedCString(const char *cstr, const char *format) { Printf(format, cstr); } +void Stream::FormatAnsiTerminalCodes(llvm::Stri
[Lldb-commits] [lldb] [lldb][breakpoint] Grey out disabled breakpoints (PR #91404)
https://github.com/JDevlieghere commented: This is looking good, but needs a (pexpect) test. I would also like to add a setting to make the prefix and suffix configurable, like we do for things like `show-progress-ansi-prefix` and `show-autosuggestion-ansi-prefix`. That way user can pick their own preferred color, or even just turn this one feature off without having to disable color if they want to. https://github.com/llvm/llvm-project/pull/91404 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][breakpoint] Grey out disabled breakpoints (PR #91404)
chelcassanova wrote: Just making sure, a setting for changing the prefix/suffix would be in `CoreProperties.td` right? https://github.com/llvm/llvm-project/pull/91404 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][breakpoint] Grey out disabled breakpoints (PR #91404)
https://github.com/chelcassanova edited https://github.com/llvm/llvm-project/pull/91404 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][breakpoint] Grey out disabled breakpoints (PR #91404)
https://github.com/chelcassanova edited https://github.com/llvm/llvm-project/pull/91404 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][breakpoint] Grey out disabled breakpoints (PR #91404)
@@ -848,6 +850,13 @@ void Breakpoint::GetDescription(Stream *s, lldb::DescriptionLevel level, const size_t num_locations = GetNumLocations(); const size_t num_resolved_locations = GetNumResolvedLocations(); + // Grey out any disabled breakpoints in the list of breakpoints. + if (GetTarget().GetDebugger().GetUseColor()) +s->Printf("%s", + IsEnabled() + ? ansi::FormatAnsiTerminalCodes("${ansi.normal}").c_str() + : ansi::FormatAnsiTerminalCodes("${ansi.faint}").c_str()); JDevlieghere wrote: You need to reset the color after printing, otherwise everything in the terminal printed after will be faint. Also seems like you can simplify this by moving the check for `IsEnabled()` into the first `if`: ``` const bool print_faint = !IsEnabled() && GetTarget().GetDebugger().GetUseColor(); if (print_faint) s->Print(ansi::FormatAnsiTerminalCodes("${ansi.faint}")); [...] if (print_faint) s->Print(ansi::FormatAnsiTerminalCodes("${ansi.reset}")); ``` https://github.com/llvm/llvm-project/pull/91404 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][breakpoint] Grey out disabled breakpoints (PR #91404)
@@ -848,6 +850,13 @@ void Breakpoint::GetDescription(Stream *s, lldb::DescriptionLevel level, const size_t num_locations = GetNumLocations(); const size_t num_resolved_locations = GetNumResolvedLocations(); + // Grey out any disabled breakpoints in the list of breakpoints. + if (GetTarget().GetDebugger().GetUseColor()) +s->Printf("%s", + IsEnabled() + ? ansi::FormatAnsiTerminalCodes("${ansi.normal}").c_str() + : ansi::FormatAnsiTerminalCodes("${ansi.faint}").c_str()); chelcassanova wrote: ![image](https://github.com/llvm/llvm-project/assets/21184907/0a95ccc6-bf9a-4acb-b8e0-2746d5911f15) I also thought that I'd have to reset the colours back to normal somewhere but when I tried this for a little the colours seemed fine without explicitly placing a reset somewhere. I can still add a reset alongside this simplified logic though. https://github.com/llvm/llvm-project/pull/91404 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][breakpoint] Grey out disabled breakpoints (PR #91404)
https://github.com/chelcassanova updated https://github.com/llvm/llvm-project/pull/91404 >From 0e45adeac968aa435f58dfef026ef308e56b40a5 Mon Sep 17 00:00:00 2001 From: Chelsea Cassanova Date: Thu, 9 May 2024 11:08:29 -0700 Subject: [PATCH] [lldb][breakpoint] Grey out disabled breakpoints This commit adds colour settings to the list of breakpoints in order to grey out breakpoints that have been disabled. --- lldb/source/Breakpoint/Breakpoint.cpp | 12 1 file changed, 12 insertions(+) diff --git a/lldb/source/Breakpoint/Breakpoint.cpp b/lldb/source/Breakpoint/Breakpoint.cpp index ae845e92762b9..8d28c5f1873e1 100644 --- a/lldb/source/Breakpoint/Breakpoint.cpp +++ b/lldb/source/Breakpoint/Breakpoint.cpp @@ -15,6 +15,7 @@ #include "lldb/Breakpoint/BreakpointResolver.h" #include "lldb/Breakpoint/BreakpointResolverFileLine.h" #include "lldb/Core/Address.h" +#include "lldb/Core/Debugger.h" #include "lldb/Core/Module.h" #include "lldb/Core/ModuleList.h" #include "lldb/Core/SearchFilter.h" @@ -26,6 +27,7 @@ #include "lldb/Target/SectionLoadList.h" #include "lldb/Target/Target.h" #include "lldb/Target/ThreadSpec.h" +#include "lldb/Utility/AnsiTerminal.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" #include "lldb/Utility/Stream.h" @@ -837,6 +839,12 @@ void Breakpoint::GetDescription(Stream *s, lldb::DescriptionLevel level, bool show_locations) { assert(s != nullptr); + // Grey out any disabled breakpoints in the list of breakpoints. + const bool print_faint = + (!IsEnabled() && GetTarget().GetDebugger().GetUseColor()); + if (print_faint) +s->Printf("%s", ansi::FormatAnsiTerminalCodes("${ansi.faint}").c_str()); + if (!m_kind_description.empty()) { if (level == eDescriptionLevelBrief) { s->PutCString(GetBreakpointKind()); @@ -933,6 +941,10 @@ void Breakpoint::GetDescription(Stream *s, lldb::DescriptionLevel level, } s->IndentLess(); } + + // Reset the colors back to normal if they were previously greyed out. + if (print_faint) +s->Printf("%s", ansi::FormatAnsiTerminalCodes("${ansi.normal}").c_str()); } void Breakpoint::GetResolverDescription(Stream *s) { ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][breakpoint] Grey out disabled breakpoints (PR #91404)
jimingham wrote: I'm not sure checking the debugger here resolves the difficulty I pointed out earlier. If I make an SBStream and call SBBreakpoint::GetDescription, I will have passed in an SBStream with `use_color` explicitly off. It still seems to me that should take precedence over the debugger's global setting. I wanted this for programmatic uses presumably, and shouldn't have to deal with color codes... Having to temporarily unset the global use color in the debugger to get this effect is not a good design. https://github.com/llvm/llvm-project/pull/91404 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][breakpoint] Grey out disabled breakpoints (PR #91404)
chelcassanova wrote: Hm, so in that case should we focus on adding an `SBStream::GetUseColor` so that the stream's colour settings can take precedence here? https://github.com/llvm/llvm-project/pull/91404 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][breakpoint] Grey out disabled breakpoints (PR #91404)
jimingham wrote: > Hm, so in that case should we focus on adding an `SBStream::GetUseColor` so > that the stream's colour settings can take precedence here? That's the only way it makes sense to me. But if we are going to rely on the stream, we also have to be sure that we're setting the Streams "use color" correctly when we make them. That might be an annoying accounting exercise. I think an easier way allow streams to both get their "use color" from context and also override it for special purposes is to replace the use color bool with a "use color, don't use color, no opinion" enum. We'd make streams creation default to "no opinion". The SB API's would instead create them with "no use color" - and we should add an accessor to the SB API's. And then internally, "no opinion" means "consult the debugger" - but more generally it means "determine my use color value from context". That saves you from having to track down all the cases where streams were made and get their "use color" right. https://github.com/llvm/llvm-project/pull/91404 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][breakpoint] Grey out disabled breakpoints (PR #91404)
chelcassanova wrote: Cool, sorry for the delay but I made changes to `Stream` and `SBStream` to add a `GetUseColor()` accessor and to change the bool it uses in initialization to a `std::optional` and I'm gonna put them up in a separate PR. https://github.com/llvm/llvm-project/pull/91404 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][breakpoint] Grey out disabled breakpoints (PR #91404)
JDevlieghere wrote: The stream already knows whether color support is enabled, but it looks like we're only storing that in the underlying llvm stream, which is commonly used in combination with `WithColor`. I think we could add a member to check if the stream has colors enabled. It's already a property of `m_forwarder` so we don't need to create a new member. ``` SBStream::HasColor() { return m_forwarded.has_color(); } ``` Alternatively, we could add a helper that does that under the hood. ``` SBStream::FormatAnsiTerminalCodes(llvm::StringRef format) { if (m_forwarded.has_color()) print(FormatAnsiTerminalCodes(format)); } ``` So instead of having to write: ``` if(s->HasColor()) s->Printf("%s", ansi::FormatAnsiTerminalCodes("${ansi.faint}").c_str()); ``` You can just write: ``` s-> FormatAnsiTerminalCodes("${ansi.faint}") ``` and have it do the right thing. ``` I think I understand Jim's concern. Most places that we use colors, we do that through `WithColor` which checks the color property of the underlying LLVM stream. Chelsea is not going through WithColor, so in her case she would indeed need to check https://github.com/llvm/llvm-project/pull/91404 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits