[lldb][breakpoint] Grey out disabled breakpoints #91404

chelcassanova · 2024-05-07T21:26:47Z

This commit adds colour settings to the list of breakpoints in order to
grey out breakpoints that have been disabled.

llvmbot · 2024-05-07T21:27:21Z

@llvm/pr-subscribers-lldb

Author: Chelsea Cassanova (chelcassanova)

Changes

This commit adds colour highlighting to the disabled keyword in the breakpoint list so that it appears red (and should be easier to see in a large sea of breakpoints).

Full diff: https://github.com/llvm/llvm-project/pull/91404.diff

2 Files Affected:

(modified) lldb/include/lldb/Breakpoint/BreakpointOptions.h (+6)
(modified) lldb/source/Breakpoint/BreakpointOptions.cpp (+2-1)

diff --git a/lldb/include/lldb/Breakpoint/BreakpointOptions.h b/lldb/include/lldb/Breakpoint/BreakpointOptions.h
index 7bf545717422f..3dc3a7190d03e 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointOptions.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointOptions.h
@@ -400,6 +400,12 @@ friend class Breakpoint;
   /// Which options are set at this level.
   /// Drawn from BreakpointOptions::SetOptionsFlags.
   Flags m_set_flags;
+  /// Settings that allow the 'disabled' keyword to be displayed in red.
+  Stream::HighlightSettings m_disbaled_breakpoint_highlight_settings{
+      "disabled",
+      "\x1b[31m",
+      "\x1b[37m",
+  };
 };
 
 } // namespace lldb_private
