diff --git a/CHANGELOG.md b/CHANGELOG.md index ce9a98b49..eb15c75be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ Supported `polkadot-sdk` rev: `2604.2.0` ### Fixed - Restored the top-level `revive_version` field in `--standard-json` output and the `resolc_version` field in `--combined-json` output, both of which stopped being populated in `v0.4.0`. [#557](https://github.com/paritytech/revive/pull/557) +- ICE triggered by `--disable-solc-optimizer --newyork where inlined loops could emit an unterminated basic block in the inlined entrypoint. [#561](https://github.com/paritytech/revive/pull/561) ## v1.3.0 diff --git a/crates/integration/contracts/FoldedGuardZext.yul b/crates/integration/contracts/FoldedGuardZext.yul new file mode 100644 index 000000000..cde2ccbab --- /dev/null +++ b/crates/integration/contracts/FoldedGuardZext.yul @@ -0,0 +1,173 @@ +/// ICE PoC (paritytech/revive#560): the one-armed `if` lowering in `to_llvm` +/// built the conditional branch first and materialized the fall-through +/// `inputs` afterwards, with the builder still positioned at the end of the +/// now-terminated entry block. That is invisible while every input is already +/// word-typed (no instruction is emitted), but a narrow input — here the raw +/// `gt` comparison result flowing out of the inlined `fun_h0` — needs a `zext` +/// to reach the join phi, and that `zext` landed after the terminator: +/// "Basic Block in function 'fun_run_runtime' does not have terminator!". +/// +/// The shape needs all of: +/// - `fun_h0`'s `if 0x01 { ... leave }`: the constant-true guard folds, so +/// the `leave`-carried return value copy-propagates to the bare `gt` +/// (an i1 value that is neither a constant nor a word-typed if-output); +/// - `fun_h1(acc, acc)`: equal arguments fold `iszero(lt(a, b))` to +/// constant true, collapsing the leave-`if` so the `leave`-elimination +/// guard around the remainder keeps the raw `gt` as its fall-through +/// input with a constant-false condition; +/// - the rotated `for` loop (condition `1`, `break` in the body — solc's +/// unoptimized lowering) before that guard, which keeps the simplifier +/// from folding the constant-condition `if` away; +/// - `fun_run` staying above the never-inline size threshold (the verbose +/// copy chains are load-bearing): fully inlined into the entry block, +/// the guard folds and the shape disappears. +/// +/// This is solc's `--no-optimize-yul` output for issue #560's contract with +/// the selector dispatch and revert-helper indirections trimmed. `run(s)` +/// never assigns its unnamed return value, so every call must return 0. +object "FoldedGuardZext" { + code { + let size := datasize("FoldedGuardZext_deployed") + codecopy(0, dataoffset("FoldedGuardZext_deployed"), size) + return(0, size) + } + object "FoldedGuardZext_deployed" { + code { + { + mstore(64, memoryguard(0x80)) + external_fun_run() + revert(0, 0) + } + function allocate_unbounded() -> memPtr + { memPtr := mload(64) } + function cleanup_uint256(value) -> cleaned + { cleaned := value } + function validator_revert_uint256(value) + { + if iszero(eq(value, cleanup_uint256(value))) { revert(0, 0) } + } + function abi_decode_uint256(offset, end) -> value + { + value := calldataload(offset) + validator_revert_uint256(value) + } + function abi_decode_tuple_uint256(headStart, dataEnd) -> value0 + { + if slt(sub(dataEnd, headStart), 32) + { + revert(0, 0) + } + let offset := 0 + value0 := abi_decode_uint256(add(headStart, offset), dataEnd) + } + function abi_encode_uint256_to_uint256(value, pos) + { + mstore(pos, cleanup_uint256(value)) + } + function abi_encode_uint256(headStart, value0) -> tail + { + tail := add(headStart, 32) + abi_encode_uint256_to_uint256(value0, add(headStart, 0)) + } + function external_fun_run() + { + if callvalue() + { + revert(0, 0) + } + let param := abi_decode_tuple_uint256(4, calldatasize()) + let ret := fun_run(param) + let memPos := allocate_unbounded() + let memEnd := abi_encode_uint256(memPos, ret) + return(memPos, sub(memEnd, memPos)) + } + function zero_value_for_split_uint256() -> ret + { ret := 0 } + function fun_run(var_s) -> var + { + let zero_uint256 := zero_value_for_split_uint256() + var := zero_uint256 + let _1 := var_s + let expr := _1 + let var_acc := expr + let _2 := var_acc + let expr_1 := _2 + let _3 := var_acc + let expr_2 := _3 + let expr_3 := fun_h0(expr_1, expr_2) + var_acc := expr_3 + let _4 := var_acc + let expr_4 := _4 + let _5 := var_acc + let expr_5 := _5 + let expr_6 := fun_h1(expr_4, expr_5) + var_acc := expr_6 + } + function fun_h0(var_a, var_b) -> var_r + { + let zero_t_uint256 := zero_value_for_split_uint256() + var_r := zero_t_uint256 + let expr := 0x01 + if expr + { + var_r := gt(var_r, var_b) + let _1 := var_r + let expr_1 := _1 + var_r := expr_1 + leave + } + } + function cleanup_t_rational_by(value) -> cleaned + { cleaned := value } + function identity(value) -> ret + { ret := value } + function convert_t_rational_by_to_t_uint256(value) -> converted + { + converted := cleanup_uint256(identity(cleanup_t_rational_by(value))) + } + function increment_wrapping_uint256(value) -> ret + { + ret := cleanup_uint256(add(value, 1)) + } + function cleanup_rational_by(value) -> cleaned + { cleaned := value } + function convert_rational_by_to_uint256(value) -> converted + { + converted := cleanup_uint256(identity(cleanup_rational_by(value))) + } + function fun_h1(var_a, var_b) -> var_r + { + let zero_uint256 := zero_value_for_split_uint256() + var_r := zero_uint256 + let expr := 0x00 + let var_i := convert_t_rational_by_to_t_uint256(expr) + for { } + 1 + { + let _1 := var_i + let _2 := increment_wrapping_uint256(_1) + var_i := _2 + } + { + let _3 := var_i + let expr_1 := _3 + let expr_2 := 0x01 + let expr_3 := lt(cleanup_uint256(expr_1), convert_rational_by_to_uint256(expr_2)) + if iszero(expr_3) { break } + } + let _4 := var_a + let expr_4 := _4 + let _5 := var_b + let expr_5 := _5 + let expr_6 := iszero(lt(cleanup_uint256(expr_4), cleanup_uint256(expr_5))) + if expr_6 + { + let _6 := var_a + let expr_7 := _6 + var_r := expr_7 + leave + } + } + } + } +} diff --git a/crates/integration/src/tests.rs b/crates/integration/src/tests.rs index 6040f52cf..000c9056b 100644 --- a/crates/integration/src/tests.rs +++ b/crates/integration/src/tests.rs @@ -2844,6 +2844,30 @@ fn switch_callvalue_cse_dangling() { run_differential(actions); } +/// The one-armed `if` lowering must materialize its fall-through `inputs` +/// before the conditional branch terminates the entry block: a narrow (i1) input needs a `zext` to +/// reach the join phi, and emitting it after the branch leaves the block without a trailing +/// terminator ("Basic Block ... does not have terminator! label %for_join"). Compiling at all +/// exercises the fix; `run(s)` never assigns its return value, so every call returns 0. See +/// FoldedGuardZext.yul (paritytech/revive#560). +#[test] +fn folded_guard_narrow_if_input_zext() { + let mut actions = instantiate_yul("contracts/FoldedGuardZext.yul", "FoldedGuardZext"); + for s in [U256::ZERO, U256::from(7u64), U256::MAX] { + let mut data = vec![0xa4, 0x44, 0xf5, 0xe9]; + data.extend_from_slice(&s.to_be_bytes::<32>()); + actions.push(Call { + origin: TestAddress::Alice, + dest: TestAddress::Instantiated(0), + value: 0, + gas_limit: None, + storage_deposit_limit: None, + data, + }); + } + run_differential(actions); +} + /// The panic-pattern outliner must not collapse a window that contains a /// side-effecting call. `inner()` reverts with 0xdeadbeef before the panic revert; dropping it and /// emitting PanicRevert would revert with the wrong payload. EVM reverts with 0xdeadbeef; the bug diff --git a/crates/newyork/src/to_llvm.rs b/crates/newyork/src/to_llvm.rs index ad7d8a246..8c948725f 100644 --- a/crates/newyork/src/to_llvm.rs +++ b/crates/newyork/src/to_llvm.rs @@ -4714,8 +4714,6 @@ impl<'ctx> LlvmCodegen<'ctx> { } } else { let entry_block = context.basic_block(); - context.build_conditional_branch(condition_bool, then_block, join_block)?; - let mut else_yields = Vec::new(); for (index, input_value) in inputs.iter().enumerate() { else_yields.push(self.translate_value_as_word( @@ -4724,6 +4722,7 @@ impl<'ctx> LlvmCodegen<'ctx> { &format!("input_yield_{}", index), )?); } + context.build_conditional_branch(condition_bool, then_block, join_block)?; phi_incoming.push((else_yields, entry_block)); context.set_basic_block(then_block); diff --git a/crates/resolc/src/cli_utils.rs b/crates/resolc/src/cli_utils.rs index aa6b8c30d..e2acb586d 100644 --- a/crates/resolc/src/cli_utils.rs +++ b/crates/resolc/src/cli_utils.rs @@ -15,6 +15,10 @@ pub const SOLIDITY_DEPENDENCY_CONTRACT_PATH: &str = "src/tests/data/solidity/dep /// The simple Solidity contract containing i256 divisions and remains /// that should be compiled correctly. pub const SOLIDITY_LARGE_DIV_REM_CONTRACT_PATH: &str = "src/tests/data/solidity/large_div_rem.sol"; +/// The verbatim reproducer from paritytech/revive#560: a folded guard plus an +/// inlined for-loop that crashed the newyork pipeline under `--disable-solc-optimizer`. +pub const SOLIDITY_FOLDED_GUARD_INLINED_LOOP_PATH: &str = + "src/tests/data/solidity/folded_guard_inlined_loop.sol"; /// The simple YUL contract test fixture path. pub const YUL_CONTRACT_PATH: &str = "src/tests/data/yul/contract.yul"; diff --git a/crates/resolc/src/tests/cli/bin.rs b/crates/resolc/src/tests/cli/bin.rs index 19fad4dc1..5b163e0bd 100644 --- a/crates/resolc/src/tests/cli/bin.rs +++ b/crates/resolc/src/tests/cli/bin.rs @@ -2,8 +2,9 @@ use crate::{ cli_utils::{ - absolute_path, assert_command_success, execute_command, CommandResult, ResolcOptSettings, - SolcOptSettings, SOLIDITY_CONTRACT_PATH, STANDARD_JSON_CONTRACTS_PATH, + absolute_path, assert_command_success, execute_command, execute_resolc, CommandResult, + ResolcOptSettings, SolcOptSettings, SOLIDITY_CONTRACT_PATH, + SOLIDITY_FOLDED_GUARD_INLINED_LOOP_PATH, STANDARD_JSON_CONTRACTS_PATH, YUL_MEMSET_CONTRACT_PATH, }, SolcCompiler, @@ -165,6 +166,20 @@ fn compiles_yul_to_binary_blob() { assert_evm_blob_from_yul(&solc_result); } +/// Compile regression for [paritytech/revive#560](https://github.com/paritytech/revive/issues/560): +/// the verbatim Solidity from the issue crashed the newyork pipeline under +/// `--disable-solc-optimizer` with "Basic Block in function 'fun_run_runtime' does +/// not have terminator! label %for_join" (the one-armed `if` lowering emitted the +/// fall-through input `zext` after the branch terminator). +#[test] +fn compiles_newyork_folded_guard_inlined_loop() { + let path = absolute_path(SOLIDITY_FOLDED_GUARD_INLINED_LOOP_PATH); + let arguments = &[&path, "--newyork", "--disable-solc-optimizer", "--bin"]; + + let result = execute_resolc(arguments); + assert_pvm_blob(&result); +} + /// This test mimics the command used in the `resolc` benchmarks when compiling Solidity via standard JSON input. #[test] fn compiles_json_to_binary_blob() { diff --git a/crates/resolc/src/tests/data/solidity/folded_guard_inlined_loop.sol b/crates/resolc/src/tests/data/solidity/folded_guard_inlined_loop.sol new file mode 100644 index 000000000..da6f2d2b0 --- /dev/null +++ b/crates/resolc/src/tests/data/solidity/folded_guard_inlined_loop.sol @@ -0,0 +1,15 @@ +pragma solidity ^0.8.0; +contract C { + function h0(uint256 a, uint256 b) internal returns (uint256 r) { + if (true) { assembly { r := gt(r, b) } return r; } + } + function h1(uint256 a, uint256 b) internal returns (uint256 r) { + for (uint256 i = 0; i < 1; i++) {} + if (a >= b) { return a; } + } + function run(uint256 s) external returns (uint256) { + uint256 acc = s; + acc = h0(acc, acc); + acc = h1(acc, acc); + } +}