Return and modifiers

[dfilaretti]

This gives a different implementation to return so that it behaves as expected, especially in interaction with function modifiers.

Previously, Solidity's return was simply compiled into IELE's ret. Now things are a bit different; essentially, we compile return into jumps (to appropriate targets that we generate), and only at the end we eventually reach a ret (of which there's only 1 per function).

Before, we used to insert a "default" return at the end of each function, but only if the function didn't already have one; now, we always do this; indeed, this automatically inserted return is the one that will eventually be reached and perform the actual return operation; all other, user-defined returns will be compiled into jumps and assignments of the return value to the return parameters (declared in function definition).

To keep track of the return value, we use the explicit or implicit return parameters.
The idea is that "return behaves as break" (see e.g. ethereum/solidity#686) so that when a function returns, the remaining statements (if any) in the modifier are still executed.

See comments below for some examples of the kind of code it produces.

[dfilaretti]

Example 1 (function_modifier_multiple_times_local_vars)

Source:

contract C {
    uint public a;
    
    modifier mod(uint x) { 
        uint b = x; 
        a += b; 
        _; 
        a -= b; 
        assert(b == x); 
    }
    
    function f(uint x) mod(2) mod(5) mod(x) returns(uint) { 
        return a; 
    }
}

Compiles to:

contract "/home/dnf/Documents/playground.sol:C" {

@"a" = 0

define public @"a()"() {
entry:
  %callvalue = call @iele.callvalue()
  br %callvalue, throw
  %a.val = sload @"a"
  ret %a.val

throw:
  revert -1
}

define public @"f(uint256)"(%x_0) {
entry:
  %callvalue = call @iele.callvalue()
  br %callvalue, throw
  %out.of.range = cmp lt %x_0, 0
  br %out.of.range, throw
  %x_2 = 2
  %b_3 = %x_2
  %tmp = sload @"a"
  %tmp1 = add %tmp, %b_3
  sstore %tmp1, @"a"
  %x_4 = 5
  %b_5 = %x_4
  %tmp2 = sload @"a"
  %tmp3 = add %tmp2, %b_5
  sstore %tmp3, @"a"
  %x_6 = %x_0
  %b_7 = %x_6
  %tmp4 = sload @"a"
  %tmp5 = add %tmp4, %b_7
  sstore %tmp5, @"a"
  %a.val = sload @"a"
  %_1 = %a.val
  br ret_jmp_dest

ret_jmp_dest:
  %tmp6 = sload @"a"
  %tmp7 = cmp lt %tmp6, %b_7
  br %tmp7, throw
  %tmp8 = sub %tmp6, %b_7
  sstore %tmp8, @"a"
  %tmp9 = cmp eq %b_7, %x_6
  %tmp10 = iszero %tmp9
  br %tmp10, invalid

ret_jmp_dest11:
  %tmp12 = sload @"a"
  %tmp13 = cmp lt %tmp12, %b_5
  br %tmp13, throw
  %tmp14 = sub %tmp12, %b_5
  sstore %tmp14, @"a"
  %tmp15 = cmp eq %b_5, %x_4
  %tmp16 = iszero %tmp15
  br %tmp16, invalid

ret_jmp_dest17:
  %tmp18 = sload @"a"
  %tmp19 = cmp lt %tmp18, %b_3
  br %tmp19, throw
  %tmp20 = sub %tmp18, %b_3
  sstore %tmp20, @"a"
  %tmp21 = cmp eq %b_3, %x_2
  %tmp22 = iszero %tmp21
  br %tmp22, invalid

return:
  ret %_1

throw:
  revert -1

invalid:
}

define @init() {
entry:
  ret void
}

}

[dfilaretti]

Example 2 (return_does_not_skip_modifier)

contract C {
    uint public x;
    modifier setsx {
        _;
        x = 9;
    }
    function f() setsx returns (uint) {
        return 2;
    }
}

Compiles to

contract "/home/dnf/Documents/playground.sol:C" {

@"x" = 0

define public @"x()"() {
entry:
  %callvalue = call @iele.callvalue()
  br %callvalue, throw
  %x.val = sload @"x"
  ret %x.val

throw:
  revert -1
}

define public @"f()"() {
entry:
  %callvalue = call @iele.callvalue()
  br %callvalue, throw
  %_0 = 2
  br ret_jmp_dest

ret_jmp_dest:
  sstore 9, @"x"

return:
  ret %_0

throw:
  revert -1
}

define @init() {
entry:
  ret void
}

}

[theo25]

Functionality looks good but I requested some changes related to choice of data structures and style. See my comments below.

[theo25]

I don't think this has to be a map. Couldn't it be a std::vector or even a std::stack? Also there is no reseting or clearing this. Are you sure it would not cause any bugs? Using an actual stack would be more intuitive and safer I think.

[dfilaretti]

Actually, using a stack was my initial plan, but it didn't work; I tried a few things and, eventually, a map from Modifier Depth, which worked, so it stayed that way.

I tried again using a stack but it doesn't work.
I agree at this point it would be useful to get to the bottom of it and fully understand what's going on.

I'll investigate in more detail and post updates here.

PS: other 2 minor comments are now addressed.

[theo25]

to keep things uniform I think you should be clearing this at the end of the visitor together with the rest of the reset code (line 455 and after)

[theo25]

I would rename this to CompilingFunctionReturnParameters and reset it at the end of the function visitor as mentioned above.

[dfilaretti]

@theo25 actually managed to replace the map with stack as suggested. Ready to review again.

[theo25]

The changes look good to me.

[dfilaretti]

Jenkins: test this please

[dfilaretti]

Looks like we have 2 new failures after merge:

<TestCase name="byte_arrays" result="failed" assertions_passed="73" assertions_failed="4" expected_failures="1"/><TestCase name="short_input_bytes" result="failed" assertions_passed="114" assertions_failed="4" expected_failures="1"/>

[dfilaretti]

Jenkins: test this please