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

[jingham]

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.

[jingham]

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]

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.

[jingham]

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.

[kastiglione]

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]

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]

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]

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]

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]

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]

lldb/source/Commands/Options.td

232

👍

lldb/source/Commands/CommandObjectBreakpoint.cpp

CommandCompletions::InvokeCommonCompletionCallbacks(
         GetCommandInterpreter(), CommandCompletions::eBreakpointCompletion,
         request, nullptr);
   }
 
   Options *GetOptions() override { return &m_options; }
 
   class CommandOptions : public Options {
   public:
-    CommandOptions() : Options(), m_use_dummy(false), m_force(false) {}
+    CommandOptions() : Options(), m_use_dummy(false), m_force(false),
+      m_delete_disabled(false) {}
 
     ~CommandOptions() override = default;
 
     Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
                           ExecutionContext *execution_context) override {
       Status error;
       const int short_option = m_getopt_table[option_idx].val;
 
       switch (short_option) {
       case 'f':
         m_force = true;
         break;
 
       case 'D':
         m_use_dummy = true;
         break;
         
+      case 'd':
+        m_delete_disabled = true;
+        break;
+
       default:
         llvm_unreachable("Unimplemented option");
       }
 
       return error;
     }
 
     void OptionParsingStarting(ExecutionContext *execution_context) override {
       m_use_dummy = false;
       m_force = false;
+      m_delete_disabled = false;
     }
 
     llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
       return llvm::makeArrayRef(g_breakpoint_delete_options);
     }
 
     // Instance variables to hold the values for command options.
     bool m_use_dummy;
     bool m_force;
+    bool m_delete_disabled;
   };
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     Target &target = GetSelectedOrDummyTarget(m_options.m_use_dummy);
+    result.Clear();
     
     std::unique_lock<std::recursive_mutex> lock;
     target.GetBreakpointList().GetListMutex(lock);
 
-    const BreakpointList &breakpoints = target.GetBreakpointList();
+    BreakpointList &breakpoints = target.GetBreakpointList();
 
     size_t num_breakpoints = breakpoints.GetSize();
 
     if (num_breakpoints == 0) {
       result.AppendError("No breakpoints exist to be deleted.");
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
 
-    if (command.empty()) {
+    if (command.empty() && !m_options.m_delete_disabled) {
       if (!m_options.m_force &&
           !m_interpreter.Confirm(
               "About to delete all breakpoints, do you want to do that?",
               true)) {
         result.AppendMessage("Operation cancelled...");
       } else {
         target.RemoveAllowedBreakpoints();
         result.AppendMessageWithFormat(
             "All breakpoints removed. (%" PRIu64 " breakpoint%s)\n",
             (uint64_t)num_breakpoints, num_breakpoints > 1 ? "s" : "");
       }
       result.SetStatus(eReturnStatusSuccessFinishNoResult);
     } else {
       // Particular breakpoint selected; disable that breakpoint.
       BreakpointIDList valid_bp_ids;
+      
+      if (m_options.m_delete_disabled) {
+        if (!command.empty()) {
+          result.AppendError("Can't specify --disabled with breakpoint "
+                             "arguments.");
+          result.SetStatus(eReturnStatusFailed);
+          return false;
+        }
+        for (auto breakpoint_sp : breakpoints.Breakpoints()) {
+          if (!breakpoint_sp->IsEnabled() && breakpoint_sp->AllowDelete()) {
+            valid_bp_ids.AddBreakpointID(breakpoint_sp->GetID());
+          }
+        }
+        if (valid_bp_ids.GetSize() == 0) {
+          result.AppendError("No disabled breakpoints.");
+          result.SetStatus(eReturnStatusFailed);
+          return false;
+        }
+      } else {
         CommandObjectMultiwordBreakpoint::VerifyBreakpointOrLocationIDs(
             command, &target, result, &valid_bp_ids,
             BreakpointName::Permissions::PermissionKinds::deletePerm);
+      }
       
       if (result.Succeeded()) {
         int delete_count = 0;
         int disable_count = 0;
         const size_t count = valid_bp_ids.GetSize();
         for (size_t i = 0; i < count; ++i) {
           BreakpointID cur_bp_id = valid_bp_ids.GetBreakpointIDAtIndex(i);
 
           if (cur_bp_id.GetBreakpointID() != LLDB_INVALID_BREAK_ID) {

lldb/source/Commands/Options.td

 }
 
 let Command = "breakpoint delete" in {
   def breakpoint_delete_force : Option<"force", "f">, Group<1>,
     Desc<"Delete all breakpoints without querying for confirmation.">;
   def breakpoint_delete_dummy_breakpoints : Option<"dummy-breakpoints", "D">,
     Group<1>, Desc<"Delete Dummy breakpoints - i.e. breakpoints set before a "
     "file is provided, which prime new targets.">;
+  def breakpoint_delete_disabled : Option<"disabled", "d">, Group<1>,
+    Desc<"Delete all breakpoints which are currently disabled.">;
 }
 
 let Command = "breakpoint name" in {
   def breakpoint_name_name : Option<"name", "N">, Group<1>,
     Arg<"BreakpointName">, Desc<"Specifies a breakpoint name to use.">;
   def breakpoint_name_breakpoint_id : Option<"breakpoint-id", "B">, Group<2>,
     Arg<"BreakpointID">, Desc<"Specify a breakpoint ID to use.">;
   def breakpoint_name_dummy_breakpoints : Option<"dummy-breakpoints", "D">,
     Group<3>, Desc<"Operate on Dummy breakpoints - i.e. breakpoints set before "

lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py

def breakpoint_commands_on_creation(self):
         bkpt = target.FindBreakpointByID(1)
         self.assertTrue(bkpt.IsValid(), "Couldn't find breakpoint 1")
         com_list = lldb.SBStringList()
         bkpt.GetCommandLineCommands(com_list)
         self.assertEqual(com_list.GetSize(), 3, "Got the wrong number of commands")
         self.assertEqual(com_list.GetStringAtIndex(0), "bt", "First bt")
         self.assertEqual(com_list.GetStringAtIndex(1), "thread list", "Next thread list")
         self.assertEqual(com_list.GetStringAtIndex(2), "continue", "Last continue")
+
+    def test_breakpoint_delete_disabled(self):
+        """Test 'break delete --disabled' works"""
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+        target = self.dbg.CreateTarget(exe)
+        self.assertTrue(target.IsValid(), "Created an invalid target.")
+
+        bp_1 = target.BreakpointCreateByName("main")
+        bp_2 = target.BreakpointCreateByName("not_here")
+        bp_3 = target.BreakpointCreateByName("main")
+
+        bp_1.SetEnabled(False)
+        bp_3.SetEnabled(False)
+
+        bp_id_1 = bp_1.GetID()
+        bp_id_2 = bp_2.GetID()
+        bp_id_3 = bp_3.GetID()
+        
+        self.runCmd("breakpoint delete --disabled")
+
+        bp_1 = target.FindBreakpointByID(bp_id_1)
+        self.assertFalse(bp_1.IsValid(), "Didn't delete disabled breakpoint 1")
+
+        bp_2 = target.FindBreakpointByID(bp_id_2)
+        self.assertTrue(bp_2.IsValid(), "Deleted enabled breakpoint 2")
+
+        bp_3 = target.FindBreakpointByID(bp_id_3)
+        self.assertFalse(bp_3.IsValid(), "Didn't delete disabled breakpoint 3")