From 7b84e141dc943e28f73d3737af8716048ee0594d Mon Sep 17 00:00:00 2001 From: chriseth Date: Tue, 17 Dec 2019 13:40:42 +0100 Subject: [PATCH 1/6] Descend into for loops when determining call graph. --- libyul/optimiser/CallGraphGenerator.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libyul/optimiser/CallGraphGenerator.cpp b/libyul/optimiser/CallGraphGenerator.cpp index b1dc62f97fc6..07a381cc9aa6 100644 --- a/libyul/optimiser/CallGraphGenerator.cpp +++ b/libyul/optimiser/CallGraphGenerator.cpp @@ -50,9 +50,10 @@ void CallGraphGenerator::operator()(FunctionCall const& _functionCall) ASTWalker::operator()(_functionCall); } -void CallGraphGenerator::operator()(ForLoop const&) +void CallGraphGenerator::operator()(ForLoop const& _forLoop) { m_callGraph.functionsWithLoops.insert(m_currentFunction); + ASTWalker::operator()(_forLoop); } void CallGraphGenerator::operator()(FunctionDefinition const& _functionDefinition) From 143471fe8714e5360547f7fcda3793ad5ef7b19d Mon Sep 17 00:00:00 2001 From: chriseth Date: Tue, 17 Dec 2019 13:41:56 +0100 Subject: [PATCH 2/6] Tests. --- .../functionSideEffects/mload_in_function.yul | 11 +++++++ .../fullSuite/abi_example1.yul | 19 ++++++------ .../loadResolver/mload_in_function.yul | 29 +++++++++++++++++++ 3 files changed, 50 insertions(+), 9 deletions(-) create mode 100644 test/libyul/functionSideEffects/mload_in_function.yul create mode 100644 test/libyul/yulOptimizerTests/loadResolver/mload_in_function.yul diff --git a/test/libyul/functionSideEffects/mload_in_function.yul b/test/libyul/functionSideEffects/mload_in_function.yul new file mode 100644 index 000000000000..170535926b04 --- /dev/null +++ b/test/libyul/functionSideEffects/mload_in_function.yul @@ -0,0 +1,11 @@ +{ + function foo(x) { + for {} x { x := mload(0) mstore(0, 0)} {} + } + mstore(0, 1337) + foo(42) + sstore(0, mload(0)) +} +// ---- +// : invalidatesStorage, invalidatesMemory +// foo: invalidatesMemory diff --git a/test/libyul/yulOptimizerTests/fullSuite/abi_example1.yul b/test/libyul/yulOptimizerTests/fullSuite/abi_example1.yul index ab1ebb10da8d..0d37cf1b9df5 100644 --- a/test/libyul/yulOptimizerTests/fullSuite/abi_example1.yul +++ b/test/libyul/yulOptimizerTests/fullSuite/abi_example1.yul @@ -477,15 +477,16 @@ // pos := add(pos, 0x60) // } // let _3 := mload(64) -// if slt(sub(_3, length), 128) { revert(_1, _1) } -// let offset := calldataload(add(length, 64)) -// let _4 := 0xffffffffffffffff -// if gt(offset, _4) { revert(_1, _1) } -// let value2 := abi_decode_t_array$_t_uint256_$dyn_memory_ptr(add(length, offset), _3) -// let offset_1 := calldataload(add(length, 0x60)) -// if gt(offset_1, _4) { revert(_1, _1) } -// let value3 := abi_decode_t_array$_t_array$_t_uint256_$2_memory_$dyn_memory_ptr(add(length, offset_1), _3) -// sstore(calldataload(length), calldataload(add(length, 0x20))) +// let _4 := mload(0x20) +// if slt(sub(_3, _4), 128) { revert(_1, _1) } +// let offset := calldataload(add(_4, 64)) +// let _5 := 0xffffffffffffffff +// if gt(offset, _5) { revert(_1, _1) } +// let value2 := abi_decode_t_array$_t_uint256_$dyn_memory_ptr(add(_4, offset), _3) +// let offset_1 := calldataload(add(_4, 0x60)) +// if gt(offset_1, _5) { revert(_1, _1) } +// let value3 := abi_decode_t_array$_t_array$_t_uint256_$2_memory_$dyn_memory_ptr(add(_4, offset_1), _3) +// sstore(calldataload(_4), calldataload(add(_4, 0x20))) // sstore(value2, value3) // sstore(_1, pos) // } diff --git a/test/libyul/yulOptimizerTests/loadResolver/mload_in_function.yul b/test/libyul/yulOptimizerTests/loadResolver/mload_in_function.yul new file mode 100644 index 000000000000..9bb8261dd836 --- /dev/null +++ b/test/libyul/yulOptimizerTests/loadResolver/mload_in_function.yul @@ -0,0 +1,29 @@ +{ + function foo(x) { + for {} x { x := mload(0) mstore(0, 0)} {} + } + mstore(0, 1337) + foo(42) + sstore(0, mload(0)) +} +// ==== +// step: loadResolver +// ---- +// { +// function foo(x) +// { +// for { } +// x +// { +// let _1 := 0 +// x := mload(_1) +// mstore(_1, _1) +// } +// { } +// } +// let _4 := 1337 +// let _5 := 0 +// mstore(_5, _4) +// foo(42) +// sstore(_5, mload(_5)) +// } From 244b011a30ca984e4c572f0e9cb8c751d09985e0 Mon Sep 17 00:00:00 2001 From: chriseth Date: Tue, 17 Dec 2019 17:08:07 +0100 Subject: [PATCH 3/6] More test cases. --- .../mstore_in_function_loop_body.yul | 35 +++++++++++++++++++ .../mstore_in_function_loop_init.yul | 32 +++++++++++++++++ 2 files changed, 67 insertions(+) create mode 100644 test/libyul/yulOptimizerTests/loadResolver/mstore_in_function_loop_body.yul create mode 100644 test/libyul/yulOptimizerTests/loadResolver/mstore_in_function_loop_init.yul diff --git a/test/libyul/yulOptimizerTests/loadResolver/mstore_in_function_loop_body.yul b/test/libyul/yulOptimizerTests/loadResolver/mstore_in_function_loop_body.yul new file mode 100644 index 000000000000..2de873942867 --- /dev/null +++ b/test/libyul/yulOptimizerTests/loadResolver/mstore_in_function_loop_body.yul @@ -0,0 +1,35 @@ +{ + function userNot(x) -> y { + y := iszero(x) + } + + function funcWithLoop(x) { + for {} userNot(x) { mstore(0, 0) } {} + } + + mstore(0, 1337) + funcWithLoop(42) + sstore(0, mload(0)) +} +// ==== +// step: loadResolver +// ---- +// { +// function userNot(x) -> y +// { y := iszero(x) } +// function funcWithLoop(x_1) +// { +// for { } +// userNot(x_1) +// { +// let _1 := 0 +// mstore(_1, _1) +// } +// { } +// } +// let _3 := 1337 +// let _4 := 0 +// mstore(_4, _3) +// funcWithLoop(42) +// sstore(_4, mload(_4)) +// } diff --git a/test/libyul/yulOptimizerTests/loadResolver/mstore_in_function_loop_init.yul b/test/libyul/yulOptimizerTests/loadResolver/mstore_in_function_loop_init.yul new file mode 100644 index 000000000000..1e06cf8df3e3 --- /dev/null +++ b/test/libyul/yulOptimizerTests/loadResolver/mstore_in_function_loop_init.yul @@ -0,0 +1,32 @@ +{ + function userNot(x) -> y { + y := iszero(x) + } + + function funcWithLoop(x) { + for { mstore(0, 0) } userNot(x) {} {} + } + + mstore(0, 1337) + funcWithLoop(42) + sstore(0, mload(0)) +} +// ==== +// step: loadResolver +// ---- +// { +// function userNot(x) -> y +// { y := iszero(x) } +// function funcWithLoop(x_1) +// { +// let _1 := 0 +// mstore(_1, _1) +// for { } userNot(x_1) { } +// { } +// } +// let _3 := 1337 +// let _4 := 0 +// mstore(_4, _3) +// funcWithLoop(42) +// sstore(_4, mload(_4)) +// } From 6453d80c08e956a797167c311a41cbcc36485dea Mon Sep 17 00:00:00 2001 From: chriseth Date: Tue, 17 Dec 2019 13:46:56 +0100 Subject: [PATCH 4/6] Changelog entry. --- Changelog.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Changelog.md b/Changelog.md index 0247137b3c5e..f09fb0355c19 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,3 +1,9 @@ +### 0.5.15 (2019-12-17) + +Bugfixes: + * Yul Optimizer: Fix incorrect redundant load optimization crossing user-defined functions that contain for-loops with memory / storage writes. + + ### 0.5.14 (2019-12-09) Language Features: From 35c24befb4fccd27214005cd17b652c607f38417 Mon Sep 17 00:00:00 2001 From: chriseth Date: Tue, 17 Dec 2019 13:50:09 +0100 Subject: [PATCH 5/6] Increment version number. --- CMakeLists.txt | 2 +- docs/bugs_by_version.json | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 49b2d19c5f70..10b830a6ca5b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -10,7 +10,7 @@ include(EthPolicy) eth_policy() # project name and version should be set after cmake_policy CMP0048 -set(PROJECT_VERSION "0.5.14") +set(PROJECT_VERSION "0.5.15") project(solidity VERSION ${PROJECT_VERSION} LANGUAGES C CXX) include(TestBigEndian) diff --git a/docs/bugs_by_version.json b/docs/bugs_by_version.json index c412be83027f..e2b3c3682afc 100644 --- a/docs/bugs_by_version.json +++ b/docs/bugs_by_version.json @@ -762,6 +762,10 @@ "bugs": [], "released": "2019-12-09" }, + "0.5.15": { + "bugs": [], + "released": "2019-12-17" + }, "0.5.2": { "bugs": [ "SignedArrayStorageCopy", From f913406c65874f895eac295a686ed5d2a21e8e9c Mon Sep 17 00:00:00 2001 From: chriseth Date: Tue, 17 Dec 2019 15:45:09 +0100 Subject: [PATCH 6/6] Bug list entry about yul loop mload bug. --- docs/bugs.json | 13 +++++++++++++ docs/bugs_by_version.json | 4 +++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/docs/bugs.json b/docs/bugs.json index 6c34eb44e6a1..5d94131a19c8 100644 --- a/docs/bugs.json +++ b/docs/bugs.json @@ -1,4 +1,17 @@ [ + { + "name": "ABIEncoderV2LoopYulOptimizer", + "summary": "If both the experimental ABIEncoderV2 and the experimental Yul optimizer are activated, one component of the Yul optimizer may reuse data in memory that has been changed in the meantime.", + "description": "The Yul optimizer incorrectly replaces ``mload`` and ``sload`` calls with values that have been previously written to the load location (and potentially changed in the meantime) if all of the following conditions are met: (1) there is a matching ``mstore`` or ``sstore`` call before; (2) the contents of memory or storage is only changed in a function that is called (directly or indirectly) in between the first store and the load call; (3) called function contains a for loop where the same memory location is changed in the condition or the post or body block. When used in Solidity mode, this can only happen if the experimental ABIEncoderV2 is activated and the experimental Yul optimizer has been activated manually in addition to the regular optimizer in the compiler settings.", + "introduced": "0.5.14", + "fixed": "0.5.15", + "severity": "low", + "conditions": { + "ABIEncoderV2": true, + "optimizer": true, + "yulOptimizer": true + } + }, { "name": "ABIEncoderV2CalldataStructsWithStaticallySizedAndDynamicallyEncodedMembers", "summary": "Reading from calldata structs that contain dynamically encoded, but statically-sized members can result in incorrect values.", diff --git a/docs/bugs_by_version.json b/docs/bugs_by_version.json index e2b3c3682afc..1a0e99749db3 100644 --- a/docs/bugs_by_version.json +++ b/docs/bugs_by_version.json @@ -759,7 +759,9 @@ "released": "2019-11-14" }, "0.5.14": { - "bugs": [], + "bugs": [ + "ABIEncoderV2LoopYulOptimizer" + ], "released": "2019-12-09" }, "0.5.15": {