Skip to content

Commit

Permalink
fix: correct result when assigning shared arrays in unconstrained code (
Browse files Browse the repository at this point in the history
#4210)

# Description

## Problem\*

Resolves #3795

## Summary\*

`Instruction::IncrementRc` in Brillig actually only increments the
reference count on a copy of an array's metadata if the array was loaded
from a reference (while the loaded array is a reference rather than a
copy). This is suboptimal since it was meant to be shared across
references to the array. This is what caused the original issue where
mutating the reference saw a different reference count and thought it
was safe to mutate without copying first.

I've added a somewhat hacky check in the method to increment reference
counts to check if the array was loaded from a reference. If so, we load
a fresh value, increment rc on that, and re-store it to update the
metadata in the original array.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** 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.

---------

Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
  • Loading branch information
jfecher and vezenovm committed Feb 5, 2024
1 parent c58d691 commit bdd8a96
Show file tree
Hide file tree
Showing 6 changed files with 39 additions and 1 deletion.
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/function_builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -487,9 +487,9 @@ impl FunctionBuilder {
}
}
Type::Array(..) | Type::Slice(..) => {
self.insert_instruction(Instruction::IncrementRc { value }, None);
// If there are nested arrays or slices, we wait until ArrayGet
// is issued to increment the count of that array.
self.insert_instruction(Instruction::IncrementRc { value }, None);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -999,6 +999,7 @@ impl<'a> FunctionContext<'a> {
new_value.for_each(|value| {
let value = value.eval(self);
array = self.builder.insert_array_set(array, index, value);
self.builder.increment_array_reference_count(array);
index = self.builder.insert_binary(index, BinaryOp::Add, one);
});
array
Expand Down
5 changes: 5 additions & 0 deletions compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -730,6 +730,11 @@ impl<'a> FunctionContext<'a> {
let lhs = self.extract_current_value(&assign.lvalue)?;
let rhs = self.codegen_expression(&assign.expression)?;

rhs.clone().for_each(|value| {
let value = value.eval(self);
self.builder.increment_array_reference_count(value);
});

self.assign_new_value(lhs, rhs);
Ok(Self::unit_value())
}
Expand Down
7 changes: 7 additions & 0 deletions test_programs/execution_success/brillig_cow_assign/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "brillig_cow_assign"
type = "bin"
authors = [""]
compiler_version = ">=0.23.0"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
items_to_update = 10
index = 6
23 changes: 23 additions & 0 deletions test_programs/execution_success/brillig_cow_assign/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
global N = 10;

unconstrained fn main() {
let mut arr = [0; N];
let mut mid_change = arr;

for i in 0..N {
if i == N / 2 {
mid_change = arr;
}
arr[i] = 27;
}

// Expect:
// arr = [27, 27, 27, 27, 27, 27, 27, 27, 27, 27]
// mid_change = [27, 27, 27, 27, 27, 0, 0, 0, 0, 0]

let modified_i = N / 2 + 1;
assert_eq(arr[modified_i], 27);

// Fail here!
assert(mid_change[modified_i] != 27);
}

0 comments on commit bdd8a96

Please sign in to comment.