[Lldb-commits] [PATCH] D82155: [WIP][lldb/interpreter] Add ability to save lldb session to a file

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This seems like it could be useful in some circumstances, though for the use cases I am imagining (bug reporting) it would be easier to just copy-paste the terminal contents.

As for the implementation, if the intention is for this to eventually capture all debugger output, then I am not sure if this is the right design. I think it would be fine for python/lua interpreters as those are invoked from the lldb command interpreter, but I have a feeling that routing the output printed via `Debugger::PrintAsync` back to the command interpreter would look pretty weird. It may make sense for the core logic of this to live in the Debugger or the IOHandler(Stack) classes -- though I am not exactly sure about that either as the Debugger and CommandIntepreter classes are fairly tightly coupled. However, I think that would be consistent with the long term goal of reimplementing the command interpreter on top of the SB API (in which case the `Debugger` object should not know anything about the command interpreter (but it would still need to to "something" with the PrintAsync output).

The test plan sounds fairly straight forward -- run lldb, execute a bunch of commands, and finish it off with "session save". Then, verify that the file has the "right" contents (e.g. with FileCheck). Besides multiline commands, commands which do recursive processing are also interesting to exercise -- e.g. "command source" or breakpoint callbacks. You also should decide what do you want to happen with commands that are executed through the SB interface (SBCommandInterpreter::HandleCommand) -- those will not normally go to the debugger output, but I believe they will still be captured in the current design...



================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2815
 
+  if (line.find("session save") == line.npos) {
+    *m_session_transcripts << io_handler.GetPrompt() << ' ' << line.c_str()
----------------
JDevlieghere wrote:
> this won't work for things like unambiguous abbreviations like `sess save`. The command should do the saving.
I don't think it's unreasonable to write to the "transcript" here, but the string matching is obviously suboptimal. However, it's not clear to me why is it even needed -- having the "save" command in the transcript does not necessarily seem like a bad thing, and I believe the way it is implemented means that the command will not show up in the session file that is currently being saved (only in the subsequent ones).


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2820
+    llvm::StringRef result_output = result.GetOutputData();
+    if (!result_output.empty())
+      *m_session_transcripts << result_output;
----------------
These `!empty()` checks are pointless.


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2827
+
+    m_session_transcripts->Flush();
+  }
----------------
As is this flush call.


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2929
+  Status error;
+  user_id_t fd_dst = FileCache::GetInstance().OpenFile(
+      FileSpec(output_file), flags, lldb::eFilePermissionsFileDefault, error);
----------------
mib wrote:
> JDevlieghere wrote:
> > Why are you going through the `FileCache`? 
> I was thinking if the user saves several times during his session to the same file, that might be useful. Looking at the implementation, it uses the FileSystem instance, so I'm not sure why that would a problem ...
> 
> May be you could elaborate why I shouldn't use it ?
`FileCache` is a very specialist class, so I believe the default should be to _not_ use it. However, if you are looking for reasons, I can name a few:
- the way you are using it means you get no caching benefits whatsoever -- each `OpenFile` call creates a new file descriptor
- it makes it very easy to leak that descriptor (as you did here)


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2945
+  auto string_stream =
+      std::static_pointer_cast<StreamString>(m_session_transcripts);
+  size_t stream_size = string_stream->GetSize();
----------------
Why not make `m_session_transcripts` a `shared_ptr<StreamString>` (or even a plain `StreamString`) in the first place?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82155/new/

https://reviews.llvm.org/D82155