[lldb] Getting the suggestions also from previous sessions

[gedatsu217]

Summary

I implemented features that get the suggestions also from previous sessions.

If users use autosuggestion, it set the previous history in the constructor of IOHandlerEditline.

[teemperor]

What I actually meant was that you upload the diff between this and the previous patch (e.g. all the parts like GetAutoSuggestionForCommand shouldn't be in here). Otherwise it's not really obvious what this patch is actually changing. You can just make two git commits locally (one for this and one for the first patch) and then split the diffs this way.

From what I can see the changes here seems mostly fine.

Regarding the expected test: This isn't really related to the autosuggestion feature but a more generic feature, so I would make a new separate test where you check that the command history is persistent across lldb instances. The easiest way to check this is to add a unique command to the history and then relaunch LLDB and check its still there (you can list the command history via session history. If your branch isn't fully up-to-date, the command might still be named command history).

lldb/include/lldb/Host/Editline.h

215

I'm a bit surprised that a Add* function is actually just returning a value. Maybe GetCommandHistory().

lldb/source/Host/common/Editline.cpp

294

Maybe name this GetAsStringList.

lldb/source/Interpreter/CommandInterpreter.cpp

134

Each string is a command from the command history (I would say the single std::vector is a single "command history"), so this seems more logical:

for (const std::string &command : history)
  m_command_history.AppendString(command);

Also you don't need the this-> and another small thing: AppendString takes a StringRef, which is just a reference to a string. So if you take std::string as the command here, then you make a copy if each command but then AppendString also has to take another copy (to convert StringRef back to a std::string).

[gedatsu217]

Regarding the expected test: This isn't really related to the autosuggestion feature but a more generic feature, so I would make a new separate test where you check that the command history is persistent across lldb instances. The easiest way to check this is to add a unique command to the history and then relaunch LLDB and check its still there (you can list the command history via session history. If your branch isn't fully up-to-date, the command might still be named command history).

What command should be saved? Everyone has different saved commands, so I do not know what is unique.

And now, I try the below code, but it does not work. Do you know the causes?

self.launch(extra_args=["-o", "settings set show-autosuggestion true", "-o", "settings set use-color true"])

self.child.send("help frame\n")
self.child.send("breakpoint\n")

self.quit()

self.launch(extra_args=["-o", "settings set show-autosuggestion true", "-o", "settings set use-color true"])

self.child.send("session history\n")
self.child.expect_exact("help frame")
self.child.expect_exact("breakpoint")
self.quit()

[gedatsu217]

ping

[teemperor]

In D84355#2200813, @gedatsu217 wrote:

Regarding the expected test: This isn't really related to the autosuggestion feature but a more generic feature, so I would make a new separate test where you check that the command history is persistent across lldb instances. The easiest way to check this is to add a unique command to the history and then relaunch LLDB and check its still there (you can list the command history via session history. If your branch isn't fully up-to-date, the command might still be named command history).

What command should be saved? Everyone has different saved commands, so I do not know what is unique.

And now, I try the below code, but it does not work. Do you know the causes?

self.launch(extra_args=["-o", "settings set show-autosuggestion true", "-o", "settings set use-color true"])

self.child.send("help frame\n")
self.child.send("breakpoint\n")

self.quit()

self.launch(extra_args=["-o", "settings set show-autosuggestion true", "-o", "settings set use-color true"])

self.child.send("session history\n")
self.child.expect_exact("help frame")
self.child.expect_exact("breakpoint")
self.quit()

What exactly isn't working? If you could upload the test with the patch I could try reproducing it here.

Some general feedback: You probably don't just want to do self.child.send(...) but just self.expect("help frame"). self.child.send just sends the string and then instantly returns and doesn't wait for an answer from LLDB. You probably also want to do the same for the second check self.expect("session history", substrs=["help frame", "breakpoint"])

With unique command I mean: The test currently adds help frame to the history. But now all future test runs also have help frame in the command history, which means that your test will always pass even if it doesn't read the last commands. But if your command is something like " expr " + unique_time_stamp_number then the test can't pass by accident because the old commands were different. E.g., expr 1 and later one you do expr 2 (something like POSIX seconds since epoch is probably a good source for a unique number).

[gedatsu217]

Revise the code according to your advice.

And, test does not work well. Would you check it?
Maybe, command history on pexpect is not saved.

[teemperor]

This is great, thanks! I have some small final comments but after that this is ready to land!

lldb/source/Core/IOHandler.cpp

271

You can actually move this out of the if. Having previous sessions in the command history is a nice feature even without using autosuggestions.

