Remove ambiguity from reaching definitions (#790)

* Fix dead storage bug

Signed-off-by: Lucas Steuernagel <lucas.tnagel@gmail.com>

* Add tests to implementation

Signed-off-by: Lucas Steuernagel <lucas.tnagel@gmail.com>

src/codegen/dead_storage.rs

@@ -8,7 +8,11 @@ use std::fmt;
 #[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)]
 enum Definition {
     Undefined,
-    Instr { block_no: usize, instr_no: usize },
+    Instr {
+        block_no: usize,
+        instr_no: usize,
+        assignment_no: usize,
+    },
 }
 
 impl fmt::Display for Definition {
@@ -17,8 +21,12 @@ impl fmt::Display for Definition {
             Definition::Undefined => {
                 write!(f, "undef")
             }
-            Definition::Instr { block_no, instr_no } => {
-                write!(f, "({}, {})", block_no, instr_no)
+            Definition::Instr {
+                block_no,
+                instr_no,
+                assignment_no,
+            } => {
+                write!(f, "({}, {}, {})", block_no, instr_no, assignment_no)
             }
         }
     }
@@ -171,16 +179,23 @@ fn instr_transfers(block_no: usize, block: &BasicBlock) -> Vec<Vec<Transfer>> {
     let mut transfers = Vec::new();
 
     for (instr_no, instr) in block.instr.iter().enumerate() {
-        let def = Definition::Instr { block_no, instr_no };
-
+        let def = Definition::Instr {
+            block_no,
+            instr_no,
+            assignment_no: 0,
+        };
         let set_var = |var_nos: &[usize]| {
             let mut transfer = Vec::new();
 
-            for var_no in var_nos.iter() {
+            for (assignment_no, var_no) in var_nos.iter().enumerate() {
                 transfer.insert(0, Transfer::Kill { var_no: *var_no });
 
                 transfer.push(Transfer::Gen {
-                    def,
+                    def: Definition::Instr {
+                        block_no,
+                        instr_no,
+                        assignment_no,
+                    },
                     var_no: *var_no,
                 });
             }
@@ -570,7 +585,11 @@ pub fn dead_storage(cfg: &mut ControlFlowGraph, _ns: &mut Namespace) {
                 Instr::SetStorage { .. }
                 | Instr::SetStorageBytes { .. }
                 | Instr::ClearStorage { .. } => {
-                    let def = Definition::Instr { block_no, instr_no };
+                    let def = Definition::Instr {
+                        block_no,
+                        instr_no,
+                        assignment_no: 0,
+                    };
 
                     // add an entry if there is not one there already
                     redundant_stores.entry(def).or_insert(true);
@@ -595,8 +614,18 @@ pub fn dead_storage(cfg: &mut ControlFlowGraph, _ns: &mut Namespace) {
     // remove all stores which are marked as still redundant
     for (def, redundant) in &redundant_stores {
         if *redundant {
-            if let Definition::Instr { block_no, instr_no } = def {
-                cfg.blocks[*block_no].instr[*instr_no] = Instr::Nop;
+            if let Definition::Instr {
+                block_no, instr_no, ..
+            } = def
+            {
+                // Function calls should never be eliminated from the CFG, as they might have side effects
+                // In addition, AbiDecode might fail and halt the execution.
+                if !matches!(
+                    cfg.blocks[*block_no].instr[*instr_no],
+                    Instr::Call { .. } | Instr::AbiDecode { .. }
+                ) {
+                    cfg.blocks[*block_no].instr[*instr_no] = Instr::Nop;
+                }
             }
         }
     }
@@ -606,7 +635,10 @@ fn get_storage_definition<'a>(
     def: &Definition,
     cfg: &'a ControlFlowGraph,
 ) -> Option<(usize, &'a Expression)> {
-    if let Definition::Instr { block_no, instr_no } = def {
+    if let Definition::Instr {
+        block_no, instr_no, ..
+    } = def
+    {
         match &cfg.blocks[*block_no].instr[*instr_no] {
             Instr::LoadStorage { storage, res, .. } => Some((*res, storage)),
             _ => None,
@@ -617,7 +649,10 @@ fn get_storage_definition<'a>(
 }
 
 fn get_definition<'a>(def: &Definition, cfg: &'a ControlFlowGraph) -> Option<&'a Expression> {
-    if let Definition::Instr { block_no, instr_no } = def {
+    if let Definition::Instr {
+        block_no, instr_no, ..
+    } = def
+    {
         match &cfg.blocks[*block_no].instr[*instr_no] {
             Instr::LoadStorage { storage, .. } => Some(storage),
             Instr::Set { expr, .. } => Some(expr),
@@ -630,7 +665,9 @@ fn get_definition<'a>(def: &Definition, cfg: &'a ControlFlowGraph) -> Option<&'a
 
 fn get_vars_at(def: &Definition, block_vars: &BlockVars) -> ReachingDefs {
     match def {
-        Definition::Instr { block_no, instr_no } => {
+        Definition::Instr {
+            block_no, instr_no, ..
+        } => {
             let vars = if let Some(vars) = block_vars.get(block_no) {
                 vars[*instr_no].clone()
             } else {
@@ -746,7 +783,6 @@ fn expression_compare(
                     (Some(left_expr), Some(right_expr)) => {
                         let left_vars = get_vars_at(left.0, block_vars);
                         let right_vars = get_vars_at(right.0, block_vars);
-
                         expression_compare(
                             left_expr,
                             &left_vars,

src/codegen/reaching_definitions.rs

@@ -8,6 +8,7 @@ use std::fmt;
 pub struct Def {
     pub block_no: usize,
     pub instr_no: usize,
+    pub assignment_no: usize,
 }
 
 #[derive(Clone, Copy, PartialEq)]
@@ -96,16 +97,18 @@ fn instr_transfers(block_no: usize, block: &BasicBlock) -> Vec<Vec<Transfer>> {
     let mut transfers = Vec::new();
 
     for (instr_no, instr) in block.instr.iter().enumerate() {
-        let def = Def { block_no, instr_no };
-
         let set_var = |var_nos: &[usize]| {
             let mut transfer = Vec::new();
 
-            for var_no in var_nos.iter() {
+            for (assignment_no, var_no) in var_nos.iter().enumerate() {
                 transfer.insert(0, Transfer::Kill { var_no: *var_no });
 
                 transfer.push(Transfer::Gen {
-                    def,
+                    def: Def {
+                        block_no,
+                        instr_no,
+                        assignment_no,
+                    },
                     var_no: *var_no,
                 });
             }

src/codegen/vector_to_slice.rs

@@ -164,13 +164,16 @@ fn update_vectors_to_slice(
 
     for block_no in 0..cfg.blocks.len() {
         for instr_no in 0..cfg.blocks[block_no].instr.len() {
-            let cur = Def { block_no, instr_no };
-
             if let Instr::Set {
                 expr: Expression::AllocDynamicArray(..),
                 ..
             } = &cfg.blocks[block_no].instr[instr_no]
             {
+                let cur = Def {
+                    block_no,
+                    instr_no,
+                    assignment_no: 0,
+                };
                 if !writable.contains(&cur) {
                     defs_to_be_updated.insert(cur);
                 }

tests/codegen_testcases/dead_storage.sol

@@ -150,3 +150,22 @@ contract deadstorage {
 // CHECK: load storage slot((uint256 19
 // NOT-CHECK: load storage slot((uint256 19
 }
+
+contract foo {
+    struct S { int32 f1; }
+        S[] arr;
+
+    function g() private returns (S storage, S storage) {
+        return (arr[0], arr[1]);
+    }
+// BEGIN-CHECK: foo::foo::function::f
+    function f() public returns (S, S) {
+        S[] storage ptrArr = arr;
+        ptrArr.push(S({f1: 1}));
+        ptrArr.push(S({f1: 2}));
+// CHECK: %.temp.76, %.temp.77 = call foo::foo::function::g
+// CHECK: %temp.78 = load storage slot(%.temp.76) ty:struct foo.S
+// CHECK: %temp.79 = load storage slot(%.temp.77) ty:struct foo.S
+        return g();
+    }
+}

tests/substrate_tests/mod.rs

@@ -19,6 +19,7 @@ mod loops;
 mod mappings;
 mod modifier;
 mod primitives;
+mod storage;
 mod strings;
 mod structs;
 mod value;

tests/substrate_tests/storage.rs

@@ -0,0 +1,37 @@
+use crate::build_solidity;
+use parity_scale_codec::{Decode, Encode};
+
+#[test]
+fn storage_load_on_return() {
+    #[derive(Debug, PartialEq, Encode, Decode)]
+    struct SStruct {
+        f1: i32,
+    }
+
+    let mut runtime = build_solidity(
+        r##"
+contract foo {
+    struct S { int32 f1; }
+        S[] arr;
+
+    function g() private returns (S storage, S storage) {
+        return (arr[0], arr[1]);
+    }
+
+    function f() public returns (S, S) {
+        S[] storage ptrArr = arr;
+        ptrArr.push(S({f1: 1}));
+        ptrArr.push(S({f1: 2}));
+        return g();
+    }
+}
+        "##,
+    );
+
+    runtime.function("f", Vec::new());
+
+    assert_eq!(
+        runtime.vm.output,
+        [SStruct { f1: 1 }, SStruct { f1: 2 }].encode(),
+    );
+}