smite-ir: Add InstructionReorderMutator

[Chand-ra]

Add an InstructionReorder mutator for Smite IR. Mutates a given program by swapping two '"Act" instructions that have no data dependencies between them. This explores alternative execution orderings while preserving SSA invariants.

[Chand-ra]

Drafting right now because tests for the mutator are yet to be implemented.

[morehouse]

This all looks correctly implemented to me.

Unfortunately I don't think it's useful until we implement more Act operations and start generating more interesting programs. Currently we just have SendMessage and RecvAcceptChannel, which we actually don't want to swap (doing so would lead to 5s timeout delays as we wait to receive a message that never comes, which would register as a hang).

I think it would be best to defer further work on this mutator until we have some more interesting things to swap. Then we can figure out how to avoid swapping a Recv* operation to a location before a SendMessage. We will also then be able to collect better data about how often swapping is actually possible with the current implementation, which may lead us to investigate further changes to this mutator (e.g., moving entire dependency trees).

[morehouse]

I think this is a good first implementation of a reordering mutator. Currently it isn't useful, since we never generate programs with independent act operations, but once we have splice or generator-insertion mutators it should be more useful.

I also think it would be worth evaluating this mutator against an alternative implementation that moves and inserts an act operation elsewhere instead of swapping. That seems like a more general mutation that may be more effective.

[morehouse]

has_side_effects is almost identical to this function. We should add a comment explaining why we need a different function.

[Chand-ra]

This confused me as well. Do we need a different function, instead of making do with has_side_effects()? Here's how Act instructions are defined:

/ -- Act: side effects against the target --

The only difference between the two is that is_act() returns false for CreateFundingTransaction and has_side_effects() returns true.

This is because CreateFundingTransaction isn't mentioned in the list of Act instructions, but I think it should because it interacts with the Bitcoin CLI similar to MineBlocks and MineBlocks is listed as an Act instruction.

[morehouse]

Either way is fine with me.

Really the only benefit of having a separate function is to remove any Create* operations that only interact with the Bitcoin CLI and not the target. In practice I'd expect such functions to have minimal effect on the target itself, and I'm not sure rearranging them will make a big difference. For corpus minimization we might care more about the side effects because we're trying to preserve the exact same coverage on the target (or else AFL++ rejects the minimization).

[Chand-ra]

I'll go ahead and drop is_act().

1. I don't see a point in artificially separating operations that perform Bitcoin CLI interactions.
2. Maintaining two methods that are 99% identical sounds like a maintenance trap.
3. It may be that swapping such functions has minimal effect on the target itself, but we're not decreasing the mutator's surface area. Since the change is a net-positive, or neutral at worst, I think it's worth making.

[Chand-ra]

I also think it would be worth evaluating this mutator against an alternative implementation that moves and inserts an act operation elsewhere instead of swapping. That seems like a more general mutation that may be more effective.

Hmm, not very sure about this. The target does not, after all, care about whether we LoadBytes or DerivePoint before sending it a message or mining blocks. I do agree that it is a more general version of this mutator. Maybe there's some edge case that I cannot think of.

[morehouse]

I also think it would be worth evaluating this mutator against an alternative implementation that moves and inserts an act operation elsewhere instead of swapping. That seems like a more general mutation that may be more effective.

Hmm, not very sure about this. The target does not, after all, care about whether we LoadBytes or DerivePoint before sending it a message or mining blocks. I do agree that it is a more general version of this mutator. Maybe there's some edge case that I cannot think of.

Here's an example where the insertion mutator could mutate the program in ways a swap mutator could not:

v1 = BuildOpenChannel(...)
v2 = SendOpenChannel(v1)
v3 = RecvAcceptChannel(v2)
v4 = ExtractAcceptChannelChannelType(v3)
MineBlocks(6)

The swap mutator cannot modify the program at all. The insertion mutator can move MineBlocks to any point between or before the other Act operations.

[Chand-ra]

Will rebase after review.

[morehouse]

Ready to rebase

[Chand-ra]

Rebased on top of the latest master.

[morehouse]

LGTM.

Smoke tested against LDK IR scenario for 10 minutes.