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