Do not memcpy optimize when passed as fn arg (#5253)

This fixes a bug in `local_copy_prop_prememcpy` which didn't make an exception when a local gets passed its address into another function, thus making it unsafe to optimize it. Closes #5250 Related to #4600
FuelLabs · Nov 2, 2023 · ae31213 · ae31213
1 parent d1bc49d
commit ae31213
Show file tree

Hide file tree

Showing 7 changed files with 66 additions and 13 deletions.
diff --git a/sway-ir/src/optimize/memcpyopt.rs b/sway-ir/src/optimize/memcpyopt.rs
@@ -44,7 +44,7 @@ fn local_copy_prop_prememcpy(context: &mut Context, function: Function) -> Resul
     let mut loads_map = FxHashMap::<Symbol, Vec<Value>>::default();
     let mut stores_map = FxHashMap::<Symbol, Vec<Value>>::default();
     let mut instr_info_map = FxHashMap::<Value, InstInfo>::default();
-    let mut asm_uses = FxHashSet::<Symbol>::default();
+    let mut escaping_uses = FxHashSet::<Symbol>::default();
 
     for (pos, (block, inst)) in function.instruction_iter(context).enumerate() {
         let info = || InstInfo { block, pos };
@@ -81,11 +81,21 @@ fn local_copy_prop_prememcpy(context: &mut Context, function: Function) -> Resul
                 for arg in args {
                     if let Some(arg) = arg.initializer {
                         if let Some(local) = get_symbol(context, arg) {
-                            asm_uses.insert(local);
+                            escaping_uses.insert(local);
                         }
                     }
                 }
             }
+            Instruction {
+                op: InstOp::Call(_, args),
+                ..
+            } => {
+                for arg in args {
+                    if let Some(local) = get_symbol(context, *arg) {
+                        escaping_uses.insert(local);
+                    }
+                }
+            }
             _ => (),
         }
     }
@@ -151,7 +161,7 @@ fn local_copy_prop_prememcpy(context: &mut Context, function: Function) -> Resul
                             instr_info.block == block && instr_info.pos > pos
                         })
                         // We don't deal with ASM blocks.
-                        || asm_uses.contains(&dst_local)
+                        || escaping_uses.contains(&dst_local)
                         // We don't deal part copies.
                         || dst_local.get_type(context) != src_local.get_type(context)
                         // We don't replace the destination when it's an arg.

diff --git a/.../e2e_vm_tests/test_programs/should_pass/language/configurable_consts/json_abi_oracle.json b/.../e2e_vm_tests/test_programs/should_pass/language/configurable_consts/json_abi_oracle.json
@@ -7,7 +7,7 @@
         "typeArguments": null
       },
       "name": "C0",
-      "offset": 3420
+      "offset": 3436
     },
     {
       "configurableType": {
@@ -16,7 +16,7 @@
         "typeArguments": null
       },
       "name": "C1",
-      "offset": 3428
+      "offset": 3444
     },
     {
       "configurableType": {
@@ -25,7 +25,7 @@
         "typeArguments": null
       },
       "name": "C2",
-      "offset": 3444
+      "offset": 3460
     },
     {
       "configurableType": {
@@ -34,7 +34,7 @@
         "typeArguments": []
       },
       "name": "C3",
-      "offset": 3476
+      "offset": 3492
     },
     {
       "configurableType": {
@@ -43,7 +43,7 @@
         "typeArguments": []
       },
       "name": "C4",
-      "offset": 3492
+      "offset": 3508
     },
     {
       "configurableType": {
@@ -52,7 +52,7 @@
         "typeArguments": []
       },
       "name": "C5",
-      "offset": 3508
+      "offset": 3524
     },
     {
       "configurableType": {
@@ -61,7 +61,7 @@
         "typeArguments": null
       },
       "name": "C6",
-      "offset": 3524
+      "offset": 3540
     },
     {
       "configurableType": {
@@ -70,7 +70,7 @@
         "typeArguments": null
       },
       "name": "C7",
-      "offset": 3540
+      "offset": 3556
     },
     {
       "configurableType": {
@@ -79,7 +79,7 @@
         "typeArguments": null
       },
       "name": "C9",
-      "offset": 3588
+      "offset": 3604
     }
   ],
   "functions": [

diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/memcpy/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/language/memcpy/Forc.lock
@@ -0,0 +1,13 @@
+[[package]]
+name = "core"
+source = "path+from-root-1621225C7A88836C"
+
+[[package]]
+name = "memcpy"
+source = "member"
+dependencies = ["std"]
+
+[[package]]
+name = "std"
+source = "path+from-root-1621225C7A88836C"
+dependencies = ["core"]
diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/memcpy/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/language/memcpy/Forc.toml
@@ -0,0 +1,8 @@
+[project]
+authors = ["Fuel Labs <[email protected]>"]
+license = "Apache-2.0"
+name = "memcpy"
+entry = "main.sw"
+
+[dependencies]
+std = { path = "../../../../../../../sway-lib-std" }
diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/memcpy/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/language/memcpy/src/main.sw
@@ -0,0 +1,18 @@
+script;
+
+fn main() -> u64 {
+    let a = A { a: 11 };
+    let mut ptr_a = ptr(a);
+    ptr_a.write(A { a: 22 });
+    assert(a.a == 11);
+    a.a
+}
+
+struct A {
+  a: u64,
+}
+
+#[inline(never)]
+fn ptr<T>(t: T) -> raw_ptr {
+    __addr_of(t)
+}
diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/memcpy/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/language/memcpy/test.toml
@@ -0,0 +1,4 @@
+category = "run"
+expected_result = { action = "return", value = 11 }
+validate_abi = false
+expected_warnings=6
diff --git a/...ests/test_programs/should_pass/require_contract_deployment/call_basic_storage/src/main.sw b/...ests/test_programs/should_pass/require_contract_deployment/call_basic_storage/src/main.sw
@@ -2,7 +2,7 @@ script;
 use basic_storage_abi::{BasicStorage, Quad};
 
 fn main() -> u64 {
-    let addr = abi(BasicStorage, 0x01e64bbd633f1e8cf9d9a712bc7b8f1003c7b8d9796a0d3cd234370881ea6f0a);
+    let addr = abi(BasicStorage, 0xafd842e74db4e85a770b0569a9df1226d1f57087cb331be88aa37387f6d8841d);
     let key = 0x0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;
     let value = 4242;