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

Thu Dec 13 09:12:01 PST 2018

JDevlieghere added a comment.

In D55582#1329479 <https://reviews.llvm.org/D55582#1329479>, @labath wrote:

> 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

No worries, I appreciate you thinking this over :-)

> 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`.

Yep, the SB layer is an important boundary. Everything that happens in Xcode goes through it. It's probably going to be one of the harder parts of the feature. Every API call needs to be able to  capture and replay while keeping things like references consistent. We have spent some time thinking about this, but I'm sure a bunch of complexities will pop up once we start doing the actual work.

> 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).

I see what you mean now, and it's a fair point/concern. So far I've been postponing dealing with the SB layer until I could get reproducers to work with the command driver. Basically I'll have to audit all the SB endpoints anyway so I figured patching this up was going to be negligible compared to that effort. However, you're totally right that we shouldn't ignore its impact on design decisions. Moving it into a separate function will make it easier down the road, as we can just do "the right" thing in replay mode for that particular function.

> 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).

I didn't consider that idea but I like the sound of it :-)

> 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)

+1 on all these points. I'll look into doing it this way today.

> 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

Thanks!

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

https://reviews.llvm.org/D55582