> On Jun 22, 2020, at 5:52 AM, Pavel Labath via Phabricator via lldb-commits
> <lldb-commits@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@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
