[lldb] Add tests for command removal

[wallace]

Summary

This adds some tests for removing lldb commands, user command and aliases. I've done in the SB API side for comleteness of this features. Besides that, I found a bug in the creation of the DummyCommand used in the test file: they were being allocated in the stack but they should have been allocated in the heap because CommandPluginInterfaceImplementation ends up owning the pointer passed to it, which is internally managed by a shared_ptr.

[jingham]

What happens if you remove a command that had an alias bound to it and then call the alias? We should probably add a test for this, it should produce some reasonable error... Probably need to test both the regex & regular aliases as those manage their dispatch differently.

[wallace]

The alias still works because it still holds a reference to it. I could add that as a test

[jingham]

In D149379#4331059, @wallace wrote:

The alias still works because it still holds a reference to it. I could add that as a test

The alias works but the original command is gone? Interesting. We should definitely test that that works correctly. We should also test what happens when a regex command dispatches to a command that has been deleted.

Also, should the "delete" operation fail or return an error if the actual command is still made available because of an alias. If your intention was to remove a built-in command (still not sure why that is an operation that makes sense, but that aside...) then you haven't if you can still get to it through an alias. Should we tell the user that?

TTTT, removing user commands is cosmetic if you don't remove script as well, since there's pretty much nothing that can be done on the command line that can't be done with scripts. So I don't have a sense for how strict you are trying to be. But if I can defeat the removal of a command by putting an alias in my .lldbinit, you are being very not strict...

lldb/include/lldb/API/SBCommandInterpreter.h

316

You should probably say explicitly what the "removable status" of a built-in command is. You (correctly) have to explicitly pass force=true for that to work but it would be good to state that explicitly. We should also emphasize here there isn't any way to get a built-in command back once you've deleted it.

lldb/include/lldb/API/SBCommandInterpreter.h

#endif
 
   int HandleCompletionWithDescriptions(const char *current_line,
                                        uint32_t cursor_pos,
                                        int match_start_point,
                                        int max_return_elements,
                                        lldb::SBStringList &matches,
                                        lldb::SBStringList &descriptions);
 
-  /// Returns whether an interrupt flag was raised either by the SBDebugger - 
+  /// Returns whether an interrupt flag was raised either by the SBDebugger -
   /// when the function is not running on the RunCommandInterpreter thread, or
   /// by SBCommandInterpreter::InterruptCommand if it is.  If your code is doing
-  /// interruptible work, check this API periodically, and interrupt if it 
+  /// interruptible work, check this API periodically, and interrupt if it
   /// returns true.
   bool WasInterrupted() const;
-  
   /// Interrupts the command currently executing in the RunCommandInterpreter
   /// thread.
   ///
   /// \return
   ///   \b true if there was a command in progress to recieve the interrupt.
   ///   \b false if there's no command currently in flight.
   bool InterruptCommand();
 
#endif
   /// 'quit' command. Returns 0 if the user hasn't called 'quit' at all or
   /// without a custom exit code.
   int GetQuitStatus();
 
   /// Resolve the command just as HandleCommand would, expanding abbreviations
   /// and aliases.  If successful, result->GetOutput has the full expansion.
   void ResolveCommand(const char *command_line, SBCommandReturnObject &result);
 
+  /// Remove a command if it is removable (python or regex command). If \b force
+  /// is provided, the command is removed regardless of its removable status.
+  ///
+  /// \return
+  ///   \b true if and only if the command was effectively removed.
+  bool RemoveCommand(const char *command, bool force = false);
+
+  /// Remove a user-defined command.
+  ///
+  /// \return
+  ///   \b true if and only if the command was effectively removed.
+  bool RemoveUserCommand(const char *command);
+
+  /// Remove an alias command.
+  ///
+  /// \return
+  ///   \b true if and only if the alias was effectively removed.
+  bool RemoveAlias(const char *alias);
+
 protected:
   lldb_private::CommandInterpreter &ref();
 
   lldb_private::CommandInterpreter *get();
 
   void reset(lldb_private::CommandInterpreter *);
 
 private:

lldb/source/API/SBCommandInterpreter.cpp

 
 bool SBCommandInterpreter::AliasExists(const char *cmd) {
   LLDB_INSTRUMENT_VA(this, cmd);
 
   return (((cmd != nullptr) && IsValid()) ? m_opaque_ptr->AliasExists(cmd)
                                           : false);
 }
 
