Page MenuHomePhabricator

[lldb] Getting the suggestions also from previous sessions
Needs RevisionPublic

Authored by gedatsu217 on Jul 22 2020, 12:45 PM.

Details

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.

Diff Detail

Event Timeline

teemperor requested changes to this revision.Jul 26 2020, 11:53 AM

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).

This revision now requires changes to proceed.Jul 26 2020, 11:53 AM

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()

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).

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 requested changes to this revision.Aug 13 2020, 11:47 PM

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).

This revision now requires changes to proceed.Aug 13 2020, 11:47 PM

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.

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.

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.

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.