Page MenuHomePhabricator

[Reproducers] Add command reproducer
AbandonedPublic

Authored by JDevlieghere on Dec 11 2018, 2:25 PM.

Details

Summary

Add support for reproducing to the command interpreter. In capture mode we capture all the command to file and in reproducer mode we replay this.

Diff Detail

Event Timeline

JDevlieghere created this revision.Dec 11 2018, 2:25 PM

My main question is why do we need the separate SBDebugger::RunReplay API for this. Shouldn't the record/replay be as transparent as possible? I would expect that SBDebugger::RunCommandInterpreter will detect that it is running in replay mode and use the recorded input command stream instead of the real one.

source/Interpreter/CommandInterpreter.cpp
131

This seems pretty hacky, and easy to break in multiple ways:

  • alias rep reproducer; rep generate
  • file /tmp/reproducer/bin/lldb

Couldn't you just have the actual reproducer CommandObject check what mode we're in, and behave like a nop if there is nothing to do? (So, the commands would still be captured, but they wouldn't do anything when replaying.)

2150

Is this correct? What if the command originally has ended with an error (perhaps because the user made a typo)? Will that abort the replay here?

Address Pavel's comments.

JDevlieghere marked 2 inline comments as done.Dec 12 2018, 10:15 AM

Skip driver logic when replaying, otherwise commands are executed or sourced twice, once in the driver and once by replaying the commands.

When sourcing a file we should also ignore the individual commands otherwise they end up getting executed twice, once as part of the command source and once for every individual command in the file.

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

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!

I played around with this today:

  • I'm totally convinced that we should replay by swapping out stdin with a file from the reproducer. This makes this better overall.
  • I'm not convinced that capturing just stdin is an improvement. What I like about the current approach is that we don't have to bother with the -o/-S commands when replaying. We already have a mechanism to deal with them so I don't think it's unreasonable to piggyback on it, especially because this is how we display it to the user. On the other hand, I generally try to keep the code path the same between the regular and reproducer case, and processing the command line options in the reproducer case would reuse more of the existing code. Finally, we'd have to figure out a way to test this, because currently we rely on the commands provided/sourced through the command line to test the replay. How would we test this functionality when only capturing from stdin?

I played around with this today:

  • I'm totally convinced that we should replay by swapping out stdin with a file from the reproducer. This makes this better overall.
  • I'm not convinced that capturing just stdin is an improvement. What I like about the current approach is that we don't have to bother with the -o/-S commands when replaying. We already have a mechanism to deal with them so I don't think it's unreasonable to piggyback on it, especially because this is how we display it to the user.

Hm.. so the way, I'd imagine this working is that -o and friends (as well as pretty much every other command line argument) would be captured by the SB recorder, once they cross the SB boundary. For the -o commands that would be the moment the driver calls m_debugger.SetInputFileHandle https://github.com/llvm-mirror/lldb/blob/master/tools/driver/Driver.cpp#L697 , followed by m_debugger.RunCommandInterpreter a couple of lines lower. So, the SB recorder would notice that we're changing what the debugger considers as it's stdin, and direct the "stdin" recorder (or create a new instance of it, or whatever), to capture it.

Then, during replay, the SB replayer would notice that the driver called SBDebugger::SetInputFileHandle, so it would create a fake FILE* object (or maybe just open the file containing the captured commands from the disk?), and pass *that* as the stdin handle. Then it could just call SBDebugger::RunCommandInterperer the same way as the driver did. And I am hoping that pretty much everything below RunCommandInterpreter could run oblivious to the fact that replay is taking place.

On the other hand, I generally try to keep the code path the same between the regular and reproducer case, and processing the command line options in the reproducer case would reuse more of the existing code.

In this (imaginary) world, the recording/replay would only deal with the things that happen below the SB API. Anything that happens above it could only be glimpsed through it's effects on the SB. So, when initializing replay, as soon the driver notices the --replay argument it would just load that file, call RunReplay and be done. There wouldn't be a need for any command-line-argument recorder, or making sure the command line arguments match in some other way.

Finally, we'd have to figure out a way to test this, because currently we rely on the commands provided/sourced through the command line to test the replay. How would we test this functionality when only capturing from stdin?

By "capturing stdin" I meant capturing anything which the RunCommandInterpreter considers as it's "stdin", i.e., everything where the debugger gets its commands from.

So, if we assume everything I said above is true, then capturing the command stream produced by processing the command-line arguments should work the same way as capturing the commands given interactively. So, perhaps the first step would be to make sure you can capture the commands issued via command-line args. This should be easily testable via something like:

RUN: %lldb --record %t -b -o cmd1 -o cmd2 -O cmd3 ...
RUN: %lldb --replay %t | FileCheck %s
CHECK: make sure cmd1, cmd2, cmd3 got executed

Of course, capturing *real* stdin will come with additional challenges, both for recording and testing. For recording, we'll have to decide whether to try to capture raw stdin with enough fidelity so libedit can be driven through it, or we go one level higher, and try to capture the "cooked" command stream coming out of libedit. The testing strategy will depend on decision made here. In case we capture cooked commands, the situation should be very similar to command line options, so it might be enough to test by just passing the commands to the driver via stdin (%lldb <%t/my_commands.txt) instead of via command line args. In case of low-level recordings, we may need to test using something which can provide a realistic-looking fake terminal such as pexpect (yikes). But even in that case, that should be only necessary to test the handling of funny line-editing operations (such as backspaces, cursor keys, etc.), and for general functionality redirecting stdin from file should be enough.

Hm.. so the way, I'd imagine this working is that -o and friends (as well as pretty much every other command line argument) would be captured by the SB recorder, once they cross the SB boundary. For the -o commands that would be the moment the driver calls m_debugger.SetInputFileHandle https://github.com/llvm-mirror/lldb/blob/master/tools/driver/Driver.cpp#L697 , followed by m_debugger.RunCommandInterpreter a couple of lines lower. So, the SB recorder would notice that we're changing what the debugger considers as it's stdin, and direct the "stdin" recorder (or create a new instance of it, or whatever), to capture it.

Then, during replay, the SB replayer would notice that the driver called SBDebugger::SetInputFileHandle, so it would create a fake FILE* object (or maybe just open the file containing the captured commands from the disk?), and pass *that* as the stdin handle. Then it could just call SBDebugger::RunCommandInterperer the same way as the driver did. And I am hoping that pretty much everything below RunCommandInterpreter could run oblivious to the fact that replay is taking place.

Yes, this is how SB record/replay will work. I don't disagree with this
approach, it's just that, as I said in my earlier reply, I was hoping to get
the driver working without the need for the SB reproducers. Maybe I should give
up on that?

In this (imaginary) world, the recording/replay would only deal with the things that happen below the SB API. Anything that happens above it could only be glimpsed through it's effects on the SB. So, when initializing replay, as soon the driver notices the --replay argument it would just load that file, call RunReplay and be done. There wouldn't be a need for any command-line-argument recorder, or making sure the command line arguments match in some other way.

By "capturing stdin" I meant capturing anything which the RunCommandInterpreter considers as it's "stdin", i.e., everything where the debugger gets its commands from.

This is the tricky part though. Where and how are you going to do this? In my
opinion this shouldn't be the responsibility of the SB recorder. The latter
should be concerned with "serializing" and "deserializing" its input and
tracking what SB layer calls are being made. It should be relatively agnostic
of what the debugger does with that at a lower level.

The FILE* is particularly annoying. For the sake of argument let's say that
"serializing" a FILE* means writing its content to the reproducer, how would
we accomplish this? Maybe there's an API I don't know about, but wouldn't we
have to do this at the fgets level? For the command interpreter this is
either in libedit or in the IO handler (depending on the fidelity you touched
on below).

So, if we assume everything I said above is true, then capturing the command stream produced by processing the command-line arguments should work the same way as capturing the commands given interactively. So, perhaps the first step would be to make sure you can capture the commands issued via command-line args. This should be easily testable via something like:

RUN: %lldb --record %t -b -o cmd1 -o cmd2 -O cmd3 ...
RUN: %lldb --replay %t | FileCheck %s
CHECK: make sure cmd1, cmd2, cmd3 got executed

Of course, capturing *real* stdin will come with additional challenges, both for recording and testing. For recording, we'll have to decide whether to try to capture raw stdin with enough fidelity so libedit can be driven through it, or we go one level higher, and try to capture the "cooked" command stream coming out of libedit. The testing strategy will depend on decision made here. In case we capture cooked commands, the situation should be very similar to command line options, so it might be enough to test by just passing the commands to the driver via stdin (%lldb <%t/my_commands.txt) instead of via command line args. In case of low-level recordings, we may need to test using something which can provide a realistic-looking fake terminal such as pexpect (yikes). But even in that case, that should be only necessary to test the handling of funny line-editing operations (such as backspaces, cursor keys, etc.), and for general functionality redirecting stdin from file should be enough.

I don't think we really differentiate between *real* stdin and a file. Even
when sourcing a file go through the IOHandler, so that should be pretty
transparent, unless you actually want to differentiate which, if I understand
correctly, wouldn't be needed with your proposed approach?

Yes, this is how SB record/replay will work. I don't disagree with this
approach, it's just that, as I said in my earlier reply, I was hoping to get
the driver working without the need for the SB reproducers. Maybe I should give
up on that?

Hm... I don't know.. as this patch shows, it seems it is possible to get some results without the SB reproducers. My fear is that this will produce a solution which is very specialized to this particular use case, and which will have to be redone once you want more functionality out of it. Maybe that doesn't matter? I'm not sure..

In this (imaginary) world, the recording/replay would only deal with the things that happen below the SB API. Anything that happens above it could only be glimpsed through it's effects on the SB. So, when initializing replay, as soon the driver notices the --replay argument it would just load that file, call RunReplay and be done. There wouldn't be a need for any command-line-argument recorder, or making sure the command line arguments match in some other way.

By "capturing stdin" I meant capturing anything which the RunCommandInterpreter considers as it's "stdin", i.e., everything where the debugger gets its commands from.

This is the tricky part though. Where and how are you going to do this? In my
opinion this shouldn't be the responsibility of the SB recorder. The latter
should be concerned with "serializing" and "deserializing" its input and
tracking what SB layer calls are being made. It should be relatively agnostic
of what the debugger does with that at a lower level.

The FILE* is particularly annoying. For the sake of argument let's say that
"serializing" a FILE* means writing its content to the reproducer, how would
we accomplish this? Maybe there's an API I don't know about, but wouldn't we
have to do this at the fgets level? For the command interpreter this is
either in libedit or in the IO handler (depending on the fidelity you touched
on below).

Yes, the FILE* is quite tricky, and it might be the trickiest part of capturing the SB interface. I would actually argue that capturing this *is* in the scope of the SB recorder, which I would define as capturing the data that flows across the SB boundary. For a FILE*, that obviously doesn't mean the pointer value, because that's going to be meaningless in subsequent runs. It has do do a "deep" capture of the FILE* so that it can later be recreated with reasonable fidelity.

In an ideal world, it would just take a snapshot of the file, and then move on, but that is of course not possible, because the FILE* may be a stream, and it would have to look into the future to capture it instantaneously. So, it will have to capture it incrementally, as it is being read. Which will require some cooperation from the lower layers of the debugger.

I'd imagine this could be done by adding a level of indirection, similar to how you've made all filesystem accesses go through the FileSystem class. Instead of FILE*. everything would deal with some proxy class, which would record it's interaction into the log in record mode and replay it from saved data in replay mode. Most of the code would still be oblivious to the fact that anything funny is going on, but it would have to be changed to deal with the proxy class instead of the raw APIs.

Of course, capturing *real* stdin will come with additional challenges, both for recording and testing. For recording, we'll have to decide whether to try to capture raw stdin with enough fidelity so libedit can be driven through it, or we go one level higher, and try to capture the "cooked" command stream coming out of libedit. The testing strategy will depend on decision made here. In case we capture cooked commands, the situation should be very similar to command line options, so it might be enough to test by just passing the commands to the driver via stdin (%lldb <%t/my_commands.txt) instead of via command line args. In case of low-level recordings, we may need to test using something which can provide a realistic-looking fake terminal such as pexpect (yikes). But even in that case, that should be only necessary to test the handling of funny line-editing operations (such as backspaces, cursor keys, etc.), and for general functionality redirecting stdin from file should be enough.

I don't think we really differentiate between *real* stdin and a file. Even
when sourcing a file go through the IOHandler, so that should be pretty
transparent, unless you actually want to differentiate which, if I understand
correctly, wouldn't be needed with your proposed approach?

I am afraid you lost me there. I am not sure what you wanted to say, but yes, I am saying that (for the most part) we don't have to differentiate between a file and real (tty) stdin.

JDevlieghere abandoned this revision.Jan 30 2019, 2:42 PM

Abandoning this in favor of D56322