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

[Jim Ingham via lldb-commits]

> On Jun 22, 2020, at 5:52 AM, Pavel Labath via Phabricator via lldb-commits <lldb-commits at lists.llvm.org> wrote:
> 
> 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).

This isn’t directly related to how much and how we should capture lldb session output, and maybe I’m misunderstanding your meaning, but I wasn’t expecting that moving the command interpreter to use SB API’s would mean the Debugger Object would know nothing about the Command interpreter.  It would know as much about the command interpreter as it does about the script interpreter, namely the Debugger holds these objects and is the one to hand them out.  For instance when the breakpoint has a bunch of commands in its command action, it would go to the debugger to evaluate those commands.  I think that’s the reasonable place from which to vend the command and script interpreters.  So it’s okay IMO for the Debugger to know a little bit about these entities.  It shouldn’t know anything about the command syntax, etc.  But since it is still the vendor of these objects, it seems okay for it to have an affordance to be notified of command results.

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

Yes, I think we should be careful about this.  First off, I think command transcripts are not useful if you can’t link output to the command that triggered it.  And if we execute a command for instance in a data formatter, having that command output show up with no context in the log wouldn’t I think be helpful.  I think it would be useful to have a notion of user-actuated commands, and try to record those.  And it would also be good to provide some context.  For instance, if you have commands in a breakpoint action it would be great if you the output would identify them as such.

Jim


> 
> ================
> 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
> 
> 
> 
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits