[Lldb-commits] [PATCH] D58564: [Reproducers] Add command provider

[Pavel Labath via Phabricator via lldb-commits]

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