From c5f482f5aa35e0f4cf96b14e4bd75f2f3e29a2e2 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 9 Jun 2025 17:49:43 -0300 Subject: [PATCH 1/3] fix: handle unconditional break during SSA codegen in all cases --- compiler/noirc_evaluator/src/errors.rs | 5 + .../src/ssa/function_builder/mod.rs | 22 ---- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 117 ++++++++++-------- .../unconditional_break/src/main.nr | 16 --- .../unconditional_break/Nargo.toml | 0 .../unconditional_break/src/main.nr | 95 ++++++++++++++ .../execute__tests__expanded.snap | 18 --- ..._tests__force_brillig_false_inliner_0.snap | 26 ---- .../execute__tests__expanded.snap | 93 ++++++++++++++ 9 files changed, 261 insertions(+), 131 deletions(-) delete mode 100644 test_programs/compile_success_empty/unconditional_break/src/main.nr rename test_programs/{compile_success_empty => compile_success_no_bug}/unconditional_break/Nargo.toml (100%) create mode 100644 test_programs/compile_success_no_bug/unconditional_break/src/main.nr delete mode 100644 tooling/nargo_cli/tests/snapshots/compile_success_empty/unconditional_break/execute__tests__expanded.snap delete mode 100644 tooling/nargo_cli/tests/snapshots/compile_success_empty/unconditional_break/execute__tests__force_brillig_false_inliner_0.snap create mode 100644 tooling/nargo_cli/tests/snapshots/compile_success_no_bug/unconditional_break/execute__tests__expanded.snap diff --git a/compiler/noirc_evaluator/src/errors.rs b/compiler/noirc_evaluator/src/errors.rs index 259d5908995..49e9d4f0b06 100644 --- a/compiler/noirc_evaluator/src/errors.rs +++ b/compiler/noirc_evaluator/src/errors.rs @@ -74,6 +74,10 @@ pub enum RuntimeError { "Cannot return a function from an if or match expression, or assignment within these expressions" )] ReturnedFunctionFromDynamicIf { call_stack: CallStack }, + /// This case is not an error. It's used during codegen to prevent inserting instructions after + /// code for a break or continue is generated. + #[error("Break or continue")] + BreakOrContinue { call_stack: CallStack }, } #[derive(Debug, Clone, Serialize, Deserialize, Hash)] @@ -186,6 +190,7 @@ impl RuntimeError { | RuntimeError::UnknownReference { call_stack } => call_stack, RuntimeError::ReturnedReferenceFromDynamicIf { call_stack } => call_stack, RuntimeError::ReturnedFunctionFromDynamicIf { call_stack } => call_stack, + RuntimeError::BreakOrContinue { call_stack } => call_stack, } } } diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index cc1f620aa15..9e70acf6622 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -3,7 +3,6 @@ pub mod data_bus; use std::{borrow::Cow, collections::BTreeMap, sync::Arc}; use acvm::{FieldElement, acir::circuit::ErrorSelector}; -use fxhash::FxHashSet as HashSet; use noirc_errors::{ Location, call_stack::{CallStack, CallStackId}, @@ -42,8 +41,6 @@ use super::{ pub struct FunctionBuilder { pub current_function: Function, current_block: BasicBlockId, - current_block_closed: bool, - closed_blocked: HashSet<(FunctionId, BasicBlockId)>, finished_functions: Vec, call_stack: CallStackId, error_types: BTreeMap, @@ -65,8 +62,6 @@ impl FunctionBuilder { let new_function = Function::new(function_name, function_id); Self { current_block: new_function.entry_block(), - current_block_closed: false, - closed_blocked: HashSet::default(), current_function: new_function, finished_functions: Vec::new(), call_stack: CallStackId::root(), @@ -207,17 +202,6 @@ impl FunctionBuilder { self.current_function.dfg.make_block() } - /// Prevents any further instructions from being added to the current block. - /// That is, calls to add instructions can be called, but they will have no effect. - pub fn close_block(&mut self) { - self.closed_blocked.insert((self.current_function.id(), self.current_block)); - self.current_block_closed = true; - } - - pub fn current_block_is_closed(&self) -> bool { - self.current_block_closed - } - /// Adds a parameter with the given type to the given block. /// Returns the newly-added parameter. pub fn add_block_parameter(&mut self, block: BasicBlockId, typ: Type) -> ValueId { @@ -235,10 +219,6 @@ impl FunctionBuilder { instruction: Instruction, ctrl_typevars: Option>, ) -> InsertInstructionResult { - if self.current_block_closed { - return InsertInstructionResult::InstructionRemoved; - } - let block = self.current_block(); if self.simplify { @@ -263,8 +243,6 @@ impl FunctionBuilder { /// instructions into a new function, call new_function instead. pub fn switch_to_block(&mut self, block: BasicBlockId) { self.current_block = block; - self.current_block_closed = - self.closed_blocked.contains(&(self.current_function.id(), block)); } /// Returns the block currently being inserted into diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index c786bba3a4b..b1bf8dd1832 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -4,6 +4,7 @@ mod tests; mod value; use acvm::AcirField; +use noirc_errors::call_stack::CallStack; use noirc_frontend::hir_def::expr::Constructor; use noirc_frontend::token::FmtStrFragment; pub use program::Ssa; @@ -169,8 +170,8 @@ impl FunctionContext<'_> { } Expression::Assign(assign) => self.codegen_assign(assign), Expression::Semi(semi) => self.codegen_semi(semi), - Expression::Break => Ok(self.codegen_break()), - Expression::Continue => Ok(self.codegen_continue()), + Expression::Break => self.codegen_break(), + Expression::Continue => self.codegen_continue(), Expression::Clone(expr) => self.codegen_clone(expr), Expression::Drop(expr) => self.codegen_drop(expr), } @@ -336,12 +337,6 @@ impl FunctionContext<'_> { let mut result = Self::unit_value(); for expr in block { result = self.codegen_expression(expr)?; - - // A break or continue might happen in a block, in which case we must - // not codegen any further expressions. - if self.builder.current_block_is_closed() { - break; - } } Ok(result) } @@ -583,13 +578,11 @@ impl FunctionContext<'_> { self.builder.switch_to_block(loop_body); self.define(for_expr.index_variable, loop_index.into()); - self.codegen_expression(&for_expr.block)?; - - if !self.builder.current_block_is_closed() { - // No need to jump if the current block is already closed - let new_loop_index = self.make_offset(loop_index, 1); - self.builder.terminate_with_jmp(loop_entry, vec![new_loop_index]); - } + let result = self.codegen_expression(&for_expr.block); + self.codegen_unless_break_or_continue(result, |this, _| { + let new_loop_index = this.make_offset(loop_index, 1); + this.builder.terminate_with_jmp(loop_entry, vec![new_loop_index]); + })?; // Finish by switching back to the end of the loop self.builder.switch_to_block(loop_end); @@ -620,8 +613,10 @@ impl FunctionContext<'_> { // Compile the loop body self.builder.switch_to_block(loop_body); - self.codegen_expression(block)?; - self.builder.terminate_with_jmp(loop_body, vec![]); + let result = self.codegen_expression(block); + self.codegen_unless_break_or_continue(result, |this, _| { + this.builder.terminate_with_jmp(loop_body, vec![]); + })?; // Finish by switching to the end of the loop self.builder.switch_to_block(loop_end); @@ -661,8 +656,10 @@ impl FunctionContext<'_> { // Codegen the body self.builder.switch_to_block(while_body); - self.codegen_expression(&while_.body)?; - self.builder.terminate_with_jmp(while_entry, vec![]); + let result = self.codegen_expression(&while_.body); + self.codegen_unless_break_or_continue(result, |this, _| { + this.builder.terminate_with_jmp(while_entry, vec![]); + })?; // Finish by switching to the end of the while self.builder.switch_to_block(while_end); @@ -710,19 +707,24 @@ impl FunctionContext<'_> { self.builder.terminate_with_jmpif(condition, then_block, else_block); self.builder.switch_to_block(then_block); - let then_value = self.codegen_expression(&if_expr.consequence)?; + let then_result = self.codegen_expression(&if_expr.consequence); let mut result = Self::unit_value(); if let Some(alternative) = &if_expr.alternative { let end_block = self.builder.insert_block(); - let then_values = then_value.into_value_list(self); - self.builder.terminate_with_jmp(end_block, then_values); + + self.codegen_unless_break_or_continue(then_result, |this, then_value| { + let then_values = then_value.into_value_list(this); + this.builder.terminate_with_jmp(end_block, then_values); + })?; self.builder.switch_to_block(else_block); - let else_value = self.codegen_expression(alternative)?; - let else_values = else_value.into_value_list(self); - self.builder.terminate_with_jmp(end_block, else_values); + let else_result = self.codegen_expression(alternative); + self.codegen_unless_break_or_continue(else_result, |this, else_value| { + let else_values = else_value.into_value_list(this); + this.builder.terminate_with_jmp(end_block, else_values); + })?; // Create block arguments for the end block as needed to branch to // with our then and else value. @@ -805,13 +807,16 @@ impl FunctionContext<'_> { self.builder.switch_to_block(case_block); self.bind_case_arguments(variable.clone(), case); - let results = self.codegen_expression(&case.branch)?.into_value_list(self); + let results = self.codegen_expression(&case.branch); + self.codegen_unless_break_or_continue(results, |this, results| { + let results = results.into_value_list(this); - // Each branch will jump to a different end block for now. We have to merge them all - // later since SSA doesn't support more than two blocks jumping to the same end block. - let local_end_block = make_end_block(self); - self.builder.terminate_with_jmp(local_end_block.0, results); - blocks_to_merge.push(local_end_block); + // Each branch will jump to a different end block for now. We have to merge them all + // later since SSA doesn't support more than two blocks jumping to the same end block. + let local_end_block = make_end_block(this); + this.builder.terminate_with_jmp(local_end_block.0, results); + blocks_to_merge.push(local_end_block); + })?; self.builder.switch_to_block(else_block); } @@ -820,15 +825,21 @@ impl FunctionContext<'_> { blocks_to_merge.push((last_local_end_block, last_results)); if let Some(branch) = &match_expr.default_case { - let results = self.codegen_expression(branch)?.into_value_list(self); - self.builder.terminate_with_jmp(last_local_end_block, results); + let results = self.codegen_expression(branch); + self.codegen_unless_break_or_continue(results, |this, results| { + let results = results.into_value_list(this); + this.builder.terminate_with_jmp(last_local_end_block, results); + })?; } else { // If there is no default case, assume we saved the last case from the // last_case optimization above let case = match_expr.cases.last().unwrap(); self.bind_case_arguments(variable, case); - let results = self.codegen_expression(&case.branch)?.into_value_list(self); - self.builder.terminate_with_jmp(last_local_end_block, results); + let results = self.codegen_expression(&case.branch); + self.codegen_unless_break_or_continue(results, |this, results| { + let results = results.into_value_list(this); + this.builder.terminate_with_jmp(last_local_end_block, results); + })?; } // Merge blocks as last-in first-out: @@ -1098,13 +1109,9 @@ impl FunctionContext<'_> { // Evaluate the rhs first - when we load the expression in the lvalue we want that // to reflect any mutations from evaluating the rhs. let rhs = self.codegen_expression(&assign.expression)?; + let lhs = self.extract_current_value(&assign.lvalue)?; - // Can't assign to a variable if the expression had an unconditional break in it - if !self.builder.current_block_is_closed() { - let lhs = self.extract_current_value(&assign.lvalue)?; - - self.assign_new_value(lhs, rhs); - } + self.assign_new_value(lhs, rhs); Ok(Self::unit_value()) } @@ -1114,16 +1121,14 @@ impl FunctionContext<'_> { Ok(Self::unit_value()) } - fn codegen_break(&mut self) -> Values { + fn codegen_break(&mut self) -> Result { let loop_end = self.current_loop().loop_end; self.builder.terminate_with_jmp(loop_end, Vec::new()); - self.builder.close_block(); - - Self::unit_value() + Err(RuntimeError::BreakOrContinue { call_stack: CallStack::default() }) } - fn codegen_continue(&mut self) -> Values { + fn codegen_continue(&mut self) -> Result { let loop_ = self.current_loop(); // Must remember to increment i before jumping @@ -1134,9 +1139,7 @@ impl FunctionContext<'_> { self.builder.terminate_with_jmp(loop_.loop_entry, vec![]); } - self.builder.close_block(); - - Self::unit_value() + Err(RuntimeError::BreakOrContinue { call_stack: CallStack::default() }) } /// Evaluate the given expression, increment the reference count of each array within, @@ -1160,4 +1163,20 @@ impl FunctionContext<'_> { }); Ok(Self::unit_value()) } + + #[must_use] + fn codegen_unless_break_or_continue( + &mut self, + result: Result, + f: F, + ) -> Result<(), RuntimeError> + where + F: FnOnce(&mut Self, T), + { + match result { + Ok(value) => Ok(f(self, value)), + Err(RuntimeError::BreakOrContinue { .. }) => Ok(()), + Err(err) => Err(err), + } + } } diff --git a/test_programs/compile_success_empty/unconditional_break/src/main.nr b/test_programs/compile_success_empty/unconditional_break/src/main.nr deleted file mode 100644 index 29aaac736d4..00000000000 --- a/test_programs/compile_success_empty/unconditional_break/src/main.nr +++ /dev/null @@ -1,16 +0,0 @@ -fn main() { - // Safety: - let x = unsafe { func_1() }; - assert(x); -} - -unconstrained fn func_1() -> bool { - let mut x = true; - loop { - if true { - break; - } - x = false; - } - x -} diff --git a/test_programs/compile_success_empty/unconditional_break/Nargo.toml b/test_programs/compile_success_no_bug/unconditional_break/Nargo.toml similarity index 100% rename from test_programs/compile_success_empty/unconditional_break/Nargo.toml rename to test_programs/compile_success_no_bug/unconditional_break/Nargo.toml diff --git a/test_programs/compile_success_no_bug/unconditional_break/src/main.nr b/test_programs/compile_success_no_bug/unconditional_break/src/main.nr new file mode 100644 index 00000000000..4d14587281c --- /dev/null +++ b/test_programs/compile_success_no_bug/unconditional_break/src/main.nr @@ -0,0 +1,95 @@ +fn main() { + // Safety: + let x = unsafe { break_in_if_body() }; + assert(x); + + // Safety: + unsafe { + break_in_unary(); + break_in_infix_lhs(); + break_in_infix_rhs(); + break_in_array_element(); + break_in_if_condition(); + break_in_while_condition(); + }; +} + +unconstrained fn break_in_if_body() -> bool { + let mut x = true; + loop { + if true { + break; + } + x = false; + } + x +} + +unconstrained fn break_in_unary() { + let mut a = 0; + loop { + a = -{ + break; + a + }; + } +} + +unconstrained fn break_in_infix_lhs() { + let mut a = 0; + loop { + a = { + break; + 0 + } + - 0; + } +} + +unconstrained fn break_in_infix_rhs() { + let mut a = 0; + loop { + a = 0 + + { + break; + 0 + }; + } +} + +unconstrained fn break_in_array_element() { + loop { + let _ = [ + 1, { + break; + 2 + }, 3, + ]; + } +} + +unconstrained fn break_in_if_condition() { + loop { + if { + break; + true + } { + let _ = 10; + } + } +} + +unconstrained fn break_in_while_condition() { + loop { + while ( + { + break; + true + } + ) { + let _ = 10; + } + + break; + } +} diff --git a/tooling/nargo_cli/tests/snapshots/compile_success_empty/unconditional_break/execute__tests__expanded.snap b/tooling/nargo_cli/tests/snapshots/compile_success_empty/unconditional_break/execute__tests__expanded.snap deleted file mode 100644 index d647521eb64..00000000000 --- a/tooling/nargo_cli/tests/snapshots/compile_success_empty/unconditional_break/execute__tests__expanded.snap +++ /dev/null @@ -1,18 +0,0 @@ ---- -source: tooling/nargo_cli/tests/execute.rs -expression: expanded_code ---- -fn main() { - // Safety: comment added by `nargo expand` - let x: bool = unsafe { func_1() }; - assert(x); -} - -unconstrained fn func_1() -> bool { - let mut x: bool = true; - loop { - if true { break; }; - x = false; - } - x -} diff --git a/tooling/nargo_cli/tests/snapshots/compile_success_empty/unconditional_break/execute__tests__force_brillig_false_inliner_0.snap b/tooling/nargo_cli/tests/snapshots/compile_success_empty/unconditional_break/execute__tests__force_brillig_false_inliner_0.snap deleted file mode 100644 index 7de940827c8..00000000000 --- a/tooling/nargo_cli/tests/snapshots/compile_success_empty/unconditional_break/execute__tests__force_brillig_false_inliner_0.snap +++ /dev/null @@ -1,26 +0,0 @@ ---- -source: tooling/nargo_cli/tests/execute.rs -expression: artifact ---- -{ - "noir_version": "[noir_version]", - "hash": "[hash]", - "abi": { - "parameters": [], - "return_type": null, - "error_types": {} - }, - "bytecode": [ - "func 0", - "current witness index : _0", - "private parameters indices : []", - "public parameters indices : []", - "return value indices : []" - ], - "debug_symbols": "XY5BCsQwCEXv4rqLWfcqw1BsaosgJtikMITefWyYQOlK/3/6tcJCc9km1jXuML4rzMYivE0SA2aO6m49B+hyykbkFty4byU00gyjFpEBDpTShvaE2mpGc/oagHTx6oErC13d+XGBge158UBjnIX+ci0abjR/Uyf942Qx0FKMrqTGPPsH", - "file_map": {}, - "names": [ - "main" - ], - "brillig_names": [] -} diff --git a/tooling/nargo_cli/tests/snapshots/compile_success_no_bug/unconditional_break/execute__tests__expanded.snap b/tooling/nargo_cli/tests/snapshots/compile_success_no_bug/unconditional_break/execute__tests__expanded.snap new file mode 100644 index 00000000000..b068fb147f2 --- /dev/null +++ b/tooling/nargo_cli/tests/snapshots/compile_success_no_bug/unconditional_break/execute__tests__expanded.snap @@ -0,0 +1,93 @@ +--- +source: tooling/nargo_cli/tests/execute.rs +expression: expanded_code +--- +fn main() { + // Safety: comment added by `nargo expand` + let x: bool = unsafe { break_in_if_body() }; + assert(x); + // Safety: comment added by `nargo expand` + unsafe { + break_in_unary(); + break_in_infix_lhs(); + break_in_infix_rhs(); + break_in_array_element(); + break_in_if_condition(); + break_in_while_condition(); + }; +} + +unconstrained fn break_in_if_body() -> bool { + let mut x: bool = true; + loop { + if true { break; }; + x = false; + } + x +} + +unconstrained fn break_in_unary() { + let mut a: Field = 0; + loop { + a = -{ + break; + a + }; + } +} + +unconstrained fn break_in_infix_lhs() { + let mut a: Field = 0; + loop { + a = { + break; + 0 + } + - 0; + } +} + +unconstrained fn break_in_infix_rhs() { + let mut a: Field = 0; + loop { + a = 0 + + { + break; + 0 + }; + } +} + +unconstrained fn break_in_array_element() { + loop { + let _: [Field; 3] = [ + 1, { + break; + 2 + }, 3, + ]; + } +} + +unconstrained fn break_in_if_condition() { + loop { + if { + break; + true + } { + let _: Field = 10; + } + } +} + +unconstrained fn break_in_while_condition() { + loop { + while { + break; + true + } { + let _: Field = 10; + } + break; + } +} From c424d0a41dde0109caa6f0d3b80cc0b2bb93d59f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 9 Jun 2025 17:51:57 -0300 Subject: [PATCH 2/3] Fix comment --- compiler/noirc_evaluator/src/errors.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/errors.rs b/compiler/noirc_evaluator/src/errors.rs index 49e9d4f0b06..3fa7d8ed5e3 100644 --- a/compiler/noirc_evaluator/src/errors.rs +++ b/compiler/noirc_evaluator/src/errors.rs @@ -75,7 +75,7 @@ pub enum RuntimeError { )] ReturnedFunctionFromDynamicIf { call_stack: CallStack }, /// This case is not an error. It's used during codegen to prevent inserting instructions after - /// code for a break or continue is generated. + /// code when a break or continue is generated. #[error("Break or continue")] BreakOrContinue { call_stack: CallStack }, } From f7da55d36e9939f9a846aa383126549185fa25fb Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 9 Jun 2025 18:01:40 -0300 Subject: [PATCH 3/3] clippy --- compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index b1bf8dd1832..c361803ba98 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -1164,7 +1164,7 @@ impl FunctionContext<'_> { Ok(Self::unit_value()) } - #[must_use] + #[must_use = "do not forget to add `?` at the end of this function call"] fn codegen_unless_break_or_continue( &mut self, result: Result, @@ -1174,7 +1174,10 @@ impl FunctionContext<'_> { F: FnOnce(&mut Self, T), { match result { - Ok(value) => Ok(f(self, value)), + Ok(value) => { + f(self, value); + Ok(()) + } Err(RuntimeError::BreakOrContinue { .. }) => Ok(()), Err(err) => Err(err), }