[Decompiler] Expressions capturing bad side effects by accident

[water111]

Here is a case that is not done properly in the current decompiler, due to a really stupid mistake when writing out the "rules" of decompilation.

(set! x (if a b (begin (set! y z) c)))
(* y x)

It will do the simplification

(* y (if a b (begin (set! y z) c)))

which isn't correct - for the first y the compiler would use the value of y from before the if statement. GOAL always evaluates left-to-right.

The decompiler uses this logic: "From left to right, check if there is an expression in either operand; if so, evaluate it to a register. Then multiply registers". This is wrong - if one of the operands is a register, it must be the value of that register before subsequent arguments.

The issue is that I take a statement like (* r1 r2), and I look at the value of r1 and of r2 at the multiplication instruction, or, more specifically, after all subexpressions are evaluated. This doesn't really make sense. We really should look at the value of r1 before whatever expressions is used for r2, even if r1 ends up being a register.

I think fixing this is as easy as saying "pop r2, but enforce that the code we pop does not modify r1". This trivially ensures that the value of r1 that the decompiler uses is the value before r1's expression is evaluated. The only case where GOAL can get tangled up like this is if there's an eliminated move, and in this case, we do want to give up on expression building.

A good way to trigger this bug is to disable if condition extracting and look at the new array method.

[water111]

Fixed for this case in #218

I still think there's a possibility for this to go wrong if a form accidentally does separate calls to pop from the input stack. I should somehow guard against this and verify that this never happens in FormExpressionAnalysis before closing this.

[water111]

Fixed (conservatively) for the second case in #227 .
Solution was to disallow anything with a set! from being popped when you think you may need to do multiple separate pops for things on the same level.

I don't like this solution and I think I should reorganize this to avoid having a case where we need a workaround, so leaving it open. We can call this fixed when we make update_from_stack do nothing for a GenericElement and a SetFormFormElement. Leaving open for now.

[water111]

Closing because we haven't hit this type of issue in months