From 5b1c896c605ed1047fc17a437e0b58792a778e2d Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 24 Sep 2024 12:17:23 -0400 Subject: [PATCH] fix(ssa): RC correctness issue (#6134) # Description ## Problem\* Resolves #6123 ## Summary\* Still a draft as I want to test it a bit more and am not fully sure if it is the best solution. ## Additional Context ## Documentation\* Check one: - [X] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [X] I have tested the changes locally. - [X] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../src/ssa/ssa_gen/context.rs | 4 ++ .../brillig_rc_regression_6123/Nargo.toml | 7 ++++ .../brillig_rc_regression_6123/src/main.nr | 41 +++++++++++++++++++ tooling/debugger/ignored-tests.txt | 3 +- 4 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 test_programs/execution_success/brillig_rc_regression_6123/Nargo.toml create mode 100644 test_programs/execution_success/brillig_rc_regression_6123/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index fb7091a8854..c71c3a33edf 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -732,6 +732,10 @@ impl<'a> FunctionContext<'a> { let element_types = Self::convert_type(element_type); values.map_both(element_types, |value, element_type| { let reference = value.eval_reference(); + // Reference counting in brillig relies on us incrementing reference + // counts when arrays/slices are constructed or indexed. + // Thus, if we dereference an lvalue which happens to be array/slice we should increment its reference counter. + self.builder.increment_array_reference_count(reference); self.builder.insert_load(reference, element_type).into() }) } diff --git a/test_programs/execution_success/brillig_rc_regression_6123/Nargo.toml b/test_programs/execution_success/brillig_rc_regression_6123/Nargo.toml new file mode 100644 index 00000000000..533777df67f --- /dev/null +++ b/test_programs/execution_success/brillig_rc_regression_6123/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "brillig_rc_regression_6123" +type = "bin" +authors = [""] +compiler_version = ">=0.34.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_success/brillig_rc_regression_6123/src/main.nr b/test_programs/execution_success/brillig_rc_regression_6123/src/main.nr new file mode 100644 index 00000000000..3eb29659944 --- /dev/null +++ b/test_programs/execution_success/brillig_rc_regression_6123/src/main.nr @@ -0,0 +1,41 @@ +struct Builder { + note_hashes: BoundedVec, + nullifiers: BoundedVec, +} + +impl Builder { + fn append_note_hashes_with_logs(&mut self, num_note_hashes: u32) { + let index_offset = self.note_hashes.len(); + for i in 0..self.note_hashes.max_len() { + if i < num_note_hashes { + self.add_new_note_hash((index_offset + i) as Field); + } + } + } + + fn add_new_note_hash(&mut self, value: Field) { + self.note_hashes.push(value); + } +} + +fn swap_items(vec: &mut BoundedVec, from_index: u32, to_index: u32) { + let tmp = vec.storage[from_index]; + vec.storage[from_index] = vec.storage[to_index]; + vec.storage[to_index] = tmp; +} + +unconstrained fn main() { + let mut builder = Builder { note_hashes: BoundedVec::new(), nullifiers: BoundedVec::new() }; + + builder.append_note_hashes_with_logs(2); + builder.nullifiers.storage[1] = 27; + // Get ordered items before shuffling. + let note_hashes = builder.note_hashes.storage; + let original_first_note_hash = note_hashes[0]; + // Shuffle. + swap_items(&mut builder.note_hashes, 1, 0); + + for i in 0..1 { + assert_eq(note_hashes[i], original_first_note_hash); + } +} diff --git a/tooling/debugger/ignored-tests.txt b/tooling/debugger/ignored-tests.txt index 78e14397938..0037b8e5d5f 100644 --- a/tooling/debugger/ignored-tests.txt +++ b/tooling/debugger/ignored-tests.txt @@ -4,4 +4,5 @@ is_unconstrained macros references regression_4709 -reference_only_used_as_alias \ No newline at end of file +reference_only_used_as_alias +brillig_rc_regression_6123 \ No newline at end of file