lldb/test/API/iohandler/autosuggestion/TestAutosuggestionFromPreviousSessions.py

28

Actually just doing now_time = str(time.time()) is good enough.

29

If you move the code out of the if (see the first inline comment), then you can also remove the extra_args here.

Also the test can be renamed to just iohandler/TestCommandHistoryFromPrevioiusSession.py (it shouldn't be related to autosuggestions).

[gedatsu217]

And, the test does not work well. Would you check it?

Did you check it? This test seems to work well, but an error occurs.

def test_autosuggestion_from_previous_sessions(self):
        now_time = str(time.time())
        self.launch()
        
        # Check if histories are saved after quit.       
        self.expect("expr " + now_time)
        self.quit()

        self.launch()

        self.expect("session history", substrs=[now_time])
        self.quit()

When I remove self.quit() and self.launch() in the middle of this code, this test passes. So, if pexpect finishes once, histories are not probably saved.

[teemperor]

In D84355#2218229, @gedatsu217 wrote:

And, the test does not work well. Would you check it?

Did you check it? This test seems to work well, but an error occurs.

def test_autosuggestion_from_previous_sessions(self):
        now_time = str(time.time())
        self.launch()
        
        # Check if histories are saved after quit.       
        self.expect("expr " + now_time)
        self.quit()

        self.launch()

        self.expect("session history", substrs=[now_time])
        self.quit()

When I remove self.quit() and self.launch() in the middle of this code, this test passes. So, if pexpect finishes once, histories are not probably saved.

What error do you get? For me the test seems to fail because we apparently don't save the session history for the first session. It doesn't show up in any history files in .lldb, but the session history does show up. I assume we don't shut down LLDB correctly. You could try sending quit\n, then sleep(3) after the expect("expr ... line.

[gedatsu217]

def test_autosuggestion_from_previous_sessions(self):
        now_time = str(time.time())
        self.launch()
        
        # Check if histories are saved after quit.       
        self.expect("expr " + now_time)
        time.sleep(3)
        self.child.send("quit\n")
        self.quit()

        self.launch()

        self.expect("session history", substrs=[now_time])
        self.quit()

Like this? It seems not to work well...
I'm looking for a good solution to this problem and trying various ways now.

[gedatsu217]

Although I tried some ways, none went well. In addition, I looked into several tests about session history, but I did not come up with a good way.
Do you know other ways to shut down lldb correctly?

What error do you get? For me the test seems to fail because we apparently don't save the session history for the first session.

It is the same as me.

lldb/include/lldb/Host/Editline.h

public:
   /// Prompts for and reads a single line of user input.
   bool GetLine(std::string &line, bool &interrupted);
 
   /// Prompts for and reads a multi-line batch of user input.
   bool GetLines(int first_line_number, StringList &lines, bool &interrupted);
 
   void PrintAsync(Stream *stream, const char *s, size_t len);
 
+  std::vector<std::string> GetCommandHistory();
+
 private:
   /// Sets the lowest line number for multi-line editing sessions.  A value of
   /// zero suppresses
   /// line number printing in the prompt.
   void SetBaseLineNumber(int line_number);
 
   /// Returns the complete prompt by combining the prompt or continuation prompt
   /// with line numbers

lldb/include/lldb/Interpreter/CommandInterpreter.h

public:
 
   void GetPythonCommandsFromIOHandler(const char *prompt,
                                       IOHandlerDelegate &delegate,
                                       void *baton = nullptr);
 
   const char *GetCommandPrefix();
 
   // Properties
+  void AddCommandHistory(std::vector<std::string> histories);
+
   bool GetExpandRegexAliases() const;
 
   bool GetPromptOnQuit() const;
   void SetPromptOnQuit(bool enable);
 
   bool GetSaveSessionOnQuit() const;
   void SetSaveSessionOnQuit(bool enable);
 

lldb/source/Core/IOHandler.cpp

use_editline = GetInputFILE() && GetOutputFILE() && GetErrorFILE() &&
                  m_input_sp && m_input_sp->GetIsRealTerminal();
 
   if (use_editline) {
     m_editline_up = std::make_unique<Editline>(editline_name, GetInputFILE(),
                                                GetOutputFILE(), GetErrorFILE(),
                                                m_color_prompts);
     m_editline_up->SetIsInputCompleteCallback(IsInputCompleteCallback, this);
     m_editline_up->SetAutoCompleteCallback(AutoCompleteCallback, this);
-    if (debugger.GetUseAutosuggestion() && debugger.GetUseColor())
+    if (debugger.GetUseAutosuggestion() && debugger.GetUseColor()) {
       m_editline_up->SetSuggestionCallback(SuggestionCallback, this);
+      std::vector<std::string> histories = m_editline_up->GetCommandHistory();
+      this->GetDebugger().GetCommandInterpreter().AddCommandHistory(histories);
+    }
     // See if the delegate supports fixing indentation
     const char *indent_chars = delegate.IOHandlerGetFixIndentationCharacters();
     if (indent_chars) {
       // The delegate does support indentation, hook it up so when any
       // indentation character is typed, the delegate gets a chance to fix it
       m_editline_up->SetFixIndentationCallback(FixIndentationCallback, this,
                                                indent_chars);
     }

lldb/source/Host/common/Editline.cpp

if (m_history) {
       if (path) {
         history_w(m_history, &m_event, H_SAVE, path);
         return true;
       }
     }
     return false;
   }
 
+  std::vector<std::string> GetAsStringList() {
+    this->Load();
+    std::vector<std::string> histories;
+    if (m_history) {
+      const char *path = GetHistoryFilePath();
+      if (path) {
+        history_w(m_history, &m_event, H_GETSIZE);
+        int num = m_event.num;
+        for (int i = 0; i < num; ++i) {
+          history_w(m_history, &m_event, H_SET, i);
+          history_w(m_history, &m_event, H_CURR);
+          std::string string_history =
+              std::wstring_convert<std::codecvt_utf8<wchar_t>>().to_bytes(
+                  m_event.str);
+          histories.push_back(
+              string_history.substr(0, string_history.length() - 1));
+        }
+      }
+    }
+    return histories;
+  }
+
 protected:
   HistoryW *m_history; // The history object
   HistEventW m_event;  // The history event needed to contain all history events
   std::string m_prefix; // The prefix name (usually the editline program name)
                         // to use when loading/saving history
   std::string m_path;   // Path to the history file
 };
 }