diff --git a/lldb/source/Breakpoint/BreakpointOptions.cpp b/lldb/source/Breakpoint/BreakpointOptions.cpp
index 6c6037dd9edd3..993de590b0d4f 100644
--- a/lldb/source/Breakpoint/BreakpointOptions.cpp
+++ b/lldb/source/Breakpoint/BreakpointOptions.cpp
@@ -534,7 +534,8 @@ void BreakpointOptions::GetDescription(Stream *s,
 
     if (m_ignore_count > 0)
       s->Printf("ignore: %d ", m_ignore_count);
-    s->Printf("%sabled ", m_enabled ? "en" : "dis");
+    s->PutCStringColorHighlighted(m_enabled ? "enabled " : "disabled ",
+                                  m_disbaled_breakpoint_highlight_settings);
 
     if (m_one_shot)
       s->Printf("one-shot ");

bulbazord · 2024-05-07T21:39:17Z

lldb/include/lldb/Breakpoint/BreakpointOptions.h

+  Stream::HighlightSettings m_disbaled_breakpoint_highlight_settings{
+      "disabled",
+      "\x1b[31m",
+      "\x1b[37m",


You don't need to hardcode these values yourself. Take a look at AnsiTerminal.h, there are some constants defined for this purpose.

bulbazord · 2024-05-07T21:39:40Z

lldb/include/lldb/Breakpoint/BreakpointOptions.h

@@ -400,6 +400,12 @@ friend class Breakpoint;
  /// Which options are set at this level.
  /// Drawn from BreakpointOptions::SetOptionsFlags.
  Flags m_set_flags;
+  /// Settings that allow the 'disabled' keyword to be displayed in red.
+  Stream::HighlightSettings m_disbaled_breakpoint_highlight_settings{


typo: disbaled -> disabled

JDevlieghere · 2024-05-07T21:50:55Z

lldb/source/Breakpoint/BreakpointOptions.cpp

+    s->PutCStringColorHighlighted(m_enabled ? "enabled " : "disabled ",
+                                  m_disbaled_breakpoint_highlight_settings);


This is going to regex search for disabled and then replace it with its colored variant. That seems unnecessarily inefficient. You know whether you're going to put enabled or disabled, so you could print it colored right away.

You might also have to check the debugger setting for whether colors are enabled (Debugger::GetUseColor()). Sometimes it's set at the stream level: the stream might have colors disabled if it was created somewhere were the setting was checked). I don't know if that's always going to be the case here. If it's not, we should check the setting here. To find out I would run lldb with --no-use-colors and see if the colors still get printed.

Ah, so using Printf with ansi::FormatAnsiTerminalCodes instead? And yes using this with --no-use-colors does show the colour here so it looks like this isn't being checked at the stream level.

Note, you can get this output both when lldb decides to print in the terminal, and when someone does:

stream = lldb.SBStream() bkpt.GetDescription(stream, eDescriptionLevelWhatever)

At present SBStream's are backed by StreamString objects which get constructed with use_color -> false. So you should be okay there.

It seems odd you can't ask the stream whether it is using colors or not. Because if I'm running with use-colors on, but I want to use the description of a breakpoint w/o colors - say to put it in a log - then I would correctly make an SBStream which is supposed to not use colors, and pass that to the description. If you ONLY check whether the debugger is using colors, and override the stream's setting, then you will do the wrong thing.

This means that only the debugger tells you whether or not you're using colours then? BreakpointOptions::GetDescription() doesn't seem to have a reference to the debugger, target or interpreter so I'm wondering what the next best options to determine if we're using colours is.

This wouldn't be a problem if we make sure that whoever makes the Streams for use by the command interpreter consults GetUseColor when it creates the stream, and then we can rely on the stream to tell us what to do. If you are calling stream API's that add the color for you, that should just work. If you are doing a Print by hand with the color codes or not, you should consult the stream. For that we'll have to add Stream::GetUseColor. If we always get that right, the only problem will be if there are persistent streams, we may need to change their state or replace them when GetUseColor changes.

That seems the most principled way to solve this.

I was speaking to Ismail about this and seems like it would be nice if we had a Stream::GetUseColor for this given that you can set whether or not the stream has colours at construction but you can't query the state of it after that.

medismailben · 2024-05-08T17:27:01Z

More of a styling preference, but I wouldn't use red as the color for disabled breakpoint, I think it would be more suited for unresolved ones (that could be done in a follow-up). Instead, I'd use the ANSI_CTRL_FAINT escape code to match what you can find in many IDEs.

chelcassanova · 2024-05-09T18:21:26Z

I changed this PR to make disabled breakpoints greyed out (per Ismail's suggestion). Since this is being done at the Breakpoint level we have access to the target and its debugger to access GetUseColor().

JDevlieghere · 2024-05-09T18:46:25Z

lldb/source/Breakpoint/Breakpoint.cpp

+  if (GetTarget().GetDebugger().GetUseColor())
+    s->Printf("%s",
+              IsEnabled()
+                  ? ansi::FormatAnsiTerminalCodes("${ansi.normal}").c_str()
+                  : ansi::FormatAnsiTerminalCodes("${ansi.faint}").c_str());


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}"));

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.

jimingham · 2024-05-10T16:24:59Z

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.

chelcassanova · 2024-05-10T23:07:02Z

Hm, so in that case should we focus on adding an SBStream::GetUseColor so that the stream's colour settings can take precedence here?

jimingham · 2024-05-11T02:27:02Z

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 to 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".

Actually, I'm being old-fashioned, the modern way to do this is convert the bool to an optional. I like the no opinion in an enum because it says explicitly what the unset state means...

Anyway, that way saves you from having to track down all the cases where streams were made and get their "use color" right. Instead, you only need to check at the places where you were intending to add color, obeying the definite settings or whatever your context is.

chelcassanova · 2024-05-16T19:29:22Z

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.

JDevlieghere · 2024-05-16T21:39:23Z

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.

JDevlieghere

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.

chelcassanova · 2024-07-11T19:19:52Z

Just making sure, a setting for changing the prefix/suffix would be in CoreProperties.td right?

github-actions · 2024-10-31T16:40:17Z

✅ With the latest revision this PR passed the Python code formatter.

labath · 2024-11-04T09:00:21Z

lldb/source/API/SBStream.cpp

+
+void SBStream::FormatAnsiTerminalCodes(llvm::StringRef format) {
+  if (HasColor())
+    Printf("%s", ansi::FormatAnsiTerminalCodes(format).c_str());


Suggested change

Printf("%s", ansi::FormatAnsiTerminalCodes(format).c_str());

m_opaque_up->PutCString(ansi::FormatAnsiTerminalCodes(format));

labath · 2024-11-04T09:05:05Z

lldb/test/API/terminal/TestDisabledBreakpoints.py

+        self.child = pexpect.spawn("expect", encoding="utf-8")
+


This line is probably unnecessary, I was modeling this test after another pexpect test that spawns pexpect with the expect program but I don't think this tests needs that now that I'm looking at this again, I can remove this.

I'm pretty sure this was doing nothing as self.launch overwrites the child member. I recall we have one test which uses pexpect (the python module) to drive expect (the unix utility which lent its name to the former) to drive lldb. We definitely don't need that here (and tbh, I'm not sure the original test does either).

labath · 2024-11-04T09:06:20Z

lldb/test/API/terminal/TestDisabledBreakpoints.py

+        ansi_red_color_code = "\x1b[31m"
+
+        self.launch(use_colors=True, dimensions=(100, 100))
+        self.child.sendline(


Any reason you can't use self.expect instead?

Even if you don't pass it any expectations to it, that function will at least check for the lldb prompt, which means it will wait for lldb to process the previous command before you feed it the next one. That makes the test much more deterministic and reliable.

If I'm understanding correctly, you mean using self.expect instead of self.child.sendline to set up the breakpoints here? If so, I can change this here, I didn't know that self.expect checked for the prompt as well. I was originally using sendline and checking each output with expect instead.

Yes, that's what I meant. The expect function was meant to be used to inject lldb commands, some tests use the pexpect API directly because they're doing something more complicated (or because the thing they're driving is not lldb), but that's not the case here.

labath

I'm kinda late to the party (particularly as there's an analogous "progress" setting), but I have to say I was surprised to see a disable-breakpoint-ansi-suffix setting. That sounds like too low level of a model. The model I would expect/find more natural is if there was a single setting describing the "normal" text colour, and then several (not necessarily one each item that needs colouring) settings defining the colour of items that need colouring.

I find it hard to imagine why one would ever want to set different values for the "suffix" colours, as that means the colour of random text would depend on what was printed before it.

I don't want to make this a blocking objection, but I would urge you to consider if this is the behavior you want.

labath · 2024-11-05T12:59:49Z

lldb/include/lldb/API/SBStream.h

@@ -47,6 +48,10 @@ class LLDB_API SBStream {

  void Print(const char *str);

+  bool HasColor();
+
+  void FormatAnsiTerminalCodes(llvm::StringRef format);


You can't have a StringRef in the SB API. This needs to be a const char *. All the internal APIs can and should use StringRefs though.

labath · 2024-11-05T13:10:21Z

lldb/test/API/terminal/TestDisabledBreakpoints.py

+        self.expect("br l")
+        self.child.expect_exact(ansi_red_color_code + "1:")


Suggested change

self.expect("br l")

self.child.expect_exact(ansi_red_color_code + "1:")

self.expect("br l", substrs=[ansi_red_color_code + "1:"])

The idea is to make this mostly look like the existing SB API tests.

Kinda surprised it works without it. If I had to guess, I'd say its because of the prompt coloring code causes the prompt substring to be present more than once (which then means the substring search loses synch).

(That's one more reason to add some sort of expectations to the previous commands. The other reason is that they give you an earlier indication if something goes wrong during the test -- the pexpect error messages aren't of the most readable type)

Thanks for the suggestion to use substrs here! By "expectations" you just mean adding a self.expect after all the previous commands to check that their output is fine right? If so then yeah I can add that 👍🏾

What I meant was adding a substr argument to the existing expect calls (e.g. self.expect("br dis", substrs=["All breakpoints disabled."])), not adding new expect calls.

labath · 2024-11-05T13:15:07Z

lldb/test/API/terminal/TestDisabledBreakpoints.py

+        self.child = pexpect.spawn("expect", encoding="utf-8")
+


I'm pretty sure this was doing nothing as self.launch overwrites the child member. I recall we have one test which uses pexpect (the python module) to drive expect (the unix utility which lent its name to the former) to drive lldb. We definitely don't need that here (and tbh, I'm not sure the original test does either).

labath · 2024-11-05T13:16:48Z

lldb/test/API/terminal/TestDisabledBreakpoints.py

+        ansi_red_color_code = "\x1b[31m"
+
+        self.launch(use_colors=True, dimensions=(100, 100))
+        self.child.sendline(


Yes, that's what I meant. The expect function was meant to be used to inject lldb commands, some tests use the pexpect API directly because they're doing something more complicated (or because the thing they're driving is not lldb), but that's not the case here.

chelcassanova · 2024-11-11T19:30:49Z

I agree that those who want to change their colours in LLDB shouldn't have to touch the ANSI suffix setting in order to do this. We do use settings for changing the suffix in other places in LLDB, so if we were change how this setting works then I think it's outside of the scope of this patch and it would be better as a larger refactor to the colour settings.

chelcassanova · 2024-11-11T19:33:08Z

I also agree with how I was misusing the pexpect module here, I was way overcomplicating things there by trying to model my test after the one that drives the expect program instead of lldb itself 😅

JDevlieghere

I like the feautre but I think this PR can be a lot simpler:

I don't think you need any of the changes to Stream or SBStream. You already have to get the setting from the debugger, so just check if it has colors enabled and write the prefix (and fixed suffix) if colors are enabled.
I think we had settled on adding a new setting for highlighting "disabled things", with breakpoints being just one example of that. The setting should be generic, something like disable-ansi-prefix (*). I'm find with omitting the suffix and always issuing a clear. I can't think of a reason anyone would ever want to change that.

(*) I would prefer to call the settingdisable-format, but then it should go through FormatEntity which is definitely overkill. I think given the existing settings, disable-ansi-prefix is the most consistent.

JDevlieghere · 2025-05-09T23:54:11Z

lldb/include/lldb/API/SBStream.h

+  bool HasColor();
+
+  void FormatAnsiTerminalCodes(llvm::StringRef format);


IIUC we don't need any changes to the SBStream class anymore, right?

Probably not, though does that now mean that I'd just be writing the prefix/suffix with something like Stream::Printf now instead of the format function here?

chelcassanova · 2025-06-13T23:58:44Z

Just updated this patch to:

Remove changes to SBStream and Stream
Change the setting name to disable-ansi-prefix

lldb/source/Breakpoint/Breakpoint.cpp

github-actions · 2025-06-16T16:24:15Z

✅ With the latest revision this PR passed the C/C++ code formatter.

bulbazord · 2025-06-16T20:10:26Z

lldb/source/Breakpoint/Breakpoint.cpp

@@ -838,6 +840,14 @@ 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 highlight_disabled =


Suggestion: rename this variable to "dim_breakpoint_description" or something like that.
The word "highlight_disabled" could also be applied to situations where colors are not enabled.

bulbazord · 2025-06-16T20:10:47Z

lldb/source/Breakpoint/Breakpoint.cpp

@@ -838,6 +840,14 @@ void Breakpoint::GetDescription(Stream *s, lldb::DescriptionLevel level,
                                bool show_locations) {
  assert(s != nullptr);

+  // Grey out any disabled breakpoints in the list of breakpoints.


nit: I suggest removing the comment. I think naming the conditions variable better should make this clearer.

Just applied both of these suggestions to the patch.

This commit adds colour settings to the list of breakpoints in order to grey out breakpoints that have been disabled.

chelcassanova requested review from bulbazord and medismailben May 7, 2024 21:26

chelcassanova requested a review from JDevlieghere as a code owner May 7, 2024 21:26

llvmbot added the lldb label May 7, 2024

bulbazord requested changes May 7, 2024

View reviewed changes

JDevlieghere reviewed May 7, 2024

View reviewed changes

chelcassanova force-pushed the make-disabled-breakpoint-keyword-red branch from 4458e95 to c334f73 Compare May 9, 2024 18:18

chelcassanova changed the title ~~[lldb][breakpointoptions] Make disabled keyword red~~ [lldb][breakpoint] Grey out disabled breakpoints May 9, 2024

JDevlieghere reviewed May 9, 2024

View reviewed changes

chelcassanova force-pushed the make-disabled-breakpoint-keyword-red branch from c334f73 to 0e45ade Compare May 10, 2024 00:05

chelcassanova force-pushed the make-disabled-breakpoint-keyword-red branch from 0e45ade to 5724d6c Compare June 18, 2024 20:52

chelcassanova requested a review from JDevlieghere June 18, 2024 21:57

JDevlieghere reviewed Jul 11, 2024

View reviewed changes

chelcassanova force-pushed the make-disabled-breakpoint-keyword-red branch 2 times, most recently from 895f29d to 950386a Compare October 31, 2024 16:36

chelcassanova requested review from bulbazord, jimingham and JDevlieghere October 31, 2024 17:08

chelcassanova force-pushed the make-disabled-breakpoint-keyword-red branch from 950386a to c4ee8ba Compare October 31, 2024 17:29

labath reviewed Nov 4, 2024

View reviewed changes

chelcassanova force-pushed the make-disabled-breakpoint-keyword-red branch 2 times, most recently from c683d86 to c7c554e Compare November 4, 2024 21:38

labath reviewed Nov 5, 2024

View reviewed changes

chelcassanova force-pushed the make-disabled-breakpoint-keyword-red branch from c7c554e to 4fee5ee Compare April 26, 2025 02:43

chelcassanova requested a review from labath April 26, 2025 02:43

JDevlieghere requested changes May 10, 2025

View reviewed changes

chelcassanova force-pushed the make-disabled-breakpoint-keyword-red branch from 4fee5ee to 8608a94 Compare June 13, 2025 23:56

JDevlieghere reviewed Jun 16, 2025

View reviewed changes

lldb/source/Breakpoint/Breakpoint.cpp Outdated Show resolved Hide resolved

lldb/source/Breakpoint/Breakpoint.cpp Outdated Show resolved Hide resolved

chelcassanova force-pushed the make-disabled-breakpoint-keyword-red branch 2 times, most recently from 75061e1 to ab03eca Compare June 16, 2025 17:13

bulbazord reviewed Jun 16, 2025

View reviewed changes

[lldb][breakpoint] Grey out disabled breakpoints

dd94ef4

This commit adds colour settings to the list of breakpoints in order to grey out breakpoints that have been disabled.

chelcassanova force-pushed the make-disabled-breakpoint-keyword-red branch from ab03eca to dd94ef4 Compare June 16, 2025 20:58

JDevlieghere approved these changes Jun 18, 2025

View reviewed changes

chelcassanova merged commit a630ca6 into llvm:main Jun 18, 2025
7 checks passed

		s->PutCStringColorHighlighted(m_enabled ? "enabled " : "disabled ",
		m_disbaled_breakpoint_highlight_settings);

	Printf("%s", ansi::FormatAnsiTerminalCodes(format).c_str());
	m_opaque_up->PutCString(ansi::FormatAnsiTerminalCodes(format));

		self.expect("br l")
		self.child.expect_exact(ansi_red_color_code + "1:")

	self.expect("br l")
	self.child.expect_exact(ansi_red_color_code + "1:")
	self.expect("br l", substrs=[ansi_red_color_code + "1:"])

		bool HasColor();

		void FormatAnsiTerminalCodes(llvm::StringRef format);

[lldb][breakpoint] Grey out disabled breakpoints #91404

[lldb][breakpoint] Grey out disabled breakpoints #91404

Uh oh!

Conversation

chelcassanova commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medismailben commented May 8, 2024

Uh oh!

chelcassanova commented May 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimingham commented May 10, 2024

Uh oh!

chelcassanova commented May 10, 2024

Uh oh!

jimingham commented May 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chelcassanova commented May 16, 2024

Uh oh!

JDevlieghere commented May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JDevlieghere left a comment

Choose a reason for hiding this comment

Uh oh!

chelcassanova commented Jul 11, 2024

Uh oh!

github-actions bot commented Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labath left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chelcassanova commented May 7, 2024 •

edited

Loading

jimingham commented May 11, 2024 •

edited

Loading

JDevlieghere commented May 16, 2024 •

edited

Loading

github-actions bot commented Oct 31, 2024 •

edited

Loading

github-actions bot commented Jun 16, 2025 •

edited

Loading