diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index d8f1f9d0997..8ae4eddf1f5 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -20,7 +20,6 @@ use crate::ssa::ir::{ value::{Value, ValueId}, }; use acvm::acir::brillig::{MemoryAddress, ValueOrArray}; -use acvm::brillig_vm::MEMORY_ADDRESSING_BIT_SIZE; use acvm::{FieldElement, acir::AcirField}; use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use iter_extended::vecmap; @@ -872,65 +871,40 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { .store_instruction(array_or_vector.extract_register(), rc_register); self.brillig_context.deallocate_register(rc_register); } - Instruction::DecrementRc { value, original } => { + Instruction::DecrementRc { value } => { let array_or_vector = self.convert_ssa_value(*value, dfg); - let orig_array_or_vector = self.convert_ssa_value(*original, dfg); - let array_register = array_or_vector.extract_register(); - let orig_array_register = orig_array_or_vector.extract_register(); - - let codegen_dec_rc = |ctx: &mut BrilligContext| { - let rc_register = ctx.allocate_register(); - - ctx.load_instruction(rc_register, array_register); - - ctx.codegen_usize_op_in_place(rc_register, BrilligBinaryOp::Sub, 1); - - // Check that the refcount didn't go below 1. If we allow it to underflow - // and become 0 or usize::MAX, and then return to 1, then it will indicate - // an array as mutable when it probably shouldn't be. - if ctx.enable_debug_assertions() { - let min_addr = ReservedRegisters::usize_one(); - let condition = SingleAddrVariable::new(ctx.allocate_register(), 1); - ctx.memory_op_instruction( - min_addr, - rc_register, - condition.address, - BrilligBinaryOp::LessThanEquals, - ); - ctx.codegen_constrain( - condition, - Some("array ref-count went to zero".to_owned()), - ); - ctx.deallocate_single_addr(condition); - } - ctx.store_instruction(array_register, rc_register); - ctx.deallocate_register(rc_register); - }; + let rc_register = self.brillig_context.allocate_register(); + self.brillig_context.load_instruction(rc_register, array_register); - if array_register == orig_array_register { - codegen_dec_rc(self.brillig_context); - } else { - // Check if the current and original register point at the same memory. - // If they don't, it means that an array-copy procedure has created a - // new array, which includes decrementing the RC of the original, - // which means we don't have to do the decrement here. + // Check that the refcount isn't already 0 before we decrement. If we allow it to underflow + // and become usize::MAX, and then return to 1, then it will indicate + // an array as mutable when it probably shouldn't be. + if self.brillig_context.enable_debug_assertions() { + let min_addr = ReservedRegisters::usize_one(); let condition = SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); - // We will use a binary op to interpret the pointer as a number. - let bit_size: u32 = MEMORY_ADDRESSING_BIT_SIZE.into(); - let array_var = SingleAddrVariable::new(array_register, bit_size); - let orig_array_var = SingleAddrVariable::new(orig_array_register, bit_size); - self.brillig_context.binary_instruction( - array_var, - orig_array_var, + self.brillig_context.memory_op_instruction( + min_addr, + rc_register, + condition.address, + BrilligBinaryOp::LessThan, + ); + self.brillig_context.codegen_constrain( condition, - BrilligBinaryOp::Equals, + Some("array ref-count underflow detected".to_owned()), ); - self.brillig_context.codegen_if(condition.address, codegen_dec_rc); self.brillig_context.deallocate_single_addr(condition); } + + self.brillig_context.codegen_usize_op_in_place( + rc_register, + BrilligBinaryOp::Sub, + 1, + ); + self.brillig_context.store_instruction(array_register, rc_register); + self.brillig_context.deallocate_register(rc_register); } Instruction::EnableSideEffectsIf { .. } => { unreachable!("enable_side_effects not supported by brillig") diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 700459d3c74..b8ef7bee4fb 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -1056,8 +1056,8 @@ mod test { v19 = call f1(v5) -> u32 v20 = add v8, v19 constrain v6 == v20 - dec_rc v4 v4 - dec_rc v5 v5 + dec_rc v4 + dec_rc v5 return } diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 95944129c4e..66643fc3beb 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -394,8 +394,8 @@ impl FunctionBuilder { /// Insert an instruction to decrement an array's reference count. This only has an effect /// in unconstrained code where arrays are reference counted and copy on write. - pub(crate) fn insert_dec_rc(&mut self, value: ValueId, original: ValueId) { - self.insert_instruction(Instruction::DecrementRc { value, original }, None); + pub(crate) fn insert_dec_rc(&mut self, value: ValueId) { + self.insert_instruction(Instruction::DecrementRc { value }, None); } /// Insert an enable_side_effects_if instruction. These are normally only automatically @@ -497,7 +497,7 @@ impl FunctionBuilder { /// /// Returns the ID of the array that was affected, if any. pub(crate) fn increment_array_reference_count(&mut self, value: ValueId) -> Option { - self.update_array_reference_count(value, None) + self.update_array_reference_count(value, true) } /// Insert instructions to decrement the reference count of any array(s) stored @@ -505,12 +505,8 @@ impl FunctionBuilder { /// any arrays, this does nothing. /// /// Returns the ID of the array that was affected, if any. - pub(crate) fn decrement_array_reference_count( - &mut self, - value: ValueId, - original: ValueId, - ) -> Option { - self.update_array_reference_count(value, Some(original)) + pub(crate) fn decrement_array_reference_count(&mut self, value: ValueId) -> Option { + self.update_array_reference_count(value, false) } /// Increment or decrement the given value's reference count if it is an array. @@ -522,11 +518,7 @@ impl FunctionBuilder { /// the meantime, which in itself would make sure its RC is decremented. /// /// Returns the ID of the array that was affected, if any. - fn update_array_reference_count( - &mut self, - value: ValueId, - original: Option, - ) -> Option { + fn update_array_reference_count(&mut self, value: ValueId, increment: bool) -> Option { if self.current_function.runtime().is_acir() { return None; } @@ -536,7 +528,7 @@ impl FunctionBuilder { if element.contains_an_array() { let reference = value; let value = self.insert_load(reference, element.as_ref().clone()); - self.update_array_reference_count(value, original); + self.update_array_reference_count(value, increment); Some(value) } else { None @@ -545,10 +537,10 @@ impl FunctionBuilder { Type::Array(..) | Type::Slice(..) => { // If there are nested arrays or slices, we wait until ArrayGet // is issued to increment the count of that array. - if let Some(original) = original { - self.insert_dec_rc(value, original); - } else { + if increment { self.insert_inc_rc(value); + } else { + self.insert_dec_rc(value); } Some(value) } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 1bef9079eb8..dce437ac2a4 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -325,9 +325,7 @@ pub(crate) enum Instruction { /// This currently only has an effect in Brillig code where array sharing and copy on write is /// implemented via reference counting. In ACIR code this is done with im::Vector and these /// DecrementRc instructions are ignored. - /// - /// The `original` contains the value of the array which was incremented by the pair of this decrement. - DecrementRc { value: ValueId, original: ValueId }, + DecrementRc { value: ValueId }, /// Merge two values returned from opposite branches of a conditional into one. /// @@ -724,9 +722,7 @@ impl Instruction { mutable: *mutable, }, Instruction::IncrementRc { value } => Instruction::IncrementRc { value: f(*value) }, - Instruction::DecrementRc { value, original } => { - Instruction::DecrementRc { value: f(*value), original: f(*original) } - } + Instruction::DecrementRc { value } => Instruction::DecrementRc { value: f(*value) }, Instruction::RangeCheck { value, max_bit_size, assert_message } => { Instruction::RangeCheck { value: f(*value), @@ -797,9 +793,8 @@ impl Instruction { *value = f(*value); } Instruction::IncrementRc { value } => *value = f(*value), - Instruction::DecrementRc { value, original } => { + Instruction::DecrementRc { value } => { *value = f(*value); - *original = f(*original); } Instruction::RangeCheck { value, max_bit_size: _, assert_message: _ } => { *value = f(*value); @@ -866,12 +861,10 @@ impl Instruction { Instruction::EnableSideEffectsIf { condition } => { f(*condition); } - Instruction::IncrementRc { value } | Instruction::RangeCheck { value, .. } => { - f(*value); - } - Instruction::DecrementRc { value, original } => { + Instruction::IncrementRc { value } + | Instruction::DecrementRc { value } + | Instruction::RangeCheck { value, .. } => { f(*value); - f(*original); } Instruction::IfElse { then_condition, then_value, else_condition, else_value } => { f(*then_condition); diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 0fac2242abd..2809139da45 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -250,8 +250,8 @@ fn display_instruction_inner( Instruction::IncrementRc { value } => { writeln!(f, "inc_rc {}", show(*value)) } - Instruction::DecrementRc { value, original } => { - writeln!(f, "dec_rc {} {}", show(*value), show(*original)) + Instruction::DecrementRc { value } => { + writeln!(f, "dec_rc {}", show(*value)) } Instruction::RangeCheck { value, max_bit_size, .. } => { writeln!(f, "range_check {} to {} bits", show(*value), *max_bit_size,) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 733443908b2..67f08c13338 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -1445,7 +1445,7 @@ mod test { v2 = array_get v0, index u32 0 -> Field v4 = array_get v0, index u32 1 -> Field v5 = add v2, v4 - dec_rc v0 v0 + dec_rc v0 return v5 } "; diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index e742ad4aa5d..842946deb9e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -838,7 +838,7 @@ mod test { b0(v0: [Field; 2]): inc_rc v0 v2 = array_get v0, index u32 0 -> Field - dec_rc v0 v0 + dec_rc v0 return v2 } "; @@ -862,7 +862,7 @@ mod test { b0(v0: [Field; 2]): inc_rc v0 v2 = array_set v0, index u32 0, value u32 0 - dec_rc v0 v0 + dec_rc v0 return v2 } "; @@ -970,7 +970,7 @@ mod test { v3 = load v0 -> [Field; 3] v6 = array_set v3, index u32 0, value Field 5 store v6 at v0 - dec_rc v6 v1 + dec_rc v6 return } "; diff --git a/compiler/noirc_evaluator/src/ssa/opt/rc.rs b/compiler/noirc_evaluator/src/ssa/opt/rc.rs index 13137c94046..5fa60c7a3ec 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/rc.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/rc.rs @@ -190,7 +190,7 @@ mod test { b0(v0: [Field; 2]): inc_rc v0 inc_rc v0 - dec_rc v0 v0 + dec_rc v0 v1 = make_array [v0] : [[Field; 2]; 1] return v1 } @@ -218,7 +218,7 @@ mod test { v2 = load v1 -> [Field; 2] v5 = array_set v2, index u64 0, value Field 5 store v5 at v1 - dec_rc v0 v0 + dec_rc v0 return } "; @@ -250,7 +250,7 @@ mod test { v5 = array_set v2, index u64 0, value Field 5 store v5 at v0 v6 = load v0 -> [Field; 2] - dec_rc v6 v1 + dec_rc v1 store v6 at v0 return } diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 5b7e34c8e0d..bff278c9aa9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -1287,7 +1287,7 @@ mod tests { jmp b1() b1(): v20 = load v3 -> [u64; 6] - dec_rc v0 v0 + dec_rc v0 return v20 } "; @@ -1463,7 +1463,7 @@ mod tests { jmp b1(v16) b2(): v8 = load v4 -> [u64; 6] - dec_rc v0 v0 + dec_rc v0 return v8 }} " diff --git a/compiler/noirc_evaluator/src/ssa/parser/ast.rs b/compiler/noirc_evaluator/src/ssa/parser/ast.rs index 9126e98ecac..25dcb35df7f 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/ast.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/ast.rs @@ -116,7 +116,6 @@ pub(crate) enum ParsedInstruction { }, DecrementRc { value: ParsedValue, - original: ParsedValue, }, EnableSideEffectsIf { condition: ParsedValue, diff --git a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs index df5f545eb72..771ea458ded 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs @@ -284,10 +284,9 @@ impl Translator { }; self.builder.insert_constrain(lhs, rhs, assert_message); } - ParsedInstruction::DecrementRc { value, original } => { + ParsedInstruction::DecrementRc { value } => { let value = self.translate_value(value)?; - let original = self.translate_value(original)?; - self.builder.decrement_array_reference_count(value, original); + self.builder.decrement_array_reference_count(value); } ParsedInstruction::EnableSideEffectsIf { condition } => { let condition = self.translate_value(condition)?; diff --git a/compiler/noirc_evaluator/src/ssa/parser/mod.rs b/compiler/noirc_evaluator/src/ssa/parser/mod.rs index c3965962a50..f270b390e1d 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/mod.rs @@ -407,8 +407,7 @@ impl<'a> Parser<'a> { } let value = self.parse_value_or_error()?; - let original = self.parse_value_or_error()?; - Ok(Some(ParsedInstruction::DecrementRc { value, original })) + Ok(Some(ParsedInstruction::DecrementRc { value })) } fn parse_enable_side_effects(&mut self) -> ParseResult> { diff --git a/compiler/noirc_evaluator/src/ssa/parser/tests.rs b/compiler/noirc_evaluator/src/ssa/parser/tests.rs index b9d5b8ede31..7f3caf26422 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/tests.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/tests.rs @@ -474,7 +474,7 @@ fn test_dec_rc() { let src = " brillig(inline) fn main f0 { b0(v0: [Field; 3]): - dec_rc v0 v0 + dec_rc v0 return } "; diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index c49b28376a6..4748712cabe 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -1001,11 +1001,15 @@ impl<'a> FunctionContext<'a> { mut incremented_params: Vec<(ValueId, ValueId)>, terminator_args: &[ValueId], ) { + // TODO: This check likely leads to unsoundness. + // It is here to avoid decrementing the RC of a parameter we're returning but we + // only check the exact ValueId which can be easily circumvented by storing to and + // loading from a temporary reference. incremented_params.retain(|(parameter, _)| !terminator_args.contains(parameter)); for (parameter, original) in incremented_params { if self.builder.current_function.dfg.value_is_reference(parameter) { - self.builder.decrement_array_reference_count(parameter, original); + self.builder.decrement_array_reference_count(original); } } } diff --git a/test_programs/execution_success/reference_counts/Nargo.toml b/test_programs/execution_success/reference_counts_inliner_0/Nargo.toml similarity index 70% rename from test_programs/execution_success/reference_counts/Nargo.toml rename to test_programs/execution_success/reference_counts_inliner_0/Nargo.toml index ae787e0ccb9..06967e1b893 100644 --- a/test_programs/execution_success/reference_counts/Nargo.toml +++ b/test_programs/execution_success/reference_counts_inliner_0/Nargo.toml @@ -1,5 +1,5 @@ [package] -name = "reference_counts" +name = "reference_counts_inliner_0" type = "bin" authors = [""] compiler_version = ">=0.35.0" diff --git a/test_programs/execution_success/reference_counts/Prover.toml b/test_programs/execution_success/reference_counts_inliner_0/Prover.toml similarity index 100% rename from test_programs/execution_success/reference_counts/Prover.toml rename to test_programs/execution_success/reference_counts_inliner_0/Prover.toml diff --git a/test_programs/execution_success/reference_counts/src/main.nr b/test_programs/execution_success/reference_counts_inliner_0/src/main.nr similarity index 75% rename from test_programs/execution_success/reference_counts/src/main.nr rename to test_programs/execution_success/reference_counts_inliner_0/src/main.nr index 2ee8a13f7a4..0af6e0e79a0 100644 --- a/test_programs/execution_success/reference_counts/src/main.nr +++ b/test_programs/execution_success/reference_counts_inliner_0/src/main.nr @@ -16,9 +16,7 @@ fn main() { borrow_mut_two_separate(&mut array, &mut u32_array, rc1, rc2); // Safety: test - unsafe { - regression_7297(); - } + regression_7297(); } fn borrow(array: [Field; 3], rc_before_call: u32) { @@ -85,8 +83,7 @@ fn assert_refcount(array: [T; 3], expected: u32) { } } -unconstrained fn regression_7297() { - println("regression_7297"); +fn regression_7297() { let mut array = [0, 1, 2]; let refcount_0 = array_refcount(array); @@ -95,23 +92,27 @@ unconstrained fn regression_7297() { let array_2 = copy_mut(array, refcount_1); let refcount_2 = array_refcount(array); - println([refcount_0, refcount_1, refcount_2]); - // Mutation of the original could occur if we double decremented the RC and then went back to 1 by accident. // For this to come out we have to run the test with `--inliner-aggressiveness -9223372036854775808` assert_eq(array[0], 6, "the original should not be mutated by copy_mut, only borrow_mut_two"); assert_eq(array_2[0], 4, "the copy should have the expected content"); - // Double decrementing the RC could occur if we don't realize that array mutation made a copy, - // which decreases the RC of the original and sets the new one to 1. - // This assertion is redundant with the one following it, but it's here because `assert_eq` doesn't print - // what actual values that cause it to fail, so this is a way to highlight the bug about the refcount of - // still live arrays going to zero, without any doubt that it's just not 1, as it should be. - assert(refcount_1 != 0, "borrow_mut_two should create a fresh array and not decrease its RC"); - assert_eq(refcount_1, 1, "borrow_mut_two should create a fresh array with an RC of 1"); - // XXX: This assertion is here to demonstrate that this is happening, not because it must be like this - assert_eq( - refcount_2, - refcount_1 + 1, - "not ideal, but original RC permanently increased by copy_mut", - ); + + if std::runtime::is_unconstrained() { + // Double decrementing the RC could occur if we don't realize that array mutation made a copy, + // which decreases the RC of the original and sets the new one to 1. + // This assertion is redundant with the one following it, but it's here because `assert_eq` doesn't print + // what actual values that cause it to fail, so this is a way to highlight the bug about the refcount of + // still live arrays going to zero, without any doubt that it's just not 1, as it should be. + assert( + refcount_1 != 0, + "borrow_mut_two should create a fresh array and not decrease its RC", + ); + assert_eq(refcount_1, 1, "borrow_mut_two should create a fresh array with an RC of 1"); + // XXX: This assertion is here to demonstrate that this is happening, not because it must be like this + assert_eq( + refcount_2, + refcount_1 + 1, + "not ideal, but original RC permanently increased by copy_mut", + ); + } } diff --git a/test_programs/execution_success/reference_counts_inliner_0/stdout.txt b/test_programs/execution_success/reference_counts_inliner_0/stdout.txt new file mode 100644 index 00000000000..7e020b1074f --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_0/stdout.txt @@ -0,0 +1,10 @@ +0x00 +0x03 +0x04 +0x06 +0x06 +0x07 +8 +0x06 +0x06 +0x04 diff --git a/test_programs/execution_success/reference_counts_inliner_max/Nargo.toml b/test_programs/execution_success/reference_counts_inliner_max/Nargo.toml new file mode 100644 index 00000000000..654bcb5281d --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_max/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "reference_counts_inliner_max" +type = "bin" +authors = [""] +compiler_version = ">=0.35.0" + +[dependencies] diff --git a/test_programs/execution_success/reference_counts_inliner_max/Prover.toml b/test_programs/execution_success/reference_counts_inliner_max/Prover.toml new file mode 100644 index 00000000000..c01dd9462d8 --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_max/Prover.toml @@ -0,0 +1,2 @@ +x = 5 +b = true diff --git a/test_programs/execution_success/reference_counts_inliner_max/src/main.nr b/test_programs/execution_success/reference_counts_inliner_max/src/main.nr new file mode 100644 index 00000000000..f0c34ced4d1 --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_max/src/main.nr @@ -0,0 +1,121 @@ +use std::mem::array_refcount; + +fn main() { + let mut array = [0, 1, 2]; + assert_refcount(array, 1); + + borrow(array, array_refcount(array)); + borrow_mut(&mut array, array_refcount(array)); + let _ = copy_mut(array, array_refcount(array)); + + borrow_mut_two(&mut array, &mut array, array_refcount(array)); + + let mut u32_array = [0, 1, 2]; + let rc1 = array_refcount(array); + let rc2 = array_refcount(u32_array); + borrow_mut_two_separate(&mut array, &mut u32_array, rc1, rc2); + + // Safety: test + regression_7297(); +} + +fn borrow(array: [Field; 3], rc_before_call: u32) { + assert_refcount(array, rc_before_call); + println(array[0]); +} + +fn borrow_mut(array: &mut [Field; 3], rc_before_call: u32) { + // Optimization: inc_rc isn't needed since there is only one array (`array`) + // of the same type that `array` can be modified through + assert_refcount(*array, rc_before_call + 0); + array[0] = 3; + println(array[0]); +} + +// Returning a copy of the array, otherwise the SSA can end up optimizing away +// the `array_set`, with the whole body just becoming basically `println(4);`. +fn copy_mut(mut array: [Field; 3], rc_before_call: u32) -> [Field; 3] { + assert_refcount(array, rc_before_call + 1); + array[0] = 4; + println(array[0]); + array +} + +/// Borrow the same array mutably through both parameters, inc_rc is necessary here, although +/// only one is needed to bring the rc from 1 to 2. +fn borrow_mut_two(array1: &mut [Field; 3], array2: &mut [Field; 3], rc_before_call: u32) { + assert_refcount(*array1, rc_before_call + 1); + assert_refcount(*array2, rc_before_call + 1); + array1[0] = 5; + array2[0] = 6; + println(array1[0]); // array1 & 2 alias, so this should also print 6 + println(array2[0]); +} + +/// Borrow a different array: we should be able to reason that these types cannot be mutably +/// aliased since they're different types so we don't need any inc_rc instructions. +fn borrow_mut_two_separate( + array1: &mut [Field; 3], + array2: &mut [u32; 3], + rc_before_call1: u32, + rc_before_call2: u32, +) { + assert_refcount(*array1, rc_before_call1 + 0); + assert_refcount(*array2, rc_before_call2 + 0); + array1[0] = 7; + array2[0] = 8; + println(array1[0]); + println(array2[0]); +} + +fn assert_refcount(array: [T; 3], expected: u32) { + let count = array_refcount(array); + + // All ref counts are zero when running this as a constrained program + if std::runtime::is_unconstrained() { + if count != expected { + // Brillig doesn't print the actual & expected arguments on assertion failure + println(f"actual = {count}, expected = {expected}"); + } + assert_eq(count, expected); + } else { + assert_eq(count, 0); + } +} + +fn regression_7297() { + let mut array = [0, 1, 2]; + + let refcount_0 = array_refcount(array); + borrow_mut_two(&mut array, &mut array, refcount_0); + let refcount_1 = array_refcount(array); + let array_2 = copy_mut(array, refcount_1); + let refcount_2 = array_refcount(array); + + // Mutation of the original could occur if we double decremented the RC and then went back to 1 by accident. + // For this to come out we have to run the test with `--inliner-aggressiveness -9223372036854775808` + assert_eq(array[0], 6, "the original should not be mutated by copy_mut, only borrow_mut_two"); + assert_eq(array_2[0], 4, "the copy should have the expected content"); + + if std::runtime::is_unconstrained() { + // Double decrementing the RC could occur if we don't realize that array mutation made a copy, + // which decreases the RC of the original and sets the new one to 1. + // This assertion is redundant with the one following it, but it's here because `assert_eq` doesn't print + // what actual values that cause it to fail, so this is a way to highlight the bug about the refcount of + // still live arrays going to zero, without any doubt that it's just not 1, as it should be. + assert( + refcount_1 != 0, + "borrow_mut_two should create a fresh array and not decrease its RC", + ); + + // Difference from other inliner cases. With inliner max we fail to remove a inc_ref + assert_eq(refcount_1, 2); + + // XXX: This assertion is here to demonstrate that this is happening, not because it must be like this + assert_eq( + refcount_2, + refcount_1 + 1, + "not ideal, but original RC permanently increased by copy_mut", + ); + } +} diff --git a/test_programs/execution_success/reference_counts_inliner_max/stdout.txt b/test_programs/execution_success/reference_counts_inliner_max/stdout.txt new file mode 100644 index 00000000000..7e020b1074f --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_max/stdout.txt @@ -0,0 +1,10 @@ +0x00 +0x03 +0x04 +0x06 +0x06 +0x07 +8 +0x06 +0x06 +0x04 diff --git a/test_programs/execution_success/reference_counts_inliner_min/Nargo.toml b/test_programs/execution_success/reference_counts_inliner_min/Nargo.toml new file mode 100644 index 00000000000..92e22d8529c --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_min/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "reference_counts_inliner_min" +type = "bin" +authors = [""] +compiler_version = ">=0.35.0" + +[dependencies] diff --git a/test_programs/execution_success/reference_counts_inliner_min/Prover.toml b/test_programs/execution_success/reference_counts_inliner_min/Prover.toml new file mode 100644 index 00000000000..c01dd9462d8 --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_min/Prover.toml @@ -0,0 +1,2 @@ +x = 5 +b = true diff --git a/test_programs/execution_success/reference_counts_inliner_min/src/main.nr b/test_programs/execution_success/reference_counts_inliner_min/src/main.nr new file mode 100644 index 00000000000..0af6e0e79a0 --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_min/src/main.nr @@ -0,0 +1,118 @@ +use std::mem::array_refcount; + +fn main() { + let mut array = [0, 1, 2]; + assert_refcount(array, 1); + + borrow(array, array_refcount(array)); + borrow_mut(&mut array, array_refcount(array)); + let _ = copy_mut(array, array_refcount(array)); + + borrow_mut_two(&mut array, &mut array, array_refcount(array)); + + let mut u32_array = [0, 1, 2]; + let rc1 = array_refcount(array); + let rc2 = array_refcount(u32_array); + borrow_mut_two_separate(&mut array, &mut u32_array, rc1, rc2); + + // Safety: test + regression_7297(); +} + +fn borrow(array: [Field; 3], rc_before_call: u32) { + assert_refcount(array, rc_before_call); + println(array[0]); +} + +fn borrow_mut(array: &mut [Field; 3], rc_before_call: u32) { + // Optimization: inc_rc isn't needed since there is only one array (`array`) + // of the same type that `array` can be modified through + assert_refcount(*array, rc_before_call + 0); + array[0] = 3; + println(array[0]); +} + +// Returning a copy of the array, otherwise the SSA can end up optimizing away +// the `array_set`, with the whole body just becoming basically `println(4);`. +fn copy_mut(mut array: [Field; 3], rc_before_call: u32) -> [Field; 3] { + assert_refcount(array, rc_before_call + 1); + array[0] = 4; + println(array[0]); + array +} + +/// Borrow the same array mutably through both parameters, inc_rc is necessary here, although +/// only one is needed to bring the rc from 1 to 2. +fn borrow_mut_two(array1: &mut [Field; 3], array2: &mut [Field; 3], rc_before_call: u32) { + assert_refcount(*array1, rc_before_call + 1); + assert_refcount(*array2, rc_before_call + 1); + array1[0] = 5; + array2[0] = 6; + println(array1[0]); // array1 & 2 alias, so this should also print 6 + println(array2[0]); +} + +/// Borrow a different array: we should be able to reason that these types cannot be mutably +/// aliased since they're different types so we don't need any inc_rc instructions. +fn borrow_mut_two_separate( + array1: &mut [Field; 3], + array2: &mut [u32; 3], + rc_before_call1: u32, + rc_before_call2: u32, +) { + assert_refcount(*array1, rc_before_call1 + 0); + assert_refcount(*array2, rc_before_call2 + 0); + array1[0] = 7; + array2[0] = 8; + println(array1[0]); + println(array2[0]); +} + +fn assert_refcount(array: [T; 3], expected: u32) { + let count = array_refcount(array); + + // All ref counts are zero when running this as a constrained program + if std::runtime::is_unconstrained() { + if count != expected { + // Brillig doesn't print the actual & expected arguments on assertion failure + println(f"actual = {count}, expected = {expected}"); + } + assert_eq(count, expected); + } else { + assert_eq(count, 0); + } +} + +fn regression_7297() { + let mut array = [0, 1, 2]; + + let refcount_0 = array_refcount(array); + borrow_mut_two(&mut array, &mut array, refcount_0); + let refcount_1 = array_refcount(array); + let array_2 = copy_mut(array, refcount_1); + let refcount_2 = array_refcount(array); + + // Mutation of the original could occur if we double decremented the RC and then went back to 1 by accident. + // For this to come out we have to run the test with `--inliner-aggressiveness -9223372036854775808` + assert_eq(array[0], 6, "the original should not be mutated by copy_mut, only borrow_mut_two"); + assert_eq(array_2[0], 4, "the copy should have the expected content"); + + if std::runtime::is_unconstrained() { + // Double decrementing the RC could occur if we don't realize that array mutation made a copy, + // which decreases the RC of the original and sets the new one to 1. + // This assertion is redundant with the one following it, but it's here because `assert_eq` doesn't print + // what actual values that cause it to fail, so this is a way to highlight the bug about the refcount of + // still live arrays going to zero, without any doubt that it's just not 1, as it should be. + assert( + refcount_1 != 0, + "borrow_mut_two should create a fresh array and not decrease its RC", + ); + assert_eq(refcount_1, 1, "borrow_mut_two should create a fresh array with an RC of 1"); + // XXX: This assertion is here to demonstrate that this is happening, not because it must be like this + assert_eq( + refcount_2, + refcount_1 + 1, + "not ideal, but original RC permanently increased by copy_mut", + ); + } +} diff --git a/test_programs/execution_success/reference_counts_inliner_min/stdout.txt b/test_programs/execution_success/reference_counts_inliner_min/stdout.txt new file mode 100644 index 00000000000..7e020b1074f --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_min/stdout.txt @@ -0,0 +1,10 @@ +0x00 +0x03 +0x04 +0x06 +0x06 +0x07 +8 +0x06 +0x06 +0x04 diff --git a/tooling/debugger/ignored-tests.txt b/tooling/debugger/ignored-tests.txt index 1515e58a90f..3f26d0c1c12 100644 --- a/tooling/debugger/ignored-tests.txt +++ b/tooling/debugger/ignored-tests.txt @@ -2,7 +2,9 @@ brillig_references debug_logs is_unconstrained macros -reference_counts +reference_counts_inliner_0 +reference_counts_inliner_min +reference_counts_inliner_max references regression_4709 regression_7323 diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index b8c2d58b5b9..bc743ef1ed0 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -60,12 +60,22 @@ const IGNORED_BRILLIG_TESTS: [&str; 10] = [ "is_unconstrained", ]; -/// Tests which aren't expected to work with the default inliner cases. +/// Tests which aren't expected to work with the default minimum inliner cases. const INLINER_MIN_OVERRIDES: [(&str, i64); 1] = [ // 0 works if PoseidonHasher::write is tagged as `inline_always`, otherwise 22. ("eddsa", 0), ]; +/// Tests which aren't expected to work with the default maximum inliner cases. +const INLINER_MAX_OVERRIDES: [(&str, i64); 0] = []; + +/// These tests should only be run on exactly 1 inliner setting (the one given here) +const INLINER_OVERRIDES: [(&str, i64); 3] = [ + ("reference_counts_inliner_0", 0), + ("reference_counts_inliner_min", i64::MIN), + ("reference_counts_inliner_max", i64::MAX), +]; + /// Some tests are expected to have warnings /// These should be fixed and removed from this list. const TESTS_WITH_EXPECTED_WARNINGS: [&str; 4] = [ @@ -79,10 +89,7 @@ const TESTS_WITH_EXPECTED_WARNINGS: [&str; 4] = [ ]; /// Tests for which we don't check that stdout matches the expected output. -const TESTS_WITHOUT_STDOUT_CHECK: [&str; 1] = [ - // The output changes depending on whether `--force-brillig` is passed or not - "reference_counts", -]; +const TESTS_WITHOUT_STDOUT_CHECK: [&str; 0] = []; fn read_test_cases( test_data_dir: &Path, @@ -119,6 +126,8 @@ struct MatrixConfig { vary_inliner: bool, // If there is a non-default minimum inliner aggressiveness to use with the brillig tests. min_inliner: i64, + // If there is a non-default maximum inliner aggressiveness to use with the brillig tests. + max_inliner: i64, } // Enum to be able to preserve readable test labels and also compare to numbers. @@ -167,6 +176,9 @@ fn generate_test_cases( if !cases.iter().any(|c| c.value() == matrix_config.min_inliner) { cases.push(Inliner::Custom(matrix_config.min_inliner)); } + if !cases.iter().any(|c| c.value() == matrix_config.max_inliner) { + cases.push(Inliner::Custom(matrix_config.max_inliner)); + } cases } else { vec![Inliner::Default] @@ -177,7 +189,8 @@ fn generate_test_cases( let mut test_cases = Vec::new(); for brillig in &brillig_cases { for inliner in &inliner_cases { - if *brillig && inliner.value() < matrix_config.min_inliner { + let inliner_range = matrix_config.min_inliner..=matrix_config.max_inliner; + if *brillig && !inliner_range.contains(&inliner.value()) { continue; } test_cases.push(format!( @@ -281,17 +294,32 @@ fn generate_execution_success_tests(test_file: &mut File, test_data_dir: &Path) &MatrixConfig { vary_brillig: !IGNORED_BRILLIG_TESTS.contains(&test_name.as_str()), vary_inliner: true, - min_inliner: INLINER_MIN_OVERRIDES - .iter() - .find(|(n, _)| *n == test_name.as_str()) - .map(|(_, i)| *i) - .unwrap_or(i64::MIN), + min_inliner: min_inliner(&test_name), + max_inliner: max_inliner(&test_name), }, ); } writeln!(test_file, "}}").unwrap(); } +fn max_inliner(test_name: &str) -> i64 { + INLINER_MAX_OVERRIDES + .iter() + .chain(&INLINER_OVERRIDES) + .find(|(n, _)| *n == test_name) + .map(|(_, i)| *i) + .unwrap_or(i64::MAX) +} + +fn min_inliner(test_name: &str) -> i64 { + INLINER_MIN_OVERRIDES + .iter() + .chain(&INLINER_OVERRIDES) + .find(|(n, _)| *n == test_name) + .map(|(_, i)| *i) + .unwrap_or(i64::MIN) +} + fn generate_execution_failure_tests(test_file: &mut File, test_data_dir: &Path) { let test_type = "execution_failure"; let test_cases = read_test_cases(test_data_dir, test_type);