[Lldb-commits] [PATCH] D55582: [Reproducers] Add command reproducer

Thu Dec 13 02:40:21 PST 2018

labath added a comment.

I am very sorry about this, but I am having some second thoughts about the `RunReplay` thingy. I don't think I've correctly expressed what is bothering me about it (and that's probably because I wasn't clear with myself about what is bothering me). I'll try to formulate it better this time

What I was thinking about were the boundaries at which we want to record/replay events. If I understand correctly, one of those boundaries should be the SB layer (is that correct?). So, with that in mind, it makes sense for the lldb driver to do something "special" in replay mode, and invoke some other function which would replay the SB calls it would have normally made. That function might as well be called `RunReplay`.

Now, if we imagine we have this SB record/replay capability, during recording it would detect that the driver made a call to `SBDebugger::RunCommandInterpreter`. Then, during recording it would again make a call to SBDebugger::RunCommandInterpreter, but ensure that it behaves the same way as it did the first time (this is the auto-detection part I was speaking about).

The reason this came out all wrong is because I was also thinking about a (somewhat) unrelated thing, and that is the layer at which we do the capturing of the commands. I was thinking that maybe the command interpreter doesn't need to know anything about the record/replay taking place. If this SB replay engine substituted the `stdin` of the debugger with a recorded version of it, then it could go normally go about it's business and things would "just work" (you'd still have to make sure that libedit doesn't do something funny, but that could hopefully be done at the IOHandler level by substituting the editline IO handler).

So, I mean, that's how I'd do it, but this is your feature, and where you wan't to draw the record/replay line ultimately depends on what you want to get out of the feature. However, I do see some advantages to doing the r/r at a lower level that I'd like to try to sell to you:

- better possibility for capturing ^C. I think ^C is fairly important, particularly in console debugging, as it's the only way to interrupt a program without it voluntarily hitting a breakpoint. It will also be fairly tricky to reproduce. With the current framework, I'm not sure how you would manage to capture that. If you worked with IO handlers and was capturing stdin, I think you'd have a better chance of getting it right (since both stdin and ^C come from the terminal).
- no need for special handling of command sourcing. As you would capture almost-raw stdin, you wouldn't need to do anything special to the avoid sourced commands being captured twice. The filesystem would just provide the captured file, and stdin-recorder would capture the stdin. (which I think would make the stdin-capturer fit nicely with the Filesystem one, as they're both fairly low-level).
- right now you're merging commands coming from multiple sources (stdin via `RunCommandInterpreter`, command strings from `HandleCommand`) into one monolithic command stream. I think this means that once you have an SB recorder, it will also have to special case command APIs to avoid doing things twice. If you work at a lower level, this won't happen, because HandleCommand would be naturally captured as part of the SB stream, and stdin commands would be captured by the stdin-recorder (which would probably also be initialized and managed by the SB recorder, since it needs to be hooked into RunCommandInterpreter)

So, basically, my point is:

- let's bring back `RunReplay`. I don't think it makes sense to force this to go through `SBDebugger::RunCommandInterpreter` in the current setup, since both the caller and the callee special-case replay mode anyway.
- I'd urge you to consider whether you really want to do the capturing at this level

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55582/new/

https://reviews.llvm.org/D55582