case std::codecvt_base::partial:
           &ch, 1, std::chrono::seconds(0), status, nullptr);
       if (read_count == 0)
         return false;
       break;
     }
   }
 #endif
 }
+
+std::vector<std::string> Editline::GetCommandHistory() {
+  return m_history_sp->GetAsStringList();
+}
 No newline at end of file

lldb/source/Interpreter/CommandInterpreter.cpp

CommandInterpreter::CommandInterpreter(Debugger &debugger,
   SetEventName(eBroadcastBitThreadShouldExit, "thread-should-exit");
   SetEventName(eBroadcastBitResetPrompt, "reset-prompt");
   SetEventName(eBroadcastBitQuitCommandReceived, "quit");
   SetSynchronous(synchronous_execution);
   CheckInWithManager();
   m_collection_sp->Initialize(g_interpreter_properties);
 }
 
+void CommandInterpreter::AddCommandHistory(std::vector<std::string> histories) {
+  for (const std::string &command : histories) {
+    m_command_history.AppendString(command);
+  }
+}
+
 bool CommandInterpreter::GetExpandRegexAliases() const {
   const uint32_t idx = ePropertyExpandRegexAliases;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
       nullptr, idx, g_interpreter_properties[idx].default_uint_value != 0);
 }
 
 bool CommandInterpreter::GetPromptOnQuit() const {
   const uint32_t idx = ePropertyPromptOnQuit;

lldb/test/API/iohandler/autosuggestion/TestAutosuggestionFromPreviousSessions.py

+"""
+Tests autosuggestion from previous sessions using pexpect.
+"""
+import time
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.lldbpexpect import PExpectTest
+
+def get_time():
+    now_time = time.gmtime()
+    res = ""
+    for num in now_time:
+        res += str(num)
+    return res
+
+
+
+class TestCase(PExpectTest):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    # PExpect uses many timeouts internally and doesn't play well
+    # under ASAN on a loaded machine..
+    @skipIfAsan
+    @skipIfEditlineSupportMissing
+    def test_autosuggestion_from_previous_sessions(self):
+        now_time = get_time()
+        self.launch(extra_args=["-o", "settings set show-autosuggestion true", "-o", "settings set use-color true"])
+        
+        # Check if histories are saved after quit.       
+        self.expect("expr " + now_time)
+        self.quit()
+
+        self.launch(extra_args=["-o", "settings set show-autosuggestion true", "-o", "settings set use-color true"])
+
+        self.expect("session history", substrs=[now_time])
+        self.quit()
 No newline at end of file