labath added a comment.
In D58564#1412674 <https://reviews.llvm.org/D58564#1412674>, @JDevlieghere
wrote:
> Pavel made a good point that with the previous implementation, the first call
> to RunCommandInterpreter would replay every recorded commands. This is indeed
> incorrect, because it's possible and likely that the state of the debugger
> has changed between different runs of the commands interpreter.
>
> Now every call to RunCommandInterpreter gets its own buffer. During replay,
> we will change the input file handler before invocation of
> "RunCommandInterpreter". This works because this function is only called
> through the SB layer.
>
> I'm not convinced doing the recorded at a lower level has any benefits. I
> investigated this route before and the IOHandler seems basically the same
> things as the command interpreter. We would still need to make the
> distinction between things that should and shouldn't be recorded (e.g.
> sourcing a file vs every command in the file). This would be a lot harder to
> do there, because we have less information.
In my imagination, this "information" would come down from
`SBDebugger::SetInputFileHandle`, together with the `FILE*` we actually read
the commands from. So, a really crude prototype could be something like:
SBDebugger::SetInputFileHandle(FILE *in) {
if (recording) m_debugger->SetInputFileHandle(in, /*new argument!!!*/ new
FileShadowRecorder(...));
else if (replaying) m_debugger->SetInputFileHandle(GetRecordedFile(...),
nullptr);
}
The FILE* would trickle down into where you read the commands (this could be
the IOHandler, or it could be the CommandInterpreter object), where you would
do something like:
auto stuff = read_stuff(in);
if (shadow_recorder)
shadow_recorder->record_stuff(stuff);
Now when you're sourcing an external file, you just pass in a nullptr for the
recorder when you're setting the input handle for the command interpreter.
So, in essence the shadow recorder would kind of serve the same purpose as your
`add_to_reproducer` flag, but IMO this would be better because it would come
straight from the source (`SetInputFileHandle`) and you wouldn't need to rely
on comments like "This works because this function is only called through the
SB layer". Another benefit would be that this would work out-of-the-box in case
you have multiple SBDebugger objects around, each with it's own command
interpreter (right now this wouldn't work because the `StartNewBuffer` thingies
would step on each others toes). I don't know when or if you plan to support
that, but right now that tells me that this is a better design.
================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:144
+ for (std::size_t i = 0; i < m_commands.size(); ++i) {
+ llvm::Twine filename = llvm::Twine(info::name) + llvm::Twine("-") +
+ llvm::Twine(i) + llvm::Twine(".txt");
----------------
This is incorrect usage of the Twine class. The temporary Twine objects will be
destroyed before you get a chance to stringify them.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58564/new/
https://reviews.llvm.org/D58564
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
