Remove RETURNDATACOPY elimination from UnusedStoreEliminator
#16508
There are no files selected for viewing
|
Collaborator
There was a problem hiding this comment. Looks like removing this did not affect gas in any of our semantic tests? Please check gas benchmarks from external tests though.
Contributor
Author
There was a problem hiding this comment. relative
|
| project | bytecode_size | deployment_gas | method_gas |
|---|---|---|---|
| brink | 0% |
||
| colony | 0% |
||
| elementfi | 0% |
||
| ens | 0% |
||
| euler | |||
| gnosis | |||
| gp2 | 0% |
||
| pool-together | 0% |
||
| uniswap | 0% |
+0% |
+0% |
| yield_liquidator | 0% |
+0% |
+0% |
| zeppelin |
ir-optimize-evm+yul
| project | bytecode_size | deployment_gas | method_gas |
|---|---|---|---|
| brink | 0% |
||
| colony | 0% |
||
| elementfi | 0% |
||
| ens | 0% |
0% |
0% |
| euler | 0% |
-0% |
+0% |
| gnosis | |||
| gp2 | 0% |
||
| pool-together | 0% |
||
| uniswap | 0% |
+0% |
-0% |
| yield_liquidator | 0% |
-0% |
0% |
| zeppelin | 0% |
-0% |
+0% |
ir-optimize-evm-only
| project | bytecode_size | deployment_gas | method_gas |
|---|---|---|---|
| brink | 0% |
||
| colony | 0% |
||
| elementfi | 0% |
||
| ens | 0% |
0% |
0% |
| euler | |||
| gnosis | |||
| gp2 | 0% |
||
| pool-together | 0% |
||
| uniswap | 0% |
0% |
+0% |
| yield_liquidator | 0% |
0% |
0% |
| zeppelin | 0% |
legacy-no-optimize
| project | bytecode_size | deployment_gas | method_gas |
|---|---|---|---|
| brink | 0% |
||
| colony | 0% |
||
| elementfi | 0% |
||
| ens | 0% |
||
| euler | 0% |
+0% |
-0% |
| gnosis | 0% |
||
| gp2 | 0% |
||
| pool-together | 0% |
||
| uniswap | 0% |
+0% |
+0% |
| yield_liquidator | 0% |
-0% |
0% |
| zeppelin |
legacy-optimize-evm+yul
| project | bytecode_size | deployment_gas | method_gas |
|---|---|---|---|
| brink | 0% |
||
| colony | 0% |
||
| elementfi | 0% |
||
| ens | 0% |
0% |
0% |
| euler | 0% |
-0% |
-0% |
| gnosis | 0% |
||
| gp2 | 0% |
||
| pool-together | 0% |
||
| uniswap | 0% |
+0% |
-0% |
| yield_liquidator | 0% |
-0% |
0% |
| zeppelin | 0% |
+0% |
+0% |
legacy-optimize-evm-only
| project | bytecode_size | deployment_gas | method_gas |
|---|---|---|---|
| brink | 0% |
||
| colony | 0% |
||
| elementfi | 0% |
||
| ens | 0% |
0% |
0% |
| euler | 0% |
+0% |
-0% |
| gnosis | 0% |
||
| gp2 | 0% |
||
| pool-together | 0% |
||
| uniswap | 0% |
-0% |
+0% |
| yield_liquidator | 0% |
0% |
0% |
| zeppelin |
!V = version mismatch
!B = no value in the "before" version
!A = no value in the "after" version
!T = one or both values were not numeric and could not be compared
-0 = very small negative value rounded to zero
+0 = very small positive value rounded to zero
absolute
ir-no-optimize
| project | bytecode_size | deployment_gas | method_gas |
|---|---|---|---|
| brink | 0 | ||
| colony | 0 | ||
| elementfi | 0 | ||
| ens | 0 | ||
| euler | |||
| gnosis | |||
| gp2 | 0 | ||
| pool-together | 0 | ||
| uniswap | 0 | 36 | 82 |
| yield_liquidator | 0 | 12 | 72 |
| zeppelin |
ir-optimize-evm+yul
| project | bytecode_size | deployment_gas | method_gas |
|---|---|---|---|
| brink | 0 | ||
| colony | 0 | ||
| elementfi | 0 | ||
| ens | 0 | 0 | 0 |
| euler | 0 | -24 | 5972 |
| gnosis | |||
| gp2 | 0 | ||
| pool-together | 0 | ||
| uniswap | 0 | 48 | -103 |
| yield_liquidator | 0 | -36 | 0 |
| zeppelin | 0 | -24 | 121 |
ir-optimize-evm-only
| project | bytecode_size | deployment_gas | method_gas |
|---|---|---|---|
| brink | 0 | ||
| colony | 0 | ||
| elementfi | 0 | ||
| ens | 0 | 0 | 0 |
| euler | |||
| gnosis | |||
| gp2 | 0 | ||
| pool-together | 0 | ||
| uniswap | 0 | 0 | 3955 |
| yield_liquidator | 0 | 0 | 0 |
| zeppelin | 0 |
legacy-no-optimize
| project | bytecode_size | deployment_gas | method_gas |
|---|---|---|---|
| brink | 0 | ||
| colony | 0 | ||
| elementfi | 0 | ||
| ens | 0 | ||
| euler | 0 | 12 | -12110 |
| gnosis | 0 | ||
| gp2 | 0 | ||
| pool-together | 0 | ||
| uniswap | 0 | 36 | 380 |
| yield_liquidator | 0 | -12 | 0 |
| zeppelin |
legacy-optimize-evm+yul
| project | bytecode_size | deployment_gas | method_gas |
|---|---|---|---|
| brink | 0 | ||
| colony | 0 | ||
| elementfi | 0 | ||
| ens | 0 | 0 | 0 |
| euler | 0 | -48 | -18878 |
| gnosis | 0 | ||
| gp2 | 0 | ||
| pool-together | 0 | ||
| uniswap | 0 | 48 | -1614 |
| yield_liquidator | 0 | -12 | 0 |
| zeppelin | 0 | 100 | 1426 |
legacy-optimize-evm-only
| project | bytecode_size | deployment_gas | method_gas |
|---|---|---|---|
| brink | 0 | ||
| colony | 0 | ||
| elementfi | 0 | ||
| ens | 0 | 0 | 0 |
| euler | 0 | 24 | -10010 |
| gnosis | 0 | ||
| gp2 | 0 | ||
| pool-together | 0 | ||
| uniswap | 0 | -12 | 882 |
| yield_liquidator | 0 | 0 | 0 |
| zeppelin |
!V = version mismatch
!B = no value in the "before" version
!A = no value in the "after" version
!T = one or both values were not numeric and could not be compared
-0 = very small negative value rounded to zero
+0 = very small positive value rounded to zero
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| --ir-optimized --optimize --debug-info none --no-cbor-metadata |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,12 @@ | ||||||
| // SPDX-License-Identifier: GPL-3.0 | ||||||
| pragma solidity >=0.8.0; | ||||||
|
Collaborator
There was a problem hiding this comment. We don't hard-code versions in these.
Suggested change
Collaborator
There was a problem hiding this comment. Also, I'd group it with the other Yul optimizer tests: And the comment should also say that |
||||||
|
|
||||||
| contract C { | ||||||
| function f() public pure { | ||||||
| assembly { | ||||||
| // Destination offset 96 is chosen to make the written memory region [96, 96+returndatasize()) | ||||||
| // provably disjoint from the ABI encoder's mload(64) (which reads [64, 96)). | ||||||
| returndatacopy(96, 0, returndatasize()) | ||||||
|
Collaborator
There was a problem hiding this comment. This verifies that the I expected something like the repro from the report. Which I guess would be this: contract C {
function f() public view {
assembly {
let s := returndatasize()
pop(staticcall(gas(), 0x01, 0, 0, 0, 0))
returndatacopy(0, 0, s)
}
}
}I see that this does not reproduce the error though. Not even with the sequence we use in Yul tests for UnusedStoreEliminator ( This puts into question if the bug can even be triggered. A buggy path definitely exists, but if there's no input that can exercise it, I would not classify it as an important bug.
Collaborator
There was a problem hiding this comment. Looks like the repro does work in pure Yul: {
let s := returndatasize()
pop(staticcall(gas(), 0x01, 0, 0, 0, 0))
returndatacopy(0, 0, s)
}So the bug is real after all. But the question still remains if it's reproducible in inline assembly. If it's not then I'd still consider demoting it. We also still need a non-Yul version of the test to prove that affects evmasm as well (otherwise buglist entry needs
Collaborator
There was a problem hiding this comment. BTW, I'd put the repro under
Contributor
Author
There was a problem hiding this comment. I added semantic test which triggers the bug.
Contributor
Author
There was a problem hiding this comment. Will move it to
Contributor
Author
There was a problem hiding this comment. Actually I think we should create the tests as you mentioned but the semantic test should stay too. It checks that the call reverts. cmdLineTest only verifies that the |
||||||
| } | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| Optimized IR: | ||
| /// @use-src 0:"input.sol" | ||
| object "C_7" { | ||
| code { | ||
| { | ||
| let _1 := memoryguard(0x80) | ||
| mstore(64, _1) | ||
| if callvalue() { revert(0, 0) } | ||
| let _2 := datasize("C_7_deployed") | ||
| codecopy(_1, dataoffset("C_7_deployed"), _2) | ||
| return(_1, _2) | ||
| } | ||
| } | ||
| /// @use-src 0:"input.sol" | ||
| object "C_7_deployed" { | ||
| code { | ||
| { | ||
| mstore(64, 128) | ||
| if iszero(lt(calldatasize(), 4)) | ||
| { | ||
| if eq(0x26121ff0, shr(224, calldataload(0))) | ||
| { | ||
| if callvalue() { revert(0, 0) } | ||
| if slt(add(calldatasize(), not(3)), 0) { revert(0, 0) } | ||
| returndatacopy(96, 0, returndatasize()) | ||
| return(0, 0) | ||
| } | ||
| } | ||
| revert(0, 0) | ||
| } | ||
| } | ||
| data ".metadata" hex"" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| contract C { | ||
| function e1() external returns(uint256) { | ||
| return 1; | ||
| } | ||
|
|
||
| function e() external {} | ||
|
|
||
| // This test verifies that UnusedStoreEliminator does not incorrectly eliminate | ||
| // a returndatacopy whose size argument is a stale snapshot of returndatasize() | ||
| // taken before a subsequent call that resets the return data buffer. | ||
| // | ||
| // The test only detects the bug when run with --optimize. | ||
| // | ||
| // The destination offset 96 is chosen so that the written region [96, 128) | ||
| // is provably disjoint from the ABI encoder's mload(64) at [64, 96). | ||
| function f() public { | ||
| assembly { | ||
| mstore(0x00, shl(0xe0, 0xa2c2d666)) // bytes4(keccak256("e1()")) | ||
| pop(staticcall(gas(), address(), 0x00, 0x04, 0x00, 0x00)) | ||
| let rds := returndatasize() // rds == 32 (return value of e1()) | ||
| mstore(0x00, shl(0xe0, 0xffae15ba)) // bytes4(keccak256("e()")) | ||
| pop(staticcall(gas(), address(), 0x00, 0x04, 0x00, 0x00)) | ||
| // After e(), returndatasize() == 0, but rds is stale (== 32). | ||
| // The out-of-bounds access reverts. | ||
| returndatacopy(96, 0x00, rds) | ||
| } | ||
| } | ||
| } | ||
| // ---- | ||
| // f() -> FAILURE |
|
cameel marked this conversation as resolved.
cameel marked this conversation as resolved.
cameel marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| // This test ensures that `returndatacopy` is NOT optimized away. | ||
| { | ||
| returndatacopy(0,0,returndatasize()) | ||
| } | ||
|
|
@@ -8,8 +9,6 @@ | |
| // | ||
| // { | ||
| // { | ||
| // returndatacopy(0, 0, returndatasize()) | ||
| // } | ||
| // } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| // This optimisation step is removed and `returndatacopy` is never removed by `unusedStoreEliminator`. | ||
| { | ||
| returndatacopy(0,1,returndatasize()) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| // This test ensures that `returndatacopy` is NOT optimized away. | ||
| { | ||
| let s := returndatasize() | ||
| returndatacopy(0,0,s) | ||
|
|
@@ -9,8 +10,6 @@ | |
| // | ||
| // { | ||
| // { | ||
| // returndatacopy(0, 0, returndatasize()) | ||
| // } | ||
| // } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The release is currently planned for the 29th.
This should be corrected in the blog post as well.