+bool SBCommandInterpreter::RemoveCommand(const char *command, bool force) {
+  LLDB_INSTRUMENT_VA(this, command, force);
+
+  if (!IsValid())
+    return false;
+  return m_opaque_ptr->RemoveCommand(command, force);
+}
+
+bool SBCommandInterpreter::RemoveUserCommand(const char *command) {
+  LLDB_INSTRUMENT_VA(this, command);
+
+  if (!IsValid())
+    return false;
+  return m_opaque_ptr->RemoveUser(command);
+}
+
+bool SBCommandInterpreter::RemoveAlias(const char *alias) {
+  LLDB_INSTRUMENT_VA(this, alias);
+
+  if (!IsValid())
+    return false;
+  return m_opaque_ptr->RemoveAlias(alias);
+}
+
 bool SBCommandInterpreter::IsActive() {
   LLDB_INSTRUMENT_VA(this);
 
   return (IsValid() ? m_opaque_ptr->IsActive() : false);
 }
 
 bool SBCommandInterpreter::WasInterrupted() const {
   LLDB_INSTRUMENT_VA(this);
 
   return (IsValid() ? m_opaque_ptr->GetDebugger().InterruptRequested() : false);
 }
 
 bool SBCommandInterpreter::InterruptCommand() {
   LLDB_INSTRUMENT_VA(this);
-  
   return (IsValid() ? m_opaque_ptr->InterruptCommand() : false);
 }
 
 const char *SBCommandInterpreter::GetIOHandlerControlSequence(char ch) {
   LLDB_INSTRUMENT_VA(this, ch);
 
   return (IsValid()
               ? m_opaque_ptr->GetDebugger()

lldb/unittests/API/SBCommandInterpreterTest.cpp

bool DoExecute(SBDebugger dbg, char **command,
     result.SetStatus(eReturnStatusSuccessFinishResult);
     return result.Succeeded();
   }
 
 private:
   std::string m_message;
 };
 
+TEST_F(SBCommandInterpreterTest, RemoveCommandsAndAliases) {
+  // We first test removing non-existent commands
+  EXPECT_FALSE(m_interp.RemoveUserCommand("non-existent-user-command"));
+  EXPECT_FALSE(m_interp.RemoveCommand("non-existent-command"));
+  EXPECT_FALSE(m_interp.RemoveAlias("non-existent-alias"));
+
+  // We now test removing a built-in command
+  EXPECT_TRUE(m_interp.CommandExists("target"));
+  EXPECT_FALSE(m_interp.RemoveCommand("target"));
+  // Built-in commands need to be force removed.
+  EXPECT_TRUE(m_interp.RemoveCommand("target", /*force=*/true));
+  EXPECT_FALSE(m_interp.CommandExists("target"));
+
+  // Finally we test with a user defined command
+  m_interp.AddCommand("dummy", new DummyCommand("A dummy command"),
+                      /*help=*/nullptr);
+  EXPECT_TRUE(m_interp.UserCommandExists("dummy"));
+  EXPECT_TRUE(m_interp.RemoveUserCommand("dummy"));
+  EXPECT_FALSE(m_interp.UserCommandExists("dummy"));
+
+  // Now we remove an alias
+  EXPECT_TRUE(m_interp.AliasExists("b"));
+  EXPECT_TRUE(m_interp.RemoveAlias("b"));
+  EXPECT_FALSE(m_interp.AliasExists("b"));
+}
+
 TEST_F(SBCommandInterpreterTest, SingleWordCommand) {
   // We first test a command without autorepeat
-  DummyCommand dummy("It worked");
-  m_interp.AddCommand("dummy", &dummy, /*help=*/nullptr);
+  m_interp.AddCommand("dummy", new DummyCommand("Dummy command"),
+                      /*help=*/nullptr);
   {
     SBCommandReturnObject result;
     m_interp.HandleCommand("dummy", result, /*add_to_history=*/true);
     EXPECT_TRUE(result.Succeeded());
-    EXPECT_STREQ(result.GetOutput(), "It worked\n");
+    EXPECT_STREQ(result.GetOutput(), "Dummy command\n");
   }
   {
     SBCommandReturnObject result;
     m_interp.HandleCommand("", result);
     EXPECT_FALSE(result.Succeeded());
     EXPECT_STREQ(result.GetError(), "error: No auto repeat.\n");
   }
 
   // Now we test a command with autorepeat
-  m_interp.AddCommand("dummy_with_autorepeat", &dummy, /*help=*/nullptr,
+  m_interp.AddCommand("dummy_with_autorepeat",
+                      new DummyCommand("Command with autorepeat"),
+                      /*help=*/nullptr,
                       /*syntax=*/nullptr, /*auto_repeat_command=*/nullptr);
   {
     SBCommandReturnObject result;
     m_interp.HandleCommand("dummy_with_autorepeat", result,
                            /*add_to_history=*/true);
     EXPECT_TRUE(result.Succeeded());
-    EXPECT_STREQ(result.GetOutput(), "It worked\n");
+    EXPECT_STREQ(result.GetOutput(), "Command with autorepeat\n");
   }
   {
     SBCommandReturnObject result;
     m_interp.HandleCommand("", result);
     EXPECT_TRUE(result.Succeeded());
-    EXPECT_STREQ(result.GetOutput(), "It worked\n");
+    EXPECT_STREQ(result.GetOutput(), "Command with autorepeat\n");
   }
 }
 
 TEST_F(SBCommandInterpreterTest, MultiWordCommand) {
   auto command = m_interp.AddMultiwordCommand("multicommand", /*help=*/nullptr);
   // We first test a subcommand without autorepeat
-  DummyCommand subcommand("It worked again");
-  command.AddCommand("subcommand", &subcommand, /*help=*/nullptr);
+  command.AddCommand("subcommand", new DummyCommand("Dummy command"),
+                     /*help=*/nullptr);
   {
     SBCommandReturnObject result;
     m_interp.HandleCommand("multicommand subcommand", result,
                            /*add_to_history=*/true);
     EXPECT_TRUE(result.Succeeded());
-    EXPECT_STREQ(result.GetOutput(), "It worked again\n");
+    EXPECT_STREQ(result.GetOutput(), "Dummy command\n");
   }
   {
     SBCommandReturnObject result;
     m_interp.HandleCommand("", result);
     EXPECT_FALSE(result.Succeeded());
     EXPECT_STREQ(result.GetError(), "error: No auto repeat.\n");
   }
 
   // We first test a subcommand with autorepeat
-  command.AddCommand("subcommand_with_autorepeat", &subcommand,
+  command.AddCommand("subcommand_with_autorepeat",
+                     new DummyCommand("Dummy command with autorepeat"),
                      /*help=*/nullptr, /*syntax=*/nullptr,
                      /*auto_repeat_command=*/nullptr);
   {
     SBCommandReturnObject result;
     m_interp.HandleCommand("multicommand subcommand_with_autorepeat", result,
                            /*add_to_history=*/true);
     EXPECT_TRUE(result.Succeeded());
-    EXPECT_STREQ(result.GetOutput(), "It worked again\n");
+    EXPECT_STREQ(result.GetOutput(), "Dummy command with autorepeat\n");
   }
   {
     SBCommandReturnObject result;
     m_interp.HandleCommand("", result);
     EXPECT_TRUE(result.Succeeded());
-    EXPECT_STREQ(result.GetOutput(), "It worked again\n");
+    EXPECT_STREQ(result.GetOutput(), "Dummy command with autorepeat\n");
   }
 
-  DummyCommand subcommand2("It worked again 2");
   // We now test a subcommand with autorepeat of the command name
   command.AddCommand(
-      "subcommand_with_custom_autorepeat", &subcommand2, /*help=*/nullptr,
+      "subcommand_with_custom_autorepeat",
+      new DummyCommand("Another dummy command"), /*help=*/nullptr,
       /*syntax=*/nullptr,
       /*auto_repeat_command=*/"multicommand subcommand_with_autorepeat");
   {
     SBCommandReturnObject result;
     m_interp.HandleCommand("multicommand subcommand_with_custom_autorepeat",
                            result, /*add_to_history=*/true);
     EXPECT_TRUE(result.Succeeded());
-    EXPECT_STREQ(result.GetOutput(), "It worked again 2\n");
+    EXPECT_STREQ(result.GetOutput(), "Another dummy command\n");
   }
   {
     SBCommandReturnObject result;
     m_interp.HandleCommand("", result);
     EXPECT_TRUE(result.Succeeded());
-    EXPECT_STREQ(result.GetOutput(), "It worked again\n");
+    EXPECT_STREQ(result.GetOutput(), "Dummy command with autorepeat\n");
   }
 }