diff --git a/CHANGELOG.md b/CHANGELOG.md index b6bc4a5..3eed962 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +## Unreleased + +Supported `polkadot-sdk` rev: `2604.2.0` + +### Fixed +- `--newyork`: A bug in dead call value analysis. [#589](https://github.com/paritytech/revive/pull/589) + ## v1.4.0 Supported `polkadot-sdk` rev: `2604.2.0` diff --git a/crates/integration/contracts/FunctionCvReturnOnly.yul b/crates/integration/contracts/FunctionCvReturnOnly.yul new file mode 100644 index 0000000..d8ff247 --- /dev/null +++ b/crates/integration/contracts/FunctionCvReturnOnly.yul @@ -0,0 +1,31 @@ +object "FunctionCvReturnOnly" { + code { let s := datasize("FunctionCvReturnOnly_deployed") codecopy(0, dataoffset("FunctionCvReturnOnly_deployed"), s) return(0, s) } + object "FunctionCvReturnOnly_deployed" { + code { + // `tally` hands callvalue() back through its return variable on the fall-through path, + // so the binding's only use is recorded in `Function::return_values` rather than in any + // statement. Three callvalue() sites put the module over the outlined-callvalue + // threshold, so codegen runs the dead-callvalue analysis; it must count return + // variables as uses, otherwise the binding is skipped and the function hands back the + // zero its return slot was seeded with. Two call sites and a body above the + // always-inline size keep `tally` a real function instead of being inlined away. + function tally(seed) -> r { + sstore(10, seed) + sstore(11, add(seed, 1)) + sstore(12, add(seed, 2)) + sstore(13, add(seed, 3)) + sstore(14, add(seed, 4)) + sstore(15, add(seed, 5)) + sstore(16, add(seed, 6)) + sstore(17, add(seed, 7)) + r := callvalue() + } + let sel := calldataload(0) + switch sel + case 1 { sstore(0, tally(1)) } + case 2 { sstore(0, tally(2)) } + case 3 { sstore(0, add(callvalue(), calldatasize())) } + default { sstore(0, add(callvalue(), 0x20)) } + } + } +} diff --git a/crates/integration/contracts/SwitchCvYieldOnly.yul b/crates/integration/contracts/SwitchCvYieldOnly.yul new file mode 100644 index 0000000..d55f0cd --- /dev/null +++ b/crates/integration/contracts/SwitchCvYieldOnly.yul @@ -0,0 +1,20 @@ +object "SwitchCvYieldOnly" { + code { let s := datasize("SwitchCvYieldOnly_deployed") codecopy(0, dataoffset("SwitchCvYieldOnly_deployed"), s) return(0, s) } + object "SwitchCvYieldOnly_deployed" { + code { + let sel := calldataload(0) + // Three callvalue() sites put the module over the outlined-callvalue threshold, so + // codegen runs the dead-callvalue analysis. Case 1 binds callvalue() and its only use + // is the region yield carrying the switch result into `r`. The other branches evaluate + // their second operand first, so no branch-leading callvalue hoist merges the bindings + // and the yield-only binding survives to codegen. The analysis must count region yields + // as uses, otherwise the binding is skipped and the yield references an undefined value. + let r := 0 + switch sel + case 1 { r := callvalue() } + case 2 { r := add(callvalue(), calldatasize()) } + default { r := add(callvalue(), 0x20) } + sstore(0, r) + } + } +} diff --git a/crates/integration/src/tests.rs b/crates/integration/src/tests.rs index ef5b337..2aaff9a 100644 --- a/crates/integration/src/tests.rs +++ b/crates/integration/src/tests.rs @@ -2867,6 +2867,52 @@ fn switch_callvalue_cse_dangling() { run_differential(actions); } +/// A switch case whose region yields a bare `let cv = callvalue()` binding +/// (the yield being its only use) must keep the binding at codegen. With three or more +/// callvalue sites the outlined-callvalue path is enabled and its dead-callvalue analysis +/// skips bindings whose every use is an If condition. Region yields were not counted as +/// uses, so the binding was skipped and the yield referenced an undefined value, failing +/// compilation. Compiling and running at all exercises the fix. See SwitchCvYieldOnly.yul +/// (paritytech/revive#588). +#[test] +fn switch_callvalue_yield_only() { + let mut actions = instantiate_yul("contracts/SwitchCvYieldOnly.yul", "SwitchCvYieldOnly"); + for (sel, value) in [(1u64, 0u128), (1, 5), (2, 3), (99, 7)] { + actions.push(Call { + origin: TestAddress::Alice, + dest: TestAddress::Instantiated(0), + value, + gas_limit: None, + storage_deposit_limit: None, + data: U256::from(sel).to_be_bytes::<32>().to_vec(), + }); + } + run_differential(actions); +} + +/// A function returning a bare `let cv = callvalue()` binding through its return variable +/// must keep the binding at codegen. The dead-callvalue analysis counted uses over the +/// statement tree only, but a return variable is recorded in `Function::return_values`, +/// a field outside that tree, so a binding reaching the return slot on the fall-through +/// path looked unused and was skipped. Codegen seeds return values with zero before the +/// body runs, so the function silently returned zero instead of the transferred value. +/// See FunctionCvReturnOnly.yul (paritytech/revive#589). +#[test] +fn function_callvalue_return_only() { + let mut actions = instantiate_yul("contracts/FunctionCvReturnOnly.yul", "FunctionCvReturnOnly"); + for (sel, value) in [(1u64, 7u128), (2, 11), (3, 13), (99, 17)] { + actions.push(Call { + origin: TestAddress::Alice, + dest: TestAddress::Instantiated(0), + value, + gas_limit: None, + storage_deposit_limit: None, + data: U256::from(sel).to_be_bytes::<32>().to_vec(), + }); + } + 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 diff --git a/crates/newyork/src/to_llvm.rs b/crates/newyork/src/to_llvm.rs index 8c94872..6c895ae 100644 --- a/crates/newyork/src/to_llvm.rs +++ b/crates/newyork/src/to_llvm.rs @@ -15,8 +15,9 @@ use revive_llvm_context::{ use crate::heap_opt::HeapOptResults; use crate::ir::{ - BinaryOperation, BitWidth, Block, CallKind, CreateKind, Expression, Function, FunctionId, - MemoryRegion, Object, Region, Statement, SwitchCase, Type, UnaryOperation, Value, ValueId, + for_each_statement, BinaryOperation, BitWidth, Block, CallKind, CreateKind, Expression, + Function, FunctionId, MemoryRegion, Object, Region, Statement, SwitchCase, Type, + UnaryOperation, Value, ValueId, }; use crate::type_inference::TypeInference; @@ -3177,284 +3178,85 @@ impl<'ctx> LlvmCodegen<'ctx> { Ok(()) } - /// Find callvalue ValueIds that are ONLY used as conditions in - /// `if callvalue() { revert(0,0) }` or If condition patterns. - /// These can be skipped during codegen because __revive_callvalue_check() - /// and __revive_callvalue_nonzero() handle reading callvalue internally. + /// Find callvalue [`ValueId`]s whose every use is a [`Statement::If`] condition. + /// Their `let vN = callvalue()` bindings can be skipped during codegen + /// because `__revive_callvalue_check()` and `__revive_callvalue_nonzero()` + /// read callvalue internally when emitting those conditions. + /// + /// Uses are counted with [`Statement::for_each_value_id`] — the canonical + /// walker, which visits every operand position including region yields — + /// and compared against the count of [`Statement::If`]-condition uses gathered over the + /// same statements. The two walks cover the identical statement set, so a + /// binding is skippable exactly when the counts match. Any other use — a + /// region yield, a call argument, a store operand — reads the materialized + /// value, so the binding must stay (see paritytech/revive#588, where a + /// switch case yielding a bare callvalue binding was skipped and codegen + /// hit an undefined value). + /// + /// [`Function::return_values`] is counted on top of the statement walk: a return + /// variable is recorded on the function itself rather than in its body, yet + /// [`Self::generate_function`] reads it back when writing the return slot. A binding + /// reaching the slot on the fall-through path has no statement use at all, so without + /// this the counts match trivially and the binding is skipped, leaving the function to + /// hand back the zero its return slot was seeded with. Explicit `leave`s need no such + /// handling because [`Statement::Leave`] carries its values as operands. fn find_dead_callvalue_ids(object: &Object) -> BTreeSet { let mut callvalue_ids = BTreeSet::new(); - let mut used_ids = BTreeSet::new(); - Self::find_callvalue_bindings(&object.code.statements, &mut callvalue_ids); for function in object.functions.values() { Self::find_callvalue_bindings(&function.body.statements, &mut callvalue_ids); } - Self::find_value_uses(&object.code.statements, &callvalue_ids, &mut used_ids); + let mut use_counts: BTreeMap = BTreeMap::new(); + let mut condition_counts: BTreeMap = BTreeMap::new(); + let mut count_uses = |statements: &[Statement]| { + for statement in statements { + statement.for_each_value_id(&mut |id| { + if callvalue_ids.contains(&id.0) { + *use_counts.entry(id.0).or_default() += 1; + } + }); + } + for_each_statement(statements, &mut |statement| { + if let Statement::If { condition, .. } = statement { + if callvalue_ids.contains(&condition.id.0) { + *condition_counts.entry(condition.id.0).or_default() += 1; + } + } + }); + }; + count_uses(&object.code.statements); for function in object.functions.values() { - Self::find_value_uses(&function.body.statements, &callvalue_ids, &mut used_ids); + count_uses(&function.body.statements); } - callvalue_ids.difference(&used_ids).copied().collect() + for function in object.functions.values() { + for return_value_id in &function.return_values { + if callvalue_ids.contains(&return_value_id.0) { + *use_counts.entry(return_value_id.0).or_default() += 1; + } + } + } + + callvalue_ids + .iter() + .filter(|id| { + use_counts.get(id).copied().unwrap_or(0) + == condition_counts.get(id).copied().unwrap_or(0) + }) + .copied() + .collect() } + /// Collects the ValueIds bound by `let vN = callvalue()` statements. fn find_callvalue_bindings(statements: &[Statement], ids: &mut BTreeSet) { - for statement in statements { + for_each_statement(statements, &mut |statement| { if let Statement::Let { bindings, value } = statement { if bindings.len() == 1 && matches!(value, Expression::CallValue) { ids.insert(bindings[0].0); } } - Self::for_each_nested_region(statement, |region_statements| { - Self::find_callvalue_bindings(region_statements, ids); - }); - } - } - - /// Find uses of callvalue IDs in non-condition positions. - /// If conditions are OK (handled by callvalue_nonzero); everything else is "used". - fn find_value_uses( - statements: &[Statement], - callvalue_ids: &BTreeSet, - used: &mut BTreeSet, - ) { - for statement in statements { - match statement { - Statement::Let { value, .. } | Statement::Expression(value) => { - Self::collect_expr_value_refs(value, callvalue_ids, used); - } - Statement::MStore { offset, value, .. } - | Statement::MStore8 { offset, value, .. } => { - Self::mark_if_callvalue(offset.id.0, callvalue_ids, used); - Self::mark_if_callvalue(value.id.0, callvalue_ids, used); - } - Statement::SStore { key, value, .. } | Statement::TStore { key, value } => { - Self::mark_if_callvalue(key.id.0, callvalue_ids, used); - Self::mark_if_callvalue(value.id.0, callvalue_ids, used); - } - Statement::If { inputs, .. } => { - for operand in inputs { - Self::mark_if_callvalue(operand.id.0, callvalue_ids, used); - } - } - Statement::Switch { - scrutinee, inputs, .. - } => { - Self::mark_if_callvalue(scrutinee.id.0, callvalue_ids, used); - for operand in inputs { - Self::mark_if_callvalue(operand.id.0, callvalue_ids, used); - } - } - Statement::Revert { offset, length } | Statement::Return { offset, length } => { - Self::mark_if_callvalue(offset.id.0, callvalue_ids, used); - Self::mark_if_callvalue(length.id.0, callvalue_ids, used); - } - Statement::Log { - offset, - length, - topics, - } => { - Self::mark_if_callvalue(offset.id.0, callvalue_ids, used); - Self::mark_if_callvalue(length.id.0, callvalue_ids, used); - for topic in topics { - Self::mark_if_callvalue(topic.id.0, callvalue_ids, used); - } - } - Statement::ExternalCall { - gas, - address, - value, - args_offset, - args_length, - ret_offset, - ret_length, - .. - } => { - Self::mark_if_callvalue(gas.id.0, callvalue_ids, used); - Self::mark_if_callvalue(address.id.0, callvalue_ids, used); - if let Some(operand) = value { - Self::mark_if_callvalue(operand.id.0, callvalue_ids, used); - } - Self::mark_if_callvalue(args_offset.id.0, callvalue_ids, used); - Self::mark_if_callvalue(args_length.id.0, callvalue_ids, used); - Self::mark_if_callvalue(ret_offset.id.0, callvalue_ids, used); - Self::mark_if_callvalue(ret_length.id.0, callvalue_ids, used); - } - Statement::Create { - value, - offset, - length, - salt, - .. - } => { - Self::mark_if_callvalue(value.id.0, callvalue_ids, used); - Self::mark_if_callvalue(offset.id.0, callvalue_ids, used); - Self::mark_if_callvalue(length.id.0, callvalue_ids, used); - if let Some(salt_value) = salt { - Self::mark_if_callvalue(salt_value.id.0, callvalue_ids, used); - } - } - Statement::CustomErrorRevert { arguments, .. } => { - for argument in arguments { - Self::mark_if_callvalue(argument.id.0, callvalue_ids, used); - } - } - Statement::Leave { return_values } => { - for operand in return_values { - Self::mark_if_callvalue(operand.id.0, callvalue_ids, used); - } - } - Statement::Break { values } | Statement::Continue { values } => { - for operand in values { - Self::mark_if_callvalue(operand.id.0, callvalue_ids, used); - } - } - Statement::For { - initial_values, - condition, - .. - } => { - for operand in initial_values { - Self::mark_if_callvalue(operand.id.0, callvalue_ids, used); - } - Self::collect_expr_value_refs(condition, callvalue_ids, used); - } - Statement::MCopy { - destination, - source, - length, - } => { - Self::mark_if_callvalue(destination.id.0, callvalue_ids, used); - Self::mark_if_callvalue(source.id.0, callvalue_ids, used); - Self::mark_if_callvalue(length.id.0, callvalue_ids, used); - } - Statement::SelfDestruct { address } => { - Self::mark_if_callvalue(address.id.0, callvalue_ids, used); - } - Statement::CodeCopy { - destination, - offset, - length, - } => { - Self::mark_if_callvalue(destination.id.0, callvalue_ids, used); - Self::mark_if_callvalue(offset.id.0, callvalue_ids, used); - Self::mark_if_callvalue(length.id.0, callvalue_ids, used); - } - Statement::ExtCodeCopy { - address, - destination, - offset, - length, - } => { - Self::mark_if_callvalue(address.id.0, callvalue_ids, used); - Self::mark_if_callvalue(destination.id.0, callvalue_ids, used); - Self::mark_if_callvalue(offset.id.0, callvalue_ids, used); - Self::mark_if_callvalue(length.id.0, callvalue_ids, used); - } - Statement::MappingSStore { key, slot, value } => { - Self::mark_if_callvalue(key.id.0, callvalue_ids, used); - Self::mark_if_callvalue(slot.id.0, callvalue_ids, used); - Self::mark_if_callvalue(value.id.0, callvalue_ids, used); - } - _ => {} - } - Self::for_each_nested_region(statement, |region_statements| { - Self::find_value_uses(region_statements, callvalue_ids, used); - }); - } - } - - fn mark_if_callvalue(id: u32, callvalue_ids: &BTreeSet, used: &mut BTreeSet) { - if callvalue_ids.contains(&id) { - used.insert(id); - } - } - - fn collect_expr_value_refs( - expression: &Expression, - callvalue_ids: &BTreeSet, - used: &mut BTreeSet, - ) { - match expression { - Expression::Var(variable_id) => { - Self::mark_if_callvalue(variable_id.0, callvalue_ids, used) - } - Expression::Binary { lhs, rhs, .. } => { - Self::mark_if_callvalue(lhs.id.0, callvalue_ids, used); - Self::mark_if_callvalue(rhs.id.0, callvalue_ids, used); - } - Expression::Unary { operand, .. } - | Expression::Truncate { value: operand, .. } - | Expression::ZeroExtend { value: operand, .. } - | Expression::SignExtendTo { value: operand, .. } => { - Self::mark_if_callvalue(operand.id.0, callvalue_ids, used); - } - Expression::Call { arguments, .. } => { - for argument in arguments { - Self::mark_if_callvalue(argument.id.0, callvalue_ids, used); - } - } - Expression::Keccak256 { offset, length } - | Expression::Keccak256Pair { - word0: offset, - word1: length, - } - | Expression::MappingSLoad { - key: offset, - slot: length, - } => { - Self::mark_if_callvalue(offset.id.0, callvalue_ids, used); - Self::mark_if_callvalue(length.id.0, callvalue_ids, used); - } - Expression::Keccak256Single { word0 } => { - Self::mark_if_callvalue(word0.id.0, callvalue_ids, used); - } - Expression::CallDataLoad { offset } => { - Self::mark_if_callvalue(offset.id.0, callvalue_ids, used); - } - Expression::MLoad { offset, .. } => { - Self::mark_if_callvalue(offset.id.0, callvalue_ids, used); - } - _ => {} - } - } - - /// Call a closure for each nested region's statements in a statement. - fn for_each_nested_region(statement: &Statement, mut callback: F) { - match statement { - Statement::If { - then_region, - else_region, - .. - } => { - callback(&then_region.statements); - if let Some(region) = else_region { - callback(®ion.statements); - } - } - Statement::Switch { cases, default, .. } => { - for case in cases { - callback(&case.body.statements); - } - if let Some(default_region) = default { - callback(&default_region.statements); - } - } - Statement::For { - condition_statements, - body, - post, - .. - } => { - callback(condition_statements); - callback(&body.statements); - callback(&post.statements); - } - Statement::Block(region) => { - callback(®ion.statements); - } - _ => {} - } + }); } /// Counts MappingSLoad and MappingSStore operations separately in an object. @@ -3547,7 +3349,6 @@ impl<'ctx> LlvmCodegen<'ctx> { /// bytes in body overhead, so we only emit it when there are enough /// call sites to amortise that cost (per-site savings are ~3 inst). fn count_constant_mstore_patterns(object: &Object) -> (usize, usize) { - use crate::ir::for_each_statement; use num::Zero; let mut literals: BTreeMap = BTreeMap::new();