[lldb/interpreter] Move the history subcommand to session (NFCI)

[mib]

Summary

This patch moves the history subcommand from the command to session command.
I think it makes more sense to have it there because as the command usage
suggests, it should be used to manage custom LLDB commands.

However, history is essentially tied to a debugging session and holds
all the commands (not specifically custom ones).

This also makes it more discoverable by adding an alias for it (mimicking the shell builtin).

Signed-off-by: Med Ismail Bennani <[e-mail]>

[labath]

I don't have any strong opinions about this either way. The history alias is kind of nice because it matches the bash builtin, but I don't use either of them often enough to care.

[teemperor]

Isn't session just concerned with the *current* LLDB session? LLDB's command history is only spanning the current session at the moment, but I always thought that's more of a bug and not a feature (In fact one of my GSoC students may actually have to implement that the history is spanning multiple sessions). On the other hand it seems to best fit the current behaviour so it's fine by me.

[teemperor]

(Phab send the whole thing when I pressed enter....)

I added Jim who might have more insights on the design rationale here, but IMHO this seems like an improvement.

[jingham]

The current lldb behavior is that when you start up lldb, it reads the editline command-history file (so the history of the previous session), and loads it into it's internal command history, which you can get to by up and down arrows or ^R searches. But it doesn't prime the list that the "command history" command has access to. So command history starts out empty. But IMO, that's just bug. If a command shows up in scrolling through with up-arrow, it should also be listed in the history command wherever it lives. This really reduces the utility of the "history" command... I think we do want to show this. We might want mark in the "command history" output where the imported commands end and the new commands start, but we really should make them available.

However, even if we started doing this right, I think the list of historical commands that were imported into lldb at startup properly belong to the current session. So I don't think this really argues either way.

I'm fine with history going under session.

I didn't hit "accept revision" because I only addressed Raphael's question, I haven't read the patch yet. But if somebody else has and is okay with the details, I have no objections to it.

[teemperor]

The patch itself is fine, so let's ship it!

lldb/source/Commands/CommandObjectCommands.cpp

 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/StringList.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 // CommandObjectCommandsSource
 
-#define LLDB_OPTIONS_history
-#include "CommandOptions.inc"
-
-class CommandObjectCommandsHistory : public CommandObjectParsed {
-public:
-  CommandObjectCommandsHistory(CommandInterpreter &interpreter)
-      : CommandObjectParsed(interpreter, "command history",
-                            "Dump the history of commands in this session.\n"
-                            "Commands in the history list can be run again "
-                            "using \"!<INDEX>\".   \"!-<OFFSET>\" will re-run "
-                            "the command that is <OFFSET> commands from the end"
-                            " of the list (counting the current command).",
-                            nullptr),
-        m_options() {}
-
-  ~CommandObjectCommandsHistory() override = default;
-
-  Options *GetOptions() override { return &m_options; }
-
-protected:
-  class CommandOptions : public Options {
-  public:
-    CommandOptions()
-        : Options(), m_start_idx(0), m_stop_idx(0), m_count(0), m_clear(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 'c':
-        error = m_count.SetValueFromString(option_arg, eVarSetOperationAssign);
-        break;
-      case 's':
-        if (option_arg == "end") {
-          m_start_idx.SetCurrentValue(UINT64_MAX);
-          m_start_idx.SetOptionWasSet();
-        } else
-          error = m_start_idx.SetValueFromString(option_arg,
-                                                 eVarSetOperationAssign);
-        break;
-      case 'e':
-        error =
-            m_stop_idx.SetValueFromString(option_arg, eVarSetOperationAssign);
-        break;
-      case 'C':
-        m_clear.SetCurrentValue(true);
-        m_clear.SetOptionWasSet();
-        break;
-      default:
-        llvm_unreachable("Unimplemented option");
-      }
-
-      return error;
-    }
-
-    void OptionParsingStarting(ExecutionContext *execution_context) override {
-      m_start_idx.Clear();
-      m_stop_idx.Clear();
-      m_count.Clear();
-      m_clear.Clear();
-    }
-
-    llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
-      return llvm::makeArrayRef(g_history_options);
-    }
-
-    // Instance variables to hold the values for command options.
-
-    OptionValueUInt64 m_start_idx;
-    OptionValueUInt64 m_stop_idx;
-    OptionValueUInt64 m_count;
-    OptionValueBoolean m_clear;
-  };
-
-  bool DoExecute(Args &command, CommandReturnObject &result) override {
-    if (m_options.m_clear.GetCurrentValue() &&
-        m_options.m_clear.OptionWasSet()) {
-      m_interpreter.GetCommandHistory().Clear();
-      result.SetStatus(lldb::eReturnStatusSuccessFinishNoResult);
-    } else {
-      if (m_options.m_start_idx.OptionWasSet() &&
-          m_options.m_stop_idx.OptionWasSet() &&
-          m_options.m_count.OptionWasSet()) {
-        result.AppendError("--count, --start-index and --end-index cannot be "
-                           "all specified in the same invocation");
-        result.SetStatus(lldb::eReturnStatusFailed);
-      } else {
-        std::pair<bool, uint64_t> start_idx(
-            m_options.m_start_idx.OptionWasSet(),
-            m_options.m_start_idx.GetCurrentValue());
-        std::pair<bool, uint64_t> stop_idx(
-            m_options.m_stop_idx.OptionWasSet(),
-            m_options.m_stop_idx.GetCurrentValue());
-        std::pair<bool, uint64_t> count(m_options.m_count.OptionWasSet(),
-                                        m_options.m_count.GetCurrentValue());
-
-        const CommandHistory &history(m_interpreter.GetCommandHistory());
-
-        if (start_idx.first && start_idx.second == UINT64_MAX) {
-          if (count.first) {
-            start_idx.second = history.GetSize() - count.second;
-            stop_idx.second = history.GetSize() - 1;
-          } else if (stop_idx.first) {
-            start_idx.second = stop_idx.second;
-            stop_idx.second = history.GetSize() - 1;
-          } else {
-            start_idx.second = 0;
-            stop_idx.second = history.GetSize() - 1;
-          }
-        } else {
-          if (!start_idx.first && !stop_idx.first && !count.first) {
-            start_idx.second = 0;
-            stop_idx.second = history.GetSize() - 1;
-          } else if (start_idx.first) {
-            if (count.first) {
-              stop_idx.second = start_idx.second + count.second - 1;
-            } else if (!stop_idx.first) {
-              stop_idx.second = history.GetSize() - 1;
-            }
-          } else if (stop_idx.first) {
-            if (count.first) {
-              if (stop_idx.second >= count.second)
-                start_idx.second = stop_idx.second - count.second + 1;
-              else
-                start_idx.second = 0;
-            }
-          } else /* if (count.first) */
-          {
-            start_idx.second = 0;
-            stop_idx.second = count.second - 1;
-          }
-        }
-        history.Dump(result.GetOutputStream(), start_idx.second,
-                     stop_idx.second);
-      }
-    }
-    return result.Succeeded();
-  }
-
-  CommandOptions m_options;
-};
-
-// CommandObjectCommandsSource
-
 #define LLDB_OPTIONS_source
 #include "CommandOptions.inc"
 
 class CommandObjectCommandsSource : public CommandObjectParsed {
 public:
   CommandObjectCommandsSource(CommandInterpreter &interpreter)
       : CommandObjectParsed(
             interpreter, "command source",
CommandObjectMultiwordCommands::CommandObjectMultiwordCommands(
   LoadSubCommand("alias",
                  CommandObjectSP(new CommandObjectCommandsAlias(interpreter)));
   LoadSubCommand("unalias", CommandObjectSP(
                                 new CommandObjectCommandsUnalias(interpreter)));
   LoadSubCommand("delete",
                  CommandObjectSP(new CommandObjectCommandsDelete(interpreter)));
   LoadSubCommand(
       "regex", CommandObjectSP(new CommandObjectCommandsAddRegex(interpreter)));
-  LoadSubCommand("history", CommandObjectSP(
-                                new CommandObjectCommandsHistory(interpreter)));
   LoadSubCommand(
       "script",
       CommandObjectSP(new CommandObjectMultiwordCommandsScript(interpreter)));
 }
 
 CommandObjectMultiwordCommands::~CommandObjectMultiwordCommands() = default;

lldb/source/Commands/CommandObjectSession.cpp

 #include "CommandObjectSession.h"
+#include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Interpreter/OptionArgParser.h"
+#include "lldb/Interpreter/OptionValue.h"
+#include "lldb/Interpreter/OptionValueBoolean.h"
+#include "lldb/Interpreter/OptionValueString.h"
+#include "lldb/Interpreter/OptionValueUInt64.h"
+#include "lldb/Interpreter/Options.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 class CommandObjectSessionSave : public CommandObjectParsed {
 public:
   CommandObjectSessionSave(CommandInterpreter &interpreter)
       : CommandObjectParsed(interpreter, "session save",
bool DoExecute(Args &args, CommandReturnObject &result) override {
     if (m_interpreter.SaveTranscript(result, file_path.str()))
       result.SetStatus(eReturnStatusSuccessFinishNoResult);
     else
       result.SetStatus(eReturnStatusFailed);
     return result.Succeeded();
   }
 };
 
+#define LLDB_OPTIONS_history
+#include "CommandOptions.inc"
+
+class CommandObjectSessionHistory : public CommandObjectParsed {
+public:
+  CommandObjectSessionHistory(CommandInterpreter &interpreter)
+      : CommandObjectParsed(interpreter, "session history",
+                            "Dump the history of commands in this session.\n"
+                            "Commands in the history list can be run again "
+                            "using \"!<INDEX>\".   \"!-<OFFSET>\" will re-run "
+                            "the command that is <OFFSET> commands from the end"
+                            " of the list (counting the current command).",
+                            nullptr),
+        m_options() {}
+
+  ~CommandObjectSessionHistory() override = default;
+
+  Options *GetOptions() override { return &m_options; }
+
+protected:
+  class CommandOptions : public Options {
+  public:
+    CommandOptions()
+        : Options(), m_start_idx(0), m_stop_idx(0), m_count(0), m_clear(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 'c':
+        error = m_count.SetValueFromString(option_arg, eVarSetOperationAssign);
+        break;
+      case 's':
+        if (option_arg == "end") {
+          m_start_idx.SetCurrentValue(UINT64_MAX);
+          m_start_idx.SetOptionWasSet();
+        } else
+          error = m_start_idx.SetValueFromString(option_arg,
+                                                 eVarSetOperationAssign);
+        break;
+      case 'e':
+        error =
+            m_stop_idx.SetValueFromString(option_arg, eVarSetOperationAssign);
+        break;
+      case 'C':
+        m_clear.SetCurrentValue(true);
+        m_clear.SetOptionWasSet();
+        break;
+      default:
+        llvm_unreachable("Unimplemented option");
+      }
+
+      return error;
+    }
+
+    void OptionParsingStarting(ExecutionContext *execution_context) override {
+      m_start_idx.Clear();
+      m_stop_idx.Clear();
+      m_count.Clear();
+      m_clear.Clear();
+    }
+
+    llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
+      return llvm::makeArrayRef(g_history_options);
+    }
+
+    // Instance variables to hold the values for command options.
+
+    OptionValueUInt64 m_start_idx;
+    OptionValueUInt64 m_stop_idx;
+    OptionValueUInt64 m_count;
+    OptionValueBoolean m_clear;
+  };
+
+  bool DoExecute(Args &command, CommandReturnObject &result) override {
+    if (m_options.m_clear.GetCurrentValue() &&
+        m_options.m_clear.OptionWasSet()) {
+      m_interpreter.GetCommandHistory().Clear();
+      result.SetStatus(lldb::eReturnStatusSuccessFinishNoResult);
+    } else {
+      if (m_options.m_start_idx.OptionWasSet() &&
+          m_options.m_stop_idx.OptionWasSet() &&
+          m_options.m_count.OptionWasSet()) {
+        result.AppendError("--count, --start-index and --end-index cannot be "
+                           "all specified in the same invocation");
+        result.SetStatus(lldb::eReturnStatusFailed);
+      } else {
+        std::pair<bool, uint64_t> start_idx(
+            m_options.m_start_idx.OptionWasSet(),
+            m_options.m_start_idx.GetCurrentValue());
+        std::pair<bool, uint64_t> stop_idx(
+            m_options.m_stop_idx.OptionWasSet(),
+            m_options.m_stop_idx.GetCurrentValue());
+        std::pair<bool, uint64_t> count(m_options.m_count.OptionWasSet(),
+                                        m_options.m_count.GetCurrentValue());
+
+        const CommandHistory &history(m_interpreter.GetCommandHistory());
+
+        if (start_idx.first && start_idx.second == UINT64_MAX) {
+          if (count.first) {
+            start_idx.second = history.GetSize() - count.second;
+            stop_idx.second = history.GetSize() - 1;
+          } else if (stop_idx.first) {
+            start_idx.second = stop_idx.second;
+            stop_idx.second = history.GetSize() - 1;
+          } else {
+            start_idx.second = 0;
+            stop_idx.second = history.GetSize() - 1;
+          }
+        } else {
+          if (!start_idx.first && !stop_idx.first && !count.first) {
+            start_idx.second = 0;
+            stop_idx.second = history.GetSize() - 1;
+          } else if (start_idx.first) {
+            if (count.first) {
+              stop_idx.second = start_idx.second + count.second - 1;
+            } else if (!stop_idx.first) {
+              stop_idx.second = history.GetSize() - 1;
+            }
+          } else if (stop_idx.first) {
+            if (count.first) {
+              if (stop_idx.second >= count.second)
+                start_idx.second = stop_idx.second - count.second + 1;
+              else
+                start_idx.second = 0;
+            }
+          } else /* if (count.first) */
+          {
+            start_idx.second = 0;
+            stop_idx.second = count.second - 1;
+          }
+        }
+        history.Dump(result.GetOutputStream(), start_idx.second,
+                     stop_idx.second);
+      }
+    }
+    return result.Succeeded();
+  }
+
+  CommandOptions m_options;
+};
+
 CommandObjectSession::CommandObjectSession(CommandInterpreter &interpreter)
     : CommandObjectMultiword(interpreter, "session",
                              "Commands controlling LLDB session.",
                              "session <subcommand> [<command-options>]") {
   LoadSubCommand("save",
                  CommandObjectSP(new CommandObjectSessionSave(interpreter)));
-  //  TODO: Move 'history' subcommand from CommandObjectCommands.
+  LoadSubCommand("history",
+                 CommandObjectSP(new CommandObjectSessionHistory(interpreter)));
 }

lldb/source/Interpreter/CommandInterpreter.cpp

if (cmd_obj_sp) {
     AddAlias("var", cmd_obj_sp);
     AddAlias("vo", cmd_obj_sp, "--object-description");
   }
 
   cmd_obj_sp = GetCommandSPExact("register", false);
   if (cmd_obj_sp) {
     AddAlias("re", cmd_obj_sp);
   }
+
+  cmd_obj_sp = GetCommandSPExact("session history", false);
+  if (cmd_obj_sp) {
+    AddAlias("history", cmd_obj_sp);
+  }
 }
 
 void CommandInterpreter::Clear() {
   m_command_io_handler_sp.reset();
 }
 
 const char *CommandInterpreter::ProcessEmbeddedScriptCommands(const char *arg) {
   // This function has not yet been implemented.

lldb/test/API/commands/session/history/TestSessionHistory.py

 """
-Test the command history mechanism
+Test the session history command
 """
 
 
 
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 
-class CommandHistoryTestCase(TestBase):
+class SessionHistoryTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     @no_debug_info_test
     def test_history(self):
-        self.runCmd('command history --clear', inHistory=False)
+        self.runCmd('session history --clear', inHistory=False)
         self.runCmd('breakpoint list', check=False, inHistory=True)  # 0
         self.runCmd('register read', check=False, inHistory=True)  # 1
         self.runCmd('apropos hello', check=False, inHistory=True)  # 2
         self.runCmd('memory write', check=False, inHistory=True)  # 3
         self.runCmd('log list', check=False, inHistory=True)  # 4
         self.runCmd('disassemble', check=False, inHistory=True)  # 5
         self.runCmd('expression 1', check=False, inHistory=True)  # 6
         self.runCmd(
             'type summary list -w default',
             check=False,
             inHistory=True)  # 7
         self.runCmd('version', check=False, inHistory=True)  # 8
         self.runCmd('frame select 1', check=False, inHistory=True)  # 9
 
         self.expect(
-            "command history -s 3 -c 3",
+            "session history -s 3 -c 3",
             inHistory=True,
             substrs=[
                 '3: memory write',
                 '4: log list',
                 '5: disassemble'])
 
-        self.expect("command history -s 3 -e 3", inHistory=True,
+        self.expect("session history -s 3 -e 3", inHistory=True,
                     substrs=['3: memory write'])
 
         self.expect(
-            "command history -s 6 -e 7",
+            "session history -s 6 -e 7",
             inHistory=True,
             substrs=[
                 '6: expression 1',
                 '7: type summary list -w default'])
 
-        self.expect("command history -c 2", inHistory=True,
+        self.expect("session history -c 2", inHistory=True,
                     substrs=['0: breakpoint list', '1: register read'])
 
-        self.expect("command history -e 3 -c 1", inHistory=True,
+        self.expect("session history -e 3 -c 1", inHistory=True,
                     substrs=['3: memory write'])
 
         self.expect(
-            "command history -e 2",
+            "session history -e 2",
             inHistory=True,
             substrs=[
                 '0: breakpoint list',
                 '1: register read',
                 '2: apropos hello'])
 
         self.expect(
-            "command history -s 12",
+            "session history -s 12",
             inHistory=True,
             substrs=[
-                '12: command history -s 6 -e 7',
-                '13: command history -c 2',
-                '14: command history -e 3 -c 1',
-                '15: command history -e 2',
-                '16: command history -s 12'])
+                '12: session history -s 6 -e 7',
+                '13: session history -c 2',
+                '14: session history -e 3 -c 1',
+                '15: session history -e 2',
+                '16: session history -s 12'])
 
         self.expect(
-            "command history -s end -c 3",
+            "session history -s end -c 3",
             inHistory=True,
             substrs=[
-                '15: command history -e 2',
-                '16: command history -s 12',
-                '17: command history -s end -c 3'])
+                '15: session history -e 2',
+                '16: session history -s 12',
+                '17: session history -s end -c 3'])
 
         self.expect(
-            "command history -s end -e 15",
+            "session history -s end -e 15",
             inHistory=True,
             substrs=[
-                '15: command history -e 2',
-                '16: command history -s 12',
-                '17: command history -s end -c 3',
-                'command history -s end -e 15'])
+                '15: session history -e 2',
+                '16: session history -s 12',
+                '17: session history -s end -c 3',
+                'session history -s end -e 15'])
 
-        self.expect("command history -s 5 -c 1", inHistory=True,
+        self.expect("session history -s 5 -c 1", inHistory=True,
                     substrs=['5: disassemble'])
 
-        self.expect("command history -c 1 -s 5", inHistory=True,
+        self.expect("session history -c 1 -s 5", inHistory=True,
                     substrs=['5: disassemble'])
 
-        self.expect("command history -c 1 -e 3", inHistory=True,
+        self.expect("session history -c 1 -e 3", inHistory=True,
                     substrs=['3: memory write'])
 
         self.expect(
-            "command history -c 1 -e 3 -s 5",
+            "session history -c 1 -e 3 -s 5",
             error=True,
             inHistory=True,
             substrs=['error: --count, --start-index and --end-index cannot be all specified in the same invocation'])