From ae31213b8832110e74b55ce72f99b32beb519d75 Mon Sep 17 00:00:00 2001 From: Vaivaswatha N Date: Thu, 2 Nov 2023 15:07:14 +0530 Subject: [PATCH] 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 --- sway-ir/src/optimize/memcpyopt.rs | 16 +++++++++++++--- .../configurable_consts/json_abi_oracle.json | 18 +++++++++--------- .../should_pass/language/memcpy/Forc.lock | 13 +++++++++++++ .../should_pass/language/memcpy/Forc.toml | 8 ++++++++ .../should_pass/language/memcpy/src/main.sw | 18 ++++++++++++++++++ .../should_pass/language/memcpy/test.toml | 4 ++++ .../call_basic_storage/src/main.sw | 2 +- 7 files changed, 66 insertions(+), 13 deletions(-) create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/memcpy/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/memcpy/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/memcpy/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/language/memcpy/test.toml diff --git a/sway-ir/src/optimize/memcpyopt.rs b/sway-ir/src/optimize/memcpyopt.rs index 3cbe3546033..fcc2b1e09bd 100644 --- 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::>::default(); let mut stores_map = FxHashMap::>::default(); let mut instr_info_map = FxHashMap::::default(); - let mut asm_uses = FxHashSet::::default(); + let mut escaping_uses = FxHashSet::::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/test/src/e2e_vm_tests/test_programs/should_pass/language/configurable_consts/json_abi_oracle.json b/test/src/e2e_vm_tests/test_programs/should_pass/language/configurable_consts/json_abi_oracle.json index c50964b0a48..6fc3d2fb73c 100644 --- a/test/src/e2e_vm_tests/test_programs/should_pass/language/configurable_consts/json_abi_oracle.json +++ b/test/src/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 new file mode 100644 index 00000000000..dab30bc5604 --- /dev/null +++ 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 new file mode 100644 index 00000000000..fd4b8ac81bb --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/memcpy/Forc.toml @@ -0,0 +1,8 @@ +[project] +authors = ["Fuel Labs "] +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 new file mode 100644 index 00000000000..40d1f2edb0c --- /dev/null +++ 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) -> 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 new file mode 100644 index 00000000000..ff8fd970753 --- /dev/null +++ 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 \ No newline at end of file diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/require_contract_deployment/call_basic_storage/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/require_contract_deployment/call_basic_storage/src/main.sw index 2e31db02285..4488fb92ce7 100644 --- a/test/src/e2e_vm_tests/test_programs/should_pass/require_contract_deployment/call_basic_storage/src/main.sw +++ b/test/src/e2e_vm_tests/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;