Page MenuHomePhabricator

Add "break delete --disabled" to delete all currently disabled breakpoints
ClosedPublic

Authored by jingham on Sep 22 2020, 7:12 PM.

Details

Summary

Sometimes in a debug session you set a whole bunch of breakpoints and gradually disable the ones that aren't helpful. At some point you want to clean up the breakpoint state before continuing, and getting rid of all those disabled breakpoints at one blow is a handy tool to have.

This patch adds "break delete --disabled" that deletes all the currently disabled breakpoints.

Diff Detail

Event Timeline

jingham created this revision.Sep 22 2020, 7:12 PM
Herald added a project: Restricted Project. · View Herald Transcript
jingham requested review of this revision.Sep 22 2020, 7:12 PM
jingham updated this revision to Diff 293796.Sep 23 2020, 10:35 AM

In the original version of the patch you couldn't supply any breakpoints along with the --disabled flag. But it makes sense to be able to have some breakpoints that don't get deleted by this operation, which you can achieve by passing excluded breakpoints, so:

(lldb) break delete --disabled DeleteMeNot

will delete all disabled breakpoints that don't have the name DeleteMeNot.

kastiglione accepted this revision.Sep 23 2020, 11:02 AM
kastiglione added a subscriber: kastiglione.

Nice improvement!

I think it's strange that break delete DeleteMeNot and break delete --disabled DeleteMeNot behave differently. For the case that it supports, I would suggest that people can run two commands: break enable DeleteMeNot followed by break delete --disabled.

lldb/source/Commands/CommandObjectBreakpoint.cpp
1475–1476

why is this added?

1480

does iterating over breakpoints.Breakpoints() require this to be non-const?

lldb/source/Commands/Options.td
232

To me, it's counter intuitive that break delete --disabled 1 will not delete bp 1.

This revision is now accepted and ready to land.Sep 23 2020, 11:02 AM
jingham marked an inline comment as done.EditedSep 23 2020, 11:30 AM

I agree that having --disable flip the meaning of the listed breakpoints is a little odd, but I think its the most useful meaning by far.

lldb/source/Commands/CommandObjectBreakpoint.cpp
1475–1476

I depend on the result being Success, but not all code paths actually use it. So I want to make sure it's in the Success state when I get started.

1480

Right.

lldb/source/Commands/Options.td
232

The combination:

(lldb) break delete --disabled 1

could either mean

  1. delete all breakpoints that are disabled AND breakpoint 1
  2. delete all breakpoints that are disabled EXCEPT breakpoint 1
  3. an error

Of those interpretations, 1 and 3 don't seem very useful, but 2 does.

This is particularly handy when you specify a breakpoint name, not a breakpoint. Just make breakpoints you don't want deleted DoNotDelete, then you can easily protect all those breakpoints.

Note, your workaround would only be useful in this case if all the breakpoints named DoNotDelete are currently disabled. Otherwise you would have to remember which of the DoNotDelete breakpoints were disabled, enable them all, do the delete --disabled then only re-disable those that were originally disabled. Whereas if you can pass an exclude list you can just protect those breakpoints unconditionally regardless of their state.

So while I agree this is a little odd, it's actually the only option that really makes sense, it's easy to document, and I don't think it's likely to cause mistakes.

This revision was landed with ongoing or failed builds.Sep 23 2020, 11:35 AM
This revision was automatically updated to reflect the committed changes.
jingham marked an inline comment as done.
kastiglione added inline comments.Sep 23 2020, 12:16 PM
lldb/source/Commands/Options.td
232

why does the first interpretation not seem useful? If I'm deleting breakpoints, I might want to delete both disabled breakpoints and one or more specific breakpoints. To do that I would probably intuitively write break delete --disabled OthersToDelete.

Could the ambiguity be removed by adding another flag? break delete --disabled --except DoNotDelete?

jingham added inline comments.Sep 23 2020, 12:32 PM
lldb/source/Commands/Options.td
232

To me "delete --disabled" is a bulk operation acting on a class of breakpoints. "This class plus one random other one" seems odd to me.

A bulk operation with exclusions makes much more sense to me.

Adding another option complicates things without adding much value, and becomes annoying if you want to specify more than one excluded thing. It would be easy to make the mistake:

(lldb) break disable --disabled --except 1 2

intending to preserve 2 but in fact deleting it.

kastiglione added inline comments.Sep 23 2020, 2:55 PM
lldb/source/Commands/Options.td
232

I get that exclusions are useful, my concern is that the command "breakpoint delete" doesn't delete what you give it. If break delete foo deletes foo, then on the surface break delete --disabled foo should also delete foo. The flag does what it says, but also silently inverts the meaning of the positional args.

jingham added inline comments.Sep 23 2020, 3:11 PM
lldb/source/Commands/Options.td
232

The help for the option explicitly says that it inverts the meaning of the positional args, there's nothing silent about it. You wouldn't accidentally say break delete --disabled, so presumably you would have to have read the help for the option, which I don't think is susceptible to misconstruction.

Because of that, I'm not too bothered that break delete --disabled Foo behaves differently from break delete Foo. And it seems the simplest way to express the most useful thing you would want to add to just`break delete --disable`.

kastiglione added inline comments.Sep 23 2020, 4:03 PM
lldb/source/Commands/Options.td
232

In my experience people learn about lldb through twitter/coworkers/blogs/talks/tutorials etc, and not through help. Of those who learn from help, they may not read every word. It's quite possible to use this flag without having read the fine print.

jingham added inline comments.Sep 23 2020, 4:21 PM
lldb/source/Commands/Options.td
232

Given that misusing this command+option would result in a breakpoint NOT getting deleted, I'm less concerned about the possibility of misuse. The reaction is "I did a somewhat odd thing and the odd bit I added didn't work as expected (in a non-destructive way) so maybe I should read the help". That doesn't seem problematic to me.

kastiglione added inline comments.Sep 24 2020, 10:44 AM
lldb/source/Commands/Options.td
232

👍