From 33c1ef70e7859fdee7babfb5d38191f53e73a0df Mon Sep 17 00:00:00 2001 From: guipublic <47281315+guipublic@users.noreply.github.com> Date: Thu, 7 Mar 2024 10:19:50 +0100 Subject: [PATCH 01/10] fix: iterative flattening pass (#4492) # Description ## Problem\* Resolves #4449 ## Summary\* The issue is fixed by implementing an iterative version of the flattening pass. ## 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: jfecher --- .../src/ssa/opt/flatten_cfg.rs | 543 ++++++++++-------- 1 file changed, 312 insertions(+), 231 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 943a57c1bc0..46f1e7a2765 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -199,22 +199,47 @@ struct Context<'f> { /// found, the top of this conditions stack is popped since we are no longer under that /// condition. If we are under multiple conditions (a nested if), the topmost condition is /// the most recent condition combined with all previous conditions via `And` instructions. - conditions: Vec<(BasicBlockId, ValueId)>, + condition_stack: Vec, /// Maps SSA array values with a slice type to their size. /// This is maintained by appropriate calls to the `SliceCapacityTracker` and is used by the `ValueMerger`. slice_sizes: HashMap, + + /// Stack of block arguments + /// When processing a block, we pop this stack to get its arguments + /// and at the end we push the arguments for his successor + arguments_stack: Vec>, } +#[derive(Clone)] pub(crate) struct Store { old_value: ValueId, new_value: ValueId, } -struct Branch { - condition: ValueId, +#[derive(Clone)] +struct ConditionalBranch { + // Contains the last processed block during the processing of the branch. last_block: BasicBlockId, + // The unresolved condition of the branch + old_condition: ValueId, + // The condition of the branch + condition: ValueId, + // The store values accumulated when processing the branch store_values: HashMap, + // The allocations accumulated when processing the branch + local_allocations: HashSet, +} + +struct ConditionalContext { + // Condition from the conditional statement + condition: ValueId, + // Block containing the conditional statement + entry_block: BasicBlockId, + // First block of the then branch + then_branch: ConditionalBranch, + // First block of the else branch + else_branch: Option, } fn flatten_function_cfg(function: &mut Function) { @@ -233,90 +258,117 @@ fn flatten_function_cfg(function: &mut Function) { store_values: HashMap::default(), local_allocations: HashSet::new(), branch_ends, - conditions: Vec::new(), slice_sizes: HashMap::default(), + condition_stack: Vec::new(), + arguments_stack: Vec::new(), }; context.flatten(); } impl<'f> Context<'f> { fn flatten(&mut self) { - // Start with following the terminator of the entry block since we don't - // need to flatten the entry block into itself. - self.handle_terminator(self.inserter.function.entry_block()); + // Flatten the CFG by inlining all instructions from the queued blocks + // until all blocks have been flattened. + // We follow the terminator of each block to determine which blocks to + // process next + let mut queue = vec![self.inserter.function.entry_block()]; + while let Some(block) = queue.pop() { + self.inline_block(block); + let to_process = self.handle_terminator(block, &queue); + for incoming_block in to_process { + if !queue.contains(&incoming_block) { + queue.push(incoming_block); + } + } + } } - /// Check the terminator of the given block and recursively inline any blocks reachable from - /// it. Since each block from a jmpif terminator is inlined successively, we must handle - /// instructions with side effects like constrain and store specially to preserve correctness. - /// For these instructions we must keep track of what the current condition is and modify - /// the instructions according to the module-level comment at the top of this file. Note that - /// the current condition is all the jmpif conditions required to reach the current block, - /// combined via `And` instructions. - /// - /// Returns the last block to be inlined. This is either the return block of the function or, - /// if self.conditions is not empty, the end block of the most recent condition. - fn handle_terminator(&mut self, block: BasicBlockId) -> BasicBlockId { - // As we recursively flatten inner blocks, we need to track the slice information - // for the outer block before we start recursively inlining - let outer_block_instructions = self.inserter.function.dfg[block].instructions(); - let mut capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); - for instruction in outer_block_instructions { - let results = self.inserter.function.dfg.instruction_results(*instruction); - let instruction = &self.inserter.function.dfg[*instruction]; + /// Returns the updated condition so that + /// it is 'AND-ed' with the previous condition (if any) + fn link_condition(&mut self, condition: ValueId) -> ValueId { + // Retrieve the previous condition + if let Some(context) = self.condition_stack.last() { + let previous_branch = context.else_branch.as_ref().unwrap_or(&context.then_branch); + let and = Instruction::binary(BinaryOp::And, previous_branch.condition, condition); + self.insert_instruction(and, CallStack::new()) + } else { + condition + } + } + + /// Returns the current condition + fn get_last_condition(&self) -> Option { + self.condition_stack.last().map(|context| match &context.else_branch { + Some(else_branch) => else_branch.condition, + None => context.then_branch.condition, + }) + } + + // Inline all instructions from the given block into the entry block, and track slice capacities + fn inline_block(&mut self, block: BasicBlockId) { + if self.inserter.function.entry_block() == block { + // we do not inline the entry block into itself + // for the outer block before we start inlining + let outer_block_instructions = self.inserter.function.dfg[block].instructions(); + let mut capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); + for instruction in outer_block_instructions { + let results = self.inserter.function.dfg.instruction_results(*instruction); + let instruction = &self.inserter.function.dfg[*instruction]; + capacity_tracker.collect_slice_information( + instruction, + &mut self.slice_sizes, + results.to_vec(), + ); + } + + return; + } + + let arguments = self.arguments_stack.pop().unwrap(); + self.inserter.remember_block_params(block, &arguments); + + // If this is not a separate variable, clippy gets confused and says the to_vec is + // unnecessary, when removing it actually causes an aliasing/mutability error. + let instructions = self.inserter.function.dfg[block].instructions().to_vec(); + for instruction in instructions.iter() { + let results = self.push_instruction(*instruction); + let (instruction, _) = self.inserter.map_instruction(*instruction); + let mut capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); capacity_tracker.collect_slice_information( - instruction, + &instruction, &mut self.slice_sizes, - results.to_vec(), + results, ); } + } - match self.inserter.function.dfg[block].unwrap_terminator() { + /// Returns the list of blocks that need to be processed after the given block + /// For a normal block, it would be its successor + /// For blocks related to a conditional statement, we ensure to process + /// the 'then-branch', then the 'else-branch' (if it exists), and finally the end block + fn handle_terminator( + &mut self, + block: BasicBlockId, + work_list: &[BasicBlockId], + ) -> Vec { + let terminator = self.inserter.function.dfg[block].unwrap_terminator().clone(); + match &terminator { TerminatorInstruction::JmpIf { condition, then_destination, else_destination } => { - let old_condition = *condition; - let then_block = *then_destination; - let else_block = *else_destination; - let then_condition = self.inserter.resolve(old_condition); - - let one = FieldElement::one(); - let then_branch = - self.inline_branch(block, then_block, old_condition, then_condition, one); - - let else_condition = - self.insert_instruction(Instruction::Not(then_condition), CallStack::new()); - let zero = FieldElement::zero(); - - // Make sure the else branch sees the previous values of each store - // rather than any values created in the 'then' branch. - self.undo_stores_in_then_branch(&then_branch); - - let else_branch = - self.inline_branch(block, else_block, old_condition, else_condition, zero); - - // We must remember to reset whether side effects are enabled when both branches - // end, in addition to resetting the value of old_condition since it is set to - // known to be true/false within the then/else branch respectively. - self.insert_current_side_effects_enabled(); - - // We must map back to `then_condition` here. Mapping `old_condition` to itself would - // lose any previous mappings. - self.inserter.map_value(old_condition, then_condition); - - // While there is a condition on the stack we don't compile outside the condition - // until it is popped. This ensures we inline the full then and else branches - // before continuing from the end of the conditional here where they can be merged properly. - let end = self.branch_ends[&block]; - self.inline_branch_end(end, then_branch, else_branch) + self.arguments_stack.push(vec![]); + self.if_start(condition, then_destination, else_destination, &block) } TerminatorInstruction::Jmp { destination, arguments, call_stack: _ } => { - if let Some((end_block, _)) = self.conditions.last() { - if destination == end_block { - return block; + let arguments = vecmap(arguments.clone(), |value| self.inserter.resolve(value)); + self.arguments_stack.push(arguments); + if work_list.contains(destination) { + if work_list.last() == Some(destination) { + self.else_stop(&block) + } else { + self.then_stop(&block) } + } else { + vec![*destination] } - let destination = *destination; - let arguments = vecmap(arguments.clone(), |value| self.inserter.resolve(value)); - self.inline_block(destination, &arguments) } TerminatorInstruction::Return { return_values, call_stack } => { let call_stack = call_stack.clone(); @@ -326,133 +378,133 @@ impl<'f> Context<'f> { let entry = self.inserter.function.entry_block(); self.inserter.function.dfg.set_block_terminator(entry, new_return); - block + vec![] } } } - /// Push a condition to the stack of conditions. - /// - /// This condition should be present while we're inlining each block reachable from the 'then' - /// branch of a jmpif instruction, until the branches eventually join back together. Likewise, - /// !condition should be present while we're inlining each block reachable from the 'else' - /// branch of a jmpif instruction until the join block. - fn push_condition(&mut self, start_block: BasicBlockId, condition: ValueId) { - let end_block = self.branch_ends[&start_block]; - - if let Some((_, previous_condition)) = self.conditions.last() { - let and = Instruction::binary(BinaryOp::And, *previous_condition, condition); - let new_condition = self.insert_instruction(and, CallStack::new()); - self.conditions.push((end_block, new_condition)); - } else { - self.conditions.push((end_block, condition)); + /// Process a conditional statement + fn if_start( + &mut self, + condition: &ValueId, + then_destination: &BasicBlockId, + else_destination: &BasicBlockId, + if_entry: &BasicBlockId, + ) -> Vec { + // manage conditions + let old_condition = *condition; + let then_condition = self.inserter.resolve(old_condition); + + let one = FieldElement::one(); + let old_stores = std::mem::take(&mut self.store_values); + let old_allocations = std::mem::take(&mut self.local_allocations); + let branch = ConditionalBranch { + old_condition, + condition: self.link_condition(then_condition), + store_values: old_stores, + local_allocations: old_allocations, + last_block: *then_destination, + }; + let cond_context = ConditionalContext { + condition: then_condition, + entry_block: *if_entry, + then_branch: branch, + else_branch: None, + }; + self.condition_stack.push(cond_context); + self.insert_current_side_effects_enabled(); + // Optimization: within the then branch we know the condition to be true, so replace + // any references of it within this branch with true. Likewise, do the same with false + // with the else branch. We must be careful not to replace the condition if it is a + // known constant, otherwise we can end up setting 1 = 0 or vice-versa. + if self.inserter.function.dfg.get_numeric_constant(old_condition).is_none() { + let known_value = self.inserter.function.dfg.make_constant(one, Type::bool()); + + self.inserter.map_value(old_condition, known_value); } + vec![self.branch_ends[if_entry], *else_destination, *then_destination] } - /// Insert a new instruction into the function's entry block. - /// Unlike push_instruction, this function will not map any ValueIds. - /// within the given instruction, nor will it modify self.values in any way. - fn insert_instruction(&mut self, instruction: Instruction, call_stack: CallStack) -> ValueId { - let block = self.inserter.function.entry_block(); - self.inserter - .function - .dfg - .insert_instruction_and_results(instruction, block, None, call_stack) - .first() + /// Switch context to the 'else-branch' + fn then_stop(&mut self, block: &BasicBlockId) -> Vec { + let mut cond_context = self.condition_stack.pop().unwrap(); + cond_context.then_branch.last_block = *block; + + let else_condition = + self.insert_instruction(Instruction::Not(cond_context.condition), CallStack::new()); + let else_condition = self.link_condition(else_condition); + + let zero = FieldElement::zero(); + // Make sure the else branch sees the previous values of each store + // rather than any values created in the 'then' branch. + let old_stores = std::mem::take(&mut cond_context.then_branch.store_values); + cond_context.then_branch.store_values = std::mem::take(&mut self.store_values); + self.undo_stores_in_then_branch(&cond_context.then_branch.store_values); + + let old_allocations = std::mem::take(&mut self.local_allocations); + let else_branch = ConditionalBranch { + old_condition: cond_context.then_branch.old_condition, + condition: else_condition, + store_values: old_stores, + local_allocations: old_allocations, + last_block: *block, + }; + let old_condition = else_branch.old_condition; + cond_context.then_branch.local_allocations.clear(); + cond_context.else_branch = Some(else_branch); + self.condition_stack.push(cond_context); + + self.insert_current_side_effects_enabled(); + // Optimization: within the then branch we know the condition to be true, so replace + // any references of it within this branch with true. Likewise, do the same with false + // with the else branch. We must be careful not to replace the condition if it is a + // known constant, otherwise we can end up setting 1 = 0 or vice-versa. + if self.inserter.function.dfg.get_numeric_constant(old_condition).is_none() { + let known_value = self.inserter.function.dfg.make_constant(zero, Type::bool()); + + self.inserter.map_value(old_condition, known_value); + } + assert_eq!(self.cfg.successors(*block).len(), 1); + vec![self.cfg.successors(*block).next().unwrap()] } - /// Inserts a new instruction into the function's entry block, using the given - /// control type variables to specify result types if needed. - /// Unlike push_instruction, this function will not map any ValueIds. - /// within the given instruction, nor will it modify self.values in any way. - fn insert_instruction_with_typevars( - &mut self, - instruction: Instruction, - ctrl_typevars: Option>, - ) -> InsertInstructionResult { - let block = self.inserter.function.entry_block(); - self.inserter.function.dfg.insert_instruction_and_results( - instruction, - block, - ctrl_typevars, - CallStack::new(), - ) - } + /// Process the 'exit' block of a conditional statement + fn else_stop(&mut self, block: &BasicBlockId) -> Vec { + let mut cond_context = self.condition_stack.pop().unwrap(); + if cond_context.else_branch.is_none() { + // then_stop() has not been called, this means that the conditional statement has no else branch + // so we simply do the then_stop() now + self.condition_stack.push(cond_context); + self.then_stop(block); + cond_context = self.condition_stack.pop().unwrap(); + } - /// Checks the branch condition on the top of the stack and uses it to build and insert an - /// `EnableSideEffects` instruction into the entry block. - /// - /// If the stack is empty, a "true" u1 constant is taken to be the active condition. This is - /// necessary for re-enabling side-effects when re-emerging to a branch depth of 0. - fn insert_current_side_effects_enabled(&mut self) { - let condition = match self.conditions.last() { - Some((_, cond)) => *cond, - None => { - self.inserter.function.dfg.make_constant(FieldElement::one(), Type::unsigned(1)) - } - }; - let enable_side_effects = Instruction::EnableSideEffects { condition }; - self.insert_instruction_with_typevars(enable_side_effects, None); - } + let mut else_branch = cond_context.else_branch.unwrap(); + let stores_in_branch = std::mem::replace(&mut self.store_values, else_branch.store_values); + self.local_allocations = std::mem::take(&mut else_branch.local_allocations); + else_branch.last_block = *block; + else_branch.store_values = stores_in_branch; + cond_context.else_branch = Some(else_branch); - /// Inline one branch of a jmpif instruction. - /// - /// This will continue inlining recursively until the next end block is reached where each branch - /// of the jmpif instruction is joined back into a single block. - /// - /// Within a branch of a jmpif instruction, we can assume the condition of the jmpif to be - /// always true or false, depending on which branch we're in. - /// - /// Returns the ending block / join block of this branch. - fn inline_branch( - &mut self, - jmpif_block: BasicBlockId, - destination: BasicBlockId, - old_condition: ValueId, - new_condition: ValueId, - condition_value: FieldElement, - ) -> Branch { - if destination == self.branch_ends[&jmpif_block] { - // If the branch destination is the same as the end of the branch, this must be the - // 'else' case of an if with no else - so there is no else branch. - Branch { - condition: new_condition, - // The last block here is somewhat arbitrary. It only matters that it has no Jmp - // args that will be merged by inline_branch_end. Since jmpifs don't have - // block arguments, it is safe to use the jmpif block here. - last_block: jmpif_block, - store_values: HashMap::default(), - } - } else { - self.push_condition(jmpif_block, new_condition); - self.insert_current_side_effects_enabled(); - let old_stores = std::mem::take(&mut self.store_values); - let old_allocations = std::mem::take(&mut self.local_allocations); - - // Optimization: within the then branch we know the condition to be true, so replace - // any references of it within this branch with true. Likewise, do the same with false - // with the else branch. We must be careful not to replace the condition if it is a - // known constant, otherwise we can end up setting 1 = 0 or vice-versa. - if self.inserter.function.dfg.get_numeric_constant(old_condition).is_none() { - let known_value = - self.inserter.function.dfg.make_constant(condition_value, Type::bool()); - - self.inserter.map_value(old_condition, known_value); - } + // We must remember to reset whether side effects are enabled when both branches + // end, in addition to resetting the value of old_condition since it is set to + // known to be true/false within the then/else branch respectively. + self.insert_current_side_effects_enabled(); - let final_block = self.inline_block(destination, &[]); + // We must map back to `then_condition` here. Mapping `old_condition` to itself would + // lose any previous mappings. + self.inserter + .map_value(cond_context.then_branch.old_condition, cond_context.then_branch.condition); - self.conditions.pop(); + // While there is a condition on the stack we don't compile outside the condition + // until it is popped. This ensures we inline the full then and else branches + // before continuing from the end of the conditional here where they can be merged properly. + let end = self.branch_ends[&cond_context.entry_block]; - let stores_in_branch = std::mem::replace(&mut self.store_values, old_stores); - self.local_allocations = old_allocations; + // Merge arguments and stores from the else/end branches + self.inline_branch_end(end, cond_context); - Branch { - condition: new_condition, - last_block: final_block, - store_values: stores_in_branch, - } - } + vec![self.cfg.successors(*block).next().unwrap()] } /// Inline the ending block of a branch, the point where all blocks from a jmpif instruction @@ -467,15 +519,17 @@ impl<'f> Context<'f> { fn inline_branch_end( &mut self, destination: BasicBlockId, - then_branch: Branch, - else_branch: Branch, + cond_context: ConditionalContext, ) -> BasicBlockId { assert_eq!(self.cfg.predecessors(destination).len(), 2); + let last_then = cond_context.then_branch.last_block; + let mut else_args = Vec::new(); + if cond_context.else_branch.is_some() { + let last_else = cond_context.else_branch.clone().unwrap().last_block; + else_args = self.inserter.function.dfg[last_else].terminator_arguments().to_vec(); + } - let then_args = - self.inserter.function.dfg[then_branch.last_block].terminator_arguments().to_vec(); - let else_args = - self.inserter.function.dfg[else_branch.last_block].terminator_arguments().to_vec(); + let then_args = self.inserter.function.dfg[last_then].terminator_arguments().to_vec(); let params = self.inserter.function.dfg.block_parameters(destination); assert_eq!(params.len(), then_args.len()); @@ -500,17 +554,64 @@ impl<'f> Context<'f> { // Cannot include this in the previous vecmap since it requires exclusive access to self let args = vecmap(args, |(then_arg, else_arg)| { value_merger.merge_values( - then_branch.condition, - else_branch.condition, + cond_context.then_branch.condition, + cond_context.else_branch.clone().unwrap().condition, then_arg, else_arg, ) }); - self.merge_stores(then_branch, else_branch); + self.merge_stores(cond_context.then_branch, cond_context.else_branch); + self.arguments_stack.pop(); + self.arguments_stack.pop(); + self.arguments_stack.push(args); + destination + } - // insert merge instruction - self.inline_block(destination, &args) + /// Insert a new instruction into the function's entry block. + /// Unlike push_instruction, this function will not map any ValueIds. + /// within the given instruction, nor will it modify self.values in any way. + fn insert_instruction(&mut self, instruction: Instruction, call_stack: CallStack) -> ValueId { + let block = self.inserter.function.entry_block(); + self.inserter + .function + .dfg + .insert_instruction_and_results(instruction, block, None, call_stack) + .first() + } + + /// Inserts a new instruction into the function's entry block, using the given + /// control type variables to specify result types if needed. + /// Unlike push_instruction, this function will not map any ValueIds. + /// within the given instruction, nor will it modify self.values in any way. + fn insert_instruction_with_typevars( + &mut self, + instruction: Instruction, + ctrl_typevars: Option>, + ) -> InsertInstructionResult { + let block = self.inserter.function.entry_block(); + self.inserter.function.dfg.insert_instruction_and_results( + instruction, + block, + ctrl_typevars, + CallStack::new(), + ) + } + + /// Checks the branch condition on the top of the stack and uses it to build and insert an + /// `EnableSideEffects` instruction into the entry block. + /// + /// If the stack is empty, a "true" u1 constant is taken to be the active condition. This is + /// necessary for re-enabling side-effects when re-emerging to a branch depth of 0. + fn insert_current_side_effects_enabled(&mut self) { + let condition = match self.get_last_condition() { + Some(cond) => cond, + None => { + self.inserter.function.dfg.make_constant(FieldElement::one(), Type::unsigned(1)) + } + }; + let enable_side_effects = Instruction::EnableSideEffects { condition }; + self.insert_instruction_with_typevars(enable_side_effects, None); } /// Merge any store instructions found in each branch. @@ -518,7 +619,11 @@ impl<'f> Context<'f> { /// This function relies on the 'then' branch being merged before the 'else' branch of a jmpif /// instruction. If this ordering is changed, the ordering that store values are merged within /// this function also needs to be changed to reflect that. - fn merge_stores(&mut self, then_branch: Branch, else_branch: Branch) { + fn merge_stores( + &mut self, + then_branch: ConditionalBranch, + else_branch: Option, + ) { // Address -> (then_value, else_value, value_before_the_if) let mut new_map = BTreeMap::new(); @@ -526,11 +631,13 @@ impl<'f> Context<'f> { new_map.insert(address, (store.new_value, store.old_value, store.old_value)); } - for (address, store) in else_branch.store_values { - if let Some(entry) = new_map.get_mut(&address) { - entry.1 = store.new_value; - } else { - new_map.insert(address, (store.old_value, store.new_value, store.old_value)); + if else_branch.is_some() { + for (address, store) in else_branch.clone().unwrap().store_values { + if let Some(entry) = new_map.get_mut(&address) { + entry.1 = store.new_value; + } else { + new_map.insert(address, (store.old_value, store.new_value, store.old_value)); + } } } @@ -544,8 +651,11 @@ impl<'f> Context<'f> { } let then_condition = then_branch.condition; - let else_condition = else_branch.condition; - + let else_condition = if let Some(branch) = else_branch { + branch.condition + } else { + self.inserter.function.dfg.make_constant(FieldElement::zero(), Type::bool()) + }; let block = self.inserter.function.entry_block(); let mut value_merger = @@ -607,35 +717,6 @@ impl<'f> Context<'f> { } } - /// Inline all instructions from the given destination block into the entry block. - /// Afterwards, check the block's terminator and continue inlining recursively. - /// - /// Returns the final block that was inlined. - /// - /// Expects that the `arguments` given are already translated via self.inserter.resolve. - /// If they are not, it is possible some values which no longer exist, such as block - /// parameters, will be kept in the program. - fn inline_block(&mut self, destination: BasicBlockId, arguments: &[ValueId]) -> BasicBlockId { - self.inserter.remember_block_params(destination, arguments); - - // If this is not a separate variable, clippy gets confused and says the to_vec is - // unnecessary, when removing it actually causes an aliasing/mutability error. - let instructions = self.inserter.function.dfg[destination].instructions().to_vec(); - - for instruction in instructions.iter() { - let results = self.push_instruction(*instruction); - let (instruction, _) = self.inserter.map_instruction(*instruction); - let mut capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); - capacity_tracker.collect_slice_information( - &instruction, - &mut self.slice_sizes, - results, - ); - } - - self.handle_terminator(destination) - } - /// Push the given instruction to the end of the entry block of the current function. /// /// Note that each ValueId of the instruction will be mapped via self.inserter.resolve. @@ -666,7 +747,7 @@ impl<'f> Context<'f> { instruction: Instruction, call_stack: CallStack, ) -> Instruction { - if let Some((_, condition)) = self.conditions.last().copied() { + if let Some(condition) = self.get_last_condition() { match instruction { Instruction::Constrain(lhs, rhs, message) => { // Replace constraint `lhs == rhs` with `condition * lhs == condition * rhs`. @@ -741,8 +822,8 @@ impl<'f> Context<'f> { } } - fn undo_stores_in_then_branch(&mut self, then_branch: &Branch) { - for (address, store) in &then_branch.store_values { + fn undo_stores_in_then_branch(&mut self, store_values: &HashMap) { + for (address, store) in store_values { let address = *address; let value = store.old_value; self.insert_instruction_with_typevars(Instruction::Store { address, value }, None); From 1d653704715bf9999eb6a40ed7500e752e2c73b7 Mon Sep 17 00:00:00 2001 From: Gregorio Juliana Date: Thu, 7 Mar 2024 12:34:12 +0100 Subject: [PATCH 02/10] fix: handling of gh deps in noir_wasm (#4499) # Description Github deps are directly added to the file manager, which causes them to be missing the absolute path to the source file and only include the extraction directory relative to the fm root directory. This caused the path to the file to be formatted in an unexpected way and the compiler to panic. Resolves: https://github.com/noir-lang/noir/issues/4355 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. --- compiler/wasm/src/noir/package.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/compiler/wasm/src/noir/package.ts b/compiler/wasm/src/noir/package.ts index 81178e6ae96..2856798273a 100644 --- a/compiler/wasm/src/noir/package.ts +++ b/compiler/wasm/src/noir/package.ts @@ -105,7 +105,12 @@ export class Package { handles .filter((handle) => SOURCE_EXTENSIONS.find((ext) => handle.endsWith(ext))) .map(async (file) => { - const suffix = file.replace(this.#srcPath, ''); + // Github deps are directly added to the file manager, which causes them to be missing the absolute path to the source file + // and only include the extraction directory relative to the fm root directory + // This regexp ensures we remove the "real" source path for all dependencies, providing the compiler with what it expects for each source file: + // -> for bin/contract packages + // -> for libs + const suffix = file.replace(new RegExp(`.*${this.#srcPath}`), ''); return { path: this.getType() === 'lib' ? `${alias ? alias : this.#config.package.name}${suffix}` : file, source: (await fm.readFile(file, 'utf-8')).toString(), From 2de8fbf3f21aa2e374ac75075fbed711ae502a97 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Thu, 7 Mar 2024 14:49:08 +0000 Subject: [PATCH 03/10] chore: bump bb to 0.26.3 (#4488) # Description ## Problem\* Resolves #4468 ## Summary\* Throwing this up here so we're ready for when the artifacts are finally published on GH. ## 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. --- tooling/bb_abstraction_leaks/build.rs | 2 +- tooling/noir_js_backend_barretenberg/package.json | 2 +- yarn.lock | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tooling/bb_abstraction_leaks/build.rs b/tooling/bb_abstraction_leaks/build.rs index f9effd5d991..0bd2b1c076a 100644 --- a/tooling/bb_abstraction_leaks/build.rs +++ b/tooling/bb_abstraction_leaks/build.rs @@ -10,7 +10,7 @@ use const_format::formatcp; const USERNAME: &str = "AztecProtocol"; const REPO: &str = "aztec-packages"; -const VERSION: &str = "0.24.0"; +const VERSION: &str = "0.26.3"; const TAG: &str = formatcp!("aztec-packages-v{}", VERSION); const API_URL: &str = diff --git a/tooling/noir_js_backend_barretenberg/package.json b/tooling/noir_js_backend_barretenberg/package.json index 28c3609fd14..06c034725e3 100644 --- a/tooling/noir_js_backend_barretenberg/package.json +++ b/tooling/noir_js_backend_barretenberg/package.json @@ -42,7 +42,7 @@ "lint": "NODE_NO_WARNINGS=1 eslint . --ext .ts --ignore-path ./.eslintignore --max-warnings 0" }, "dependencies": { - "@aztec/bb.js": "0.24.0", + "@aztec/bb.js": "0.26.3", "@noir-lang/types": "workspace:*", "fflate": "^0.8.0" }, diff --git a/yarn.lock b/yarn.lock index f5f3a29f08a..49485193532 100644 --- a/yarn.lock +++ b/yarn.lock @@ -221,9 +221,9 @@ __metadata: languageName: node linkType: hard -"@aztec/bb.js@npm:0.24.0": - version: 0.24.0 - resolution: "@aztec/bb.js@npm:0.24.0" +"@aztec/bb.js@npm:0.26.3": + version: 0.26.3 + resolution: "@aztec/bb.js@npm:0.26.3" dependencies: comlink: ^4.4.1 commander: ^10.0.1 @@ -231,7 +231,7 @@ __metadata: tslib: ^2.4.0 bin: bb.js: dest/node/main.js - checksum: a086dabf30084cfa526e512148b9c02f0a0770dcc19b7dca4af9a3e98612b716acc7eaac6b52c0f12d985932e866d1cb9e534ded6ac9d747f3dd021afe25de27 + checksum: 74c2b7ef5405f56472cf7c41d1c13261df07b1d5019e3ede9b63d218378e0fb73ccf5c52f1cc524505efad5799b347b497349d7c9b6fe82286014b1574f12309 languageName: node linkType: hard @@ -4396,7 +4396,7 @@ __metadata: version: 0.0.0-use.local resolution: "@noir-lang/backend_barretenberg@workspace:tooling/noir_js_backend_barretenberg" dependencies: - "@aztec/bb.js": 0.24.0 + "@aztec/bb.js": 0.26.3 "@noir-lang/types": "workspace:*" "@types/node": ^20.6.2 "@types/prettier": ^3 From 99c4d27242c27eea9237ab042d85f42de484ac72 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Thu, 7 Mar 2024 16:48:23 +0000 Subject: [PATCH 04/10] chore: pass macro processors by reference (#4501) # Description ## Problem\* Resolves ## Summary\* We're cloning unnecessarily here as we can just pass the processors by reference. ## 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: Jake Fecher --- compiler/noirc_driver/src/lib.rs | 7 ++----- .../noirc_frontend/src/hir/def_collector/dc_crate.rs | 10 +++------- compiler/noirc_frontend/src/hir/def_map/mod.rs | 12 +++--------- compiler/noirc_frontend/src/tests.rs | 2 +- 4 files changed, 9 insertions(+), 22 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 97fc66688e1..fa134f8a0dd 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -237,11 +237,8 @@ pub fn check_crate( deny_warnings: bool, disable_macros: bool, ) -> CompilationResult<()> { - let macros: Vec<&dyn MacroProcessor> = if disable_macros { - vec![] - } else { - vec![&aztec_macros::AztecMacro as &dyn MacroProcessor] - }; + let macros: &[&dyn MacroProcessor] = + if disable_macros { &[] } else { &[&aztec_macros::AztecMacro as &dyn MacroProcessor] }; let mut errors = vec![]; let diagnostics = CrateDefMap::collect_defs(crate_id, context, macros); diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 7f36af5b30e..4a6ca5c6993 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -207,7 +207,7 @@ impl DefCollector { context: &mut Context, ast: SortedModule, root_file_id: FileId, - macro_processors: Vec<&dyn MacroProcessor>, + macro_processors: &[&dyn MacroProcessor], ) -> Vec<(CompilationError, FileId)> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; let crate_id = def_map.krate; @@ -220,11 +220,7 @@ impl DefCollector { let crate_graph = &context.crate_graph[crate_id]; for dep in crate_graph.dependencies.clone() { - errors.extend(CrateDefMap::collect_defs( - dep.crate_id, - context, - macro_processors.clone(), - )); + errors.extend(CrateDefMap::collect_defs(dep.crate_id, context, macro_processors)); let dep_def_root = context.def_map(&dep.crate_id).expect("ice: def map was just created").root; @@ -257,7 +253,7 @@ impl DefCollector { context.def_maps.insert(crate_id, def_collector.def_map); // TODO(#4653): generalize this function - for macro_processor in ¯o_processors { + for macro_processor in macro_processors { macro_processor .process_unresolved_traits_impls( &crate_id, diff --git a/compiler/noirc_frontend/src/hir/def_map/mod.rs b/compiler/noirc_frontend/src/hir/def_map/mod.rs index 8721bdb6c3c..1326ffca9f7 100644 --- a/compiler/noirc_frontend/src/hir/def_map/mod.rs +++ b/compiler/noirc_frontend/src/hir/def_map/mod.rs @@ -73,7 +73,7 @@ impl CrateDefMap { pub fn collect_defs( crate_id: CrateId, context: &mut Context, - macro_processors: Vec<&dyn MacroProcessor>, + macro_processors: &[&dyn MacroProcessor], ) -> Vec<(CompilationError, FileId)> { // Check if this Crate has already been compiled // XXX: There is probably a better alternative for this. @@ -90,7 +90,7 @@ impl CrateDefMap { let (ast, parsing_errors) = context.parsed_file_results(root_file_id); let mut ast = ast.into_sorted(); - for macro_processor in ¯o_processors { + for macro_processor in macro_processors { match macro_processor.process_untyped_ast(ast.clone(), &crate_id, context) { Ok(processed_ast) => { ast = processed_ast; @@ -115,13 +115,7 @@ impl CrateDefMap { }; // Now we want to populate the CrateDefMap using the DefCollector - errors.extend(DefCollector::collect( - def_map, - context, - ast, - root_file_id, - macro_processors.clone(), - )); + errors.extend(DefCollector::collect(def_map, context, ast, root_file_id, macro_processors)); errors.extend( parsing_errors.iter().map(|e| (e.clone().into(), root_file_id)).collect::>(), diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index c661cc92eef..0c8c677d9af 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -81,7 +81,7 @@ mod test { &mut context, program.clone().into_sorted(), root_file_id, - Vec::new(), // No macro processors + &[], // No macro processors )); } (program, context, errors) From 2a555a041dbd3e40cbf768ca131f508570f4ba50 Mon Sep 17 00:00:00 2001 From: guipublic <47281315+guipublic@users.noreply.github.com> Date: Thu, 7 Mar 2024 19:37:17 +0100 Subject: [PATCH 05/10] chore: add regression test for issue 4449 (#4503) # Description ## Problem\* Related to issue #4449 ## Summary\* This PR adds a regression test which ensure PR #4492 is able to handle large CFG. ## 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. --- .../execution_success/regression_4449/Nargo.toml | 6 ++++++ .../execution_success/regression_4449/Prover.toml | 3 +++ .../execution_success/regression_4449/src/main.nr | 14 ++++++++++++++ 3 files changed, 23 insertions(+) create mode 100644 test_programs/execution_success/regression_4449/Nargo.toml create mode 100644 test_programs/execution_success/regression_4449/Prover.toml create mode 100644 test_programs/execution_success/regression_4449/src/main.nr diff --git a/test_programs/execution_success/regression_4449/Nargo.toml b/test_programs/execution_success/regression_4449/Nargo.toml new file mode 100644 index 00000000000..925420a03a8 --- /dev/null +++ b/test_programs/execution_success/regression_4449/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "regression_4449" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/execution_success/regression_4449/Prover.toml b/test_programs/execution_success/regression_4449/Prover.toml new file mode 100644 index 00000000000..81af476bcc9 --- /dev/null +++ b/test_programs/execution_success/regression_4449/Prover.toml @@ -0,0 +1,3 @@ + +x = 0xbd +result = [204, 59, 83, 197, 18, 1, 128, 43, 247, 28, 104, 225, 106, 13, 20, 187, 42, 26, 67, 150, 48, 75, 238, 168, 121, 247, 142, 160, 71, 222, 97, 188] \ No newline at end of file diff --git a/test_programs/execution_success/regression_4449/src/main.nr b/test_programs/execution_success/regression_4449/src/main.nr new file mode 100644 index 00000000000..454a93f5d1a --- /dev/null +++ b/test_programs/execution_success/regression_4449/src/main.nr @@ -0,0 +1,14 @@ +// Regression test for issue #4449 +use dep::std; + +fn main(x: u8, result: [u8; 32]) { + let x = x % 31; + let mut digest = [0; 32]; + for i in 0..70 { + let y = x + i; + let a = [y, x, 32, 0, y + 1, y - 1, y - 2, 5]; + digest = std::sha256::digest(a); + } + + assert(digest == result); +} From ae1a9d923998177516919bbba6ff4b0584fa1e9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Thu, 7 Mar 2024 14:44:34 -0500 Subject: [PATCH 06/10] feat: Track stack frames and their variables in the debugger (#4188) # Description ## Problem\* Part of #3015 We want to track call frames and observe variables local to the currently executing frame. ## Summary\* This PR adds a bit more debugging instrumentation to track when function call frames are entered and exited, which allows to determine which variables are "live" at the current point in execution. This new instrumentation also allows labeling the frames in the call stack from the names of the functions being executed. Also, it includes improvements on how source breakpoints are mapped into opcode breakpoints. Both features combined bring substantial improvements to the debugger usability. ## Additional Context ## Documentation\* Check one: - [ ] No documentation needed. - [ ] Documentation included in this PR. - [X] **[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: synthia Co-authored-by: Martin Verzilli --- compiler/noirc_errors/src/debug_info.rs | 14 ++- compiler/noirc_evaluator/src/ssa.rs | 3 +- compiler/noirc_frontend/src/debug/mod.rs | 104 ++++++++++++++-- compiler/noirc_frontend/src/hir_def/types.rs | 8 +- .../src/monomorphization/ast.rs | 5 +- .../src/monomorphization/debug.rs | 2 +- .../src/monomorphization/debug_types.rs | 54 +++++---- .../src/monomorphization/mod.rs | 4 +- compiler/noirc_printable_type/src/lib.rs | 8 +- tooling/debugger/ignored-tests.txt | 10 +- tooling/debugger/src/context.rs | 107 ++++++++++++++++- tooling/debugger/src/dap.rs | 111 ++++-------------- tooling/debugger/src/foreign_calls.rs | 46 ++++++-- tooling/debugger/src/repl.rs | 14 ++- tooling/debugger/src/source_code_printer.rs | 10 +- tooling/debugger/tests/debug.rs | 2 +- tooling/nargo/src/artifacts/debug.rs | 10 +- tooling/nargo/src/artifacts/debug_vars.rs | 96 ++++++++++----- 18 files changed, 415 insertions(+), 193 deletions(-) diff --git a/compiler/noirc_errors/src/debug_info.rs b/compiler/noirc_errors/src/debug_info.rs index 67ec851d46d..09117bdc3b7 100644 --- a/compiler/noirc_errors/src/debug_info.rs +++ b/compiler/noirc_errors/src/debug_info.rs @@ -24,6 +24,9 @@ use serde::{ #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash, PartialOrd, Ord, Deserialize, Serialize)] pub struct DebugVarId(pub u32); +#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash, PartialOrd, Ord, Deserialize, Serialize)] +pub struct DebugFnId(pub u32); + #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash, PartialOrd, Ord, Deserialize, Serialize)] pub struct DebugTypeId(pub u32); @@ -33,7 +36,14 @@ pub struct DebugVariable { pub debug_type_id: DebugTypeId, } +#[derive(Debug, Clone, Hash, Deserialize, Serialize)] +pub struct DebugFunction { + pub name: String, + pub arg_names: Vec, +} + pub type DebugVariables = BTreeMap; +pub type DebugFunctions = BTreeMap; pub type DebugTypes = BTreeMap; #[serde_as] @@ -45,6 +55,7 @@ pub struct DebugInfo { #[serde_as(as = "BTreeMap")] pub locations: BTreeMap>, pub variables: DebugVariables, + pub functions: DebugFunctions, pub types: DebugTypes, } @@ -60,9 +71,10 @@ impl DebugInfo { pub fn new( locations: BTreeMap>, variables: DebugVariables, + functions: DebugFunctions, types: DebugTypes, ) -> Self { - Self { locations, variables, types } + Self { locations, variables, functions, types } } /// Updates the locations map when the [`Circuit`][acvm::acir::circuit::Circuit] is modified. diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 0bb81efe977..56cb76adbe4 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -88,6 +88,7 @@ pub fn create_circuit( ) -> Result<(Circuit, DebugInfo, Vec, Vec, Vec), RuntimeError> { let debug_variables = program.debug_variables.clone(); let debug_types = program.debug_types.clone(); + let debug_functions = program.debug_functions.clone(); let func_sig = program.main_function_signature.clone(); let recursive = program.recursive; let mut generated_acir = optimize_into_acir( @@ -130,7 +131,7 @@ pub fn create_circuit( .map(|(index, locations)| (index, locations.into_iter().collect())) .collect(); - let mut debug_info = DebugInfo::new(locations, debug_variables, debug_types); + let mut debug_info = DebugInfo::new(locations, debug_variables, debug_functions, debug_types); // Perform any ACIR-level optimizations let (optimized_circuit, transformation_map) = acvm::compiler::optimize(circuit); diff --git a/compiler/noirc_frontend/src/debug/mod.rs b/compiler/noirc_frontend/src/debug/mod.rs index a88567fcaf9..05916502d73 100644 --- a/compiler/noirc_frontend/src/debug/mod.rs +++ b/compiler/noirc_frontend/src/debug/mod.rs @@ -4,9 +4,11 @@ use crate::{ ast::{Path, PathKind}, parser::{Item, ItemKind}, }; +use noirc_errors::debug_info::{DebugFnId, DebugFunction}; use noirc_errors::{Span, Spanned}; use std::collections::HashMap; use std::collections::VecDeque; +use std::mem::take; const MAX_MEMBER_ASSIGN_DEPTH: usize = 8; @@ -26,8 +28,12 @@ pub struct DebugInstrumenter { // all field names referenced when assigning to a member of a variable pub field_names: HashMap, + // all collected function metadata (name + argument names) + pub functions: HashMap, + next_var_id: u32, next_field_name_id: u32, + next_fn_id: u32, // last seen variable names and their IDs grouped by scope scope: Vec>, @@ -38,9 +44,11 @@ impl Default for DebugInstrumenter { Self { variables: HashMap::default(), field_names: HashMap::default(), + functions: HashMap::default(), scope: vec![], next_var_id: 0, next_field_name_id: 1, + next_fn_id: 0, } } } @@ -76,10 +84,22 @@ impl DebugInstrumenter { field_name_id } + fn insert_function(&mut self, fn_name: String, arguments: Vec) -> DebugFnId { + let fn_id = DebugFnId(self.next_fn_id); + self.next_fn_id += 1; + self.functions.insert(fn_id, DebugFunction { name: fn_name, arg_names: arguments }); + fn_id + } + fn walk_fn(&mut self, func: &mut ast::FunctionDefinition) { + let func_name = func.name.0.contents.clone(); + let func_args = + func.parameters.iter().map(|param| pattern_to_string(¶m.pattern)).collect(); + let fn_id = self.insert_function(func_name, func_args); + let enter_stmt = build_debug_call_stmt("enter", fn_id, func.span); self.scope.push(HashMap::default()); - let set_fn_params = func + let set_fn_params: Vec<_> = func .parameters .iter() .flat_map(|param| { @@ -93,10 +113,21 @@ impl DebugInstrumenter { }) .collect(); - self.walk_scope(&mut func.body.0, func.span); + let func_body = &mut func.body.0; + let mut statements = take(func_body); + + self.walk_scope(&mut statements, func.span); - // prepend fn params: - func.body.0 = [set_fn_params, func.body.0.clone()].concat(); + // walk_scope ensures that the last statement is the return value of the function + let last_stmt = statements.pop().expect("at least one statement after walk_scope"); + let exit_stmt = build_debug_call_stmt("exit", fn_id, last_stmt.span); + + // rebuild function body + func_body.push(enter_stmt); + func_body.extend(set_fn_params); + func_body.extend(statements); + func_body.push(exit_stmt); + func_body.push(last_stmt); } // Modify a vector of statements in-place, adding instrumentation for sets and drops. @@ -427,6 +458,8 @@ impl DebugInstrumenter { use dep::__debug::{{ __debug_var_assign, __debug_var_drop, + __debug_fn_enter, + __debug_fn_exit, __debug_dereference_assign, {member_assigns}, }};"# @@ -451,14 +484,32 @@ pub fn build_debug_crate_file() -> String { } #[oracle(__debug_var_drop)] - unconstrained fn __debug_var_drop_oracle(_var_id: u32) {} - unconstrained fn __debug_var_drop_inner(var_id: u32) { + unconstrained fn __debug_var_drop_oracle(_var_id: u32) {} + unconstrained fn __debug_var_drop_inner(var_id: u32) { __debug_var_drop_oracle(var_id); } - pub fn __debug_var_drop(var_id: u32) { + pub fn __debug_var_drop(var_id: u32) { __debug_var_drop_inner(var_id); } + #[oracle(__debug_fn_enter)] + unconstrained fn __debug_fn_enter_oracle(_fn_id: u32) {} + unconstrained fn __debug_fn_enter_inner(fn_id: u32) { + __debug_fn_enter_oracle(fn_id); + } + pub fn __debug_fn_enter(fn_id: u32) { + __debug_fn_enter_inner(fn_id); + } + + #[oracle(__debug_fn_exit)] + unconstrained fn __debug_fn_exit_oracle(_fn_id: u32) {} + unconstrained fn __debug_fn_exit_inner(fn_id: u32) { + __debug_fn_exit_oracle(fn_id); + } + pub fn __debug_fn_exit(fn_id: u32) { + __debug_fn_exit_inner(fn_id); + } + #[oracle(__debug_dereference_assign)] unconstrained fn __debug_dereference_assign_oracle(_var_id: u32, _value: T) {} unconstrained fn __debug_dereference_assign_inner(var_id: u32, value: T) { @@ -561,6 +612,21 @@ fn build_assign_member_stmt( ast::Statement { kind: ast::StatementKind::Semi(ast::Expression { kind, span }), span } } +fn build_debug_call_stmt(fname: &str, fn_id: DebugFnId, span: Span) -> ast::Statement { + let kind = ast::ExpressionKind::Call(Box::new(ast::CallExpression { + func: Box::new(ast::Expression { + kind: ast::ExpressionKind::Variable(ast::Path { + segments: vec![ident(&format!["__debug_fn_{fname}"], span)], + kind: PathKind::Plain, + span, + }), + span, + }), + arguments: vec![uint_expr(fn_id.0 as u128, span)], + })); + ast::Statement { kind: ast::StatementKind::Semi(ast::Expression { kind, span }), span } +} + fn pattern_vars(pattern: &ast::Pattern) -> Vec<(ast::Ident, bool)> { let mut vars = vec![]; let mut stack = VecDeque::from([(pattern, false)]); @@ -585,6 +651,30 @@ fn pattern_vars(pattern: &ast::Pattern) -> Vec<(ast::Ident, bool)> { vars } +fn pattern_to_string(pattern: &ast::Pattern) -> String { + match pattern { + ast::Pattern::Identifier(id) => id.0.contents.clone(), + ast::Pattern::Mutable(mpat, _, _) => format!("mut {}", pattern_to_string(mpat.as_ref())), + ast::Pattern::Tuple(elements, _) => format!( + "({})", + elements.iter().map(pattern_to_string).collect::>().join(", ") + ), + ast::Pattern::Struct(name, fields, _) => { + format!( + "{} {{ {} }}", + name, + fields + .iter() + .map(|(field_ident, field_pattern)| { + format!("{}: {}", &field_ident.0.contents, pattern_to_string(field_pattern)) + }) + .collect::>() + .join(", "), + ) + } + } +} + fn ident(s: &str, span: Span) -> ast::Ident { ast::Ident(Spanned::from(span, s.to_string())) } diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index b70aa43701c..13fa41733cf 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -1768,9 +1768,11 @@ impl From<&Type> for PrintableType { Type::TypeVariable(_, _) => unreachable!(), Type::NamedGeneric(..) => unreachable!(), Type::Forall(..) => unreachable!(), - Type::Function(_, _, env) => { - PrintableType::Function { env: Box::new(env.as_ref().into()) } - } + Type::Function(arguments, return_type, env) => PrintableType::Function { + arguments: arguments.iter().map(|arg| arg.into()).collect(), + return_type: Box::new(return_type.as_ref().into()), + env: Box::new(env.as_ref().into()), + }, Type::MutableReference(typ) => { PrintableType::MutableReference { typ: Box::new(typ.as_ref().into()) } } diff --git a/compiler/noirc_frontend/src/monomorphization/ast.rs b/compiler/noirc_frontend/src/monomorphization/ast.rs index e4e619d5d92..7fcf8e87792 100644 --- a/compiler/noirc_frontend/src/monomorphization/ast.rs +++ b/compiler/noirc_frontend/src/monomorphization/ast.rs @@ -1,7 +1,7 @@ use acvm::FieldElement; use iter_extended::vecmap; use noirc_errors::{ - debug_info::{DebugTypes, DebugVariables}, + debug_info::{DebugFunctions, DebugTypes, DebugVariables}, Location, }; @@ -253,6 +253,7 @@ pub struct Program { /// Indicates to a backend whether a SNARK-friendly prover should be used. pub recursive: bool, pub debug_variables: DebugVariables, + pub debug_functions: DebugFunctions, pub debug_types: DebugTypes, } @@ -266,6 +267,7 @@ impl Program { return_visibility: Visibility, recursive: bool, debug_variables: DebugVariables, + debug_functions: DebugFunctions, debug_types: DebugTypes, ) -> Program { Program { @@ -276,6 +278,7 @@ impl Program { return_visibility, recursive, debug_variables, + debug_functions, debug_types, } } diff --git a/compiler/noirc_frontend/src/monomorphization/debug.rs b/compiler/noirc_frontend/src/monomorphization/debug.rs index a8ff4399f99..cf4e0ab792e 100644 --- a/compiler/noirc_frontend/src/monomorphization/debug.rs +++ b/compiler/noirc_frontend/src/monomorphization/debug.rs @@ -76,7 +76,7 @@ impl<'interner> Monomorphizer<'interner> { let var_type = self.interner.id_type(call.arguments[DEBUG_VALUE_ARG_SLOT]); let source_var_id = source_var_id.to_u128().into(); // then update the ID used for tracking at runtime - let var_id = self.debug_type_tracker.insert_var(source_var_id, var_type); + let var_id = self.debug_type_tracker.insert_var(source_var_id, &var_type); let interned_var_id = self.intern_var_id(var_id, &call.location); arguments[DEBUG_VAR_ID_ARG_SLOT] = self.expr(interned_var_id)?; Ok(()) diff --git a/compiler/noirc_frontend/src/monomorphization/debug_types.rs b/compiler/noirc_frontend/src/monomorphization/debug_types.rs index fea073d394f..16b82d1e7b9 100644 --- a/compiler/noirc_frontend/src/monomorphization/debug_types.rs +++ b/compiler/noirc_frontend/src/monomorphization/debug_types.rs @@ -3,7 +3,8 @@ use crate::{ hir_def::types::Type, }; use noirc_errors::debug_info::{ - DebugTypeId, DebugTypes, DebugVarId, DebugVariable, DebugVariables, + DebugFnId, DebugFunction, DebugFunctions, DebugTypeId, DebugTypes, DebugVarId, DebugVariable, + DebugVariables, }; use noirc_printable_type::PrintableType; use std::collections::HashMap; @@ -30,7 +31,10 @@ pub struct DebugTypeTracker { // All instances of tracked variables variables: HashMap, - // Types of tracked variables + // Function metadata collected during instrumentation injection + functions: HashMap, + + // Types of tracked variables and functions types: HashMap, types_reverse: HashMap, @@ -43,34 +47,29 @@ impl DebugTypeTracker { DebugTypeTracker { source_variables: instrumenter.variables.clone(), source_field_names: instrumenter.field_names.clone(), + functions: instrumenter.functions.clone(), ..DebugTypeTracker::default() } } - pub fn extract_vars_and_types(&self) -> (DebugVariables, DebugTypes) { + pub fn extract_vars_and_types(&self) -> (DebugVariables, DebugFunctions, DebugTypes) { let debug_variables = self .variables .clone() .into_iter() .map(|(var_id, (source_var_id, type_id))| { - ( - var_id, - DebugVariable { - name: self.source_variables.get(&source_var_id).cloned().unwrap_or_else( - || { - unreachable!( - "failed to retrieve variable name for {source_var_id:?}" - ); - }, - ), - debug_type_id: type_id, - }, - ) + let var_name = + self.source_variables.get(&source_var_id).cloned().unwrap_or_else(|| { + unreachable!("failed to retrieve variable name for {source_var_id:?}"); + }); + (var_id, DebugVariable { name: var_name, debug_type_id: type_id }) }) .collect(); + + let debug_functions = self.functions.clone().into_iter().collect(); let debug_types = self.types.clone().into_iter().collect(); - (debug_variables, debug_types) + (debug_variables, debug_functions, debug_types) } pub fn resolve_field_index( @@ -83,19 +82,24 @@ impl DebugTypeTracker { .and_then(|field_name| get_field(cursor_type, field_name)) } - pub fn insert_var(&mut self, source_var_id: SourceVarId, var_type: Type) -> DebugVarId { - if !self.source_variables.contains_key(&source_var_id) { - unreachable!("cannot find source debug variable {source_var_id:?}"); - } - - let ptype: PrintableType = var_type.follow_bindings().into(); - let type_id = self.types_reverse.get(&ptype).copied().unwrap_or_else(|| { + fn insert_type(&mut self, the_type: &Type) -> DebugTypeId { + let ptype: PrintableType = the_type.follow_bindings().into(); + self.types_reverse.get(&ptype).copied().unwrap_or_else(|| { let type_id = DebugTypeId(self.next_type_id); self.next_type_id += 1; self.types_reverse.insert(ptype.clone(), type_id); self.types.insert(type_id, ptype); type_id - }); + }) + } + + pub fn insert_var(&mut self, source_var_id: SourceVarId, var_type: &Type) -> DebugVarId { + if !self.source_variables.contains_key(&source_var_id) { + unreachable!("cannot find source debug variable {source_var_id:?}"); + } + + let type_id = self.insert_type(var_type); + // check if we need to instantiate the var with a new type let existing_var_id = self.source_to_debug_vars.get(&source_var_id).and_then(|var_id| { let (_, existing_type_id) = self.variables.get(var_id).unwrap(); diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index ce880401d77..cfd9a61d13f 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -165,7 +165,8 @@ pub fn monomorphize_debug( let FuncMeta { return_distinctness, return_visibility, kind, .. } = monomorphizer.interner.function_meta(&main); - let (debug_variables, debug_types) = monomorphizer.debug_type_tracker.extract_vars_and_types(); + let (debug_variables, debug_functions, debug_types) = + monomorphizer.debug_type_tracker.extract_vars_and_types(); let program = Program::new( functions, function_sig, @@ -174,6 +175,7 @@ pub fn monomorphize_debug( *return_visibility, *kind == FunctionKind::Recursive, debug_variables, + debug_functions, debug_types, ); Ok(program) diff --git a/compiler/noirc_printable_type/src/lib.rs b/compiler/noirc_printable_type/src/lib.rs index 24f4f275a14..60f233cd86d 100644 --- a/compiler/noirc_printable_type/src/lib.rs +++ b/compiler/noirc_printable_type/src/lib.rs @@ -33,6 +33,8 @@ pub enum PrintableType { length: u64, }, Function { + arguments: Vec, + return_type: Box, env: Box, }, MutableReference { @@ -176,8 +178,8 @@ fn to_string(value: &PrintableValue, typ: &PrintableType) -> Option { output.push_str("false"); } } - (PrintableValue::Field(_), PrintableType::Function { .. }) => { - output.push_str("<>"); + (PrintableValue::Field(_), PrintableType::Function { arguments, return_type, .. }) => { + output.push_str(&format!("< {:?}>>", arguments, return_type,)); } (_, PrintableType::MutableReference { .. }) => { output.push_str("<>"); @@ -350,7 +352,7 @@ pub fn decode_value( PrintableValue::Struct(struct_map) } - PrintableType::Function { env } => { + PrintableType::Function { env, .. } => { let field_element = field_iterator.next().unwrap(); let func_ref = PrintableValue::Field(field_element); // we want to consume the fields from the environment, but for now they are not actually printed diff --git a/tooling/debugger/ignored-tests.txt b/tooling/debugger/ignored-tests.txt index c472e828739..231d4d897a9 100644 --- a/tooling/debugger/ignored-tests.txt +++ b/tooling/debugger/ignored-tests.txt @@ -1,21 +1,13 @@ array_dynamic_blackbox_input -array_sort -assign_ex +bigint bit_shifts_comptime -brillig_cow -brillig_nested_arrays brillig_references brillig_to_bytes_integration debug_logs double_verify_nested_proof double_verify_proof modulus -nested_array_dynamic -nested_array_in_slice -nested_arrays_from_brillig references scalar_mul signed_comparison -simple_2d_array to_bytes_integration -bigint diff --git a/tooling/debugger/src/context.rs b/tooling/debugger/src/context.rs index 515edf0bb06..a3ee89263a4 100644 --- a/tooling/debugger/src/context.rs +++ b/tooling/debugger/src/context.rs @@ -8,11 +8,14 @@ use acvm::pwg::{ }; use acvm::{BlackBoxFunctionSolver, FieldElement}; -use nargo::artifacts::debug::DebugArtifact; +use codespan_reporting::files::{Files, SimpleFile}; +use fm::FileId; +use nargo::artifacts::debug::{DebugArtifact, StackFrame}; use nargo::errors::{ExecutionError, Location}; use nargo::NargoError; -use noirc_printable_type::{PrintableType, PrintableValue}; +use noirc_driver::DebugFile; +use std::collections::BTreeMap; use std::collections::{hash_set::Iter, HashSet}; #[derive(Debug)] @@ -29,6 +32,7 @@ pub(super) struct DebugContext<'a, B: BlackBoxFunctionSolver> { foreign_call_executor: Box, debug_artifact: &'a DebugArtifact, breakpoints: HashSet, + source_to_opcodes: BTreeMap>, } impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { @@ -39,12 +43,14 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { initial_witness: WitnessMap, foreign_call_executor: Box, ) -> Self { + let source_to_opcodes = build_source_to_opcode_debug_mappings(debug_artifact); Self { acvm: ACVM::new(blackbox_solver, &circuit.opcodes, initial_witness), brillig_solver: None, foreign_call_executor, debug_artifact, breakpoints: HashSet::new(), + source_to_opcodes, } } @@ -100,10 +106,50 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { self.debug_artifact .file_map .get(&location.file) - .map(|file| file.path.starts_with("__debug/")) + .map(is_debug_file_in_debug_crate) .unwrap_or(false) } + /// Find an opcode location matching a source code location + // We apply some heuristics here, and there are four possibilities for the + // return value of this function: + // 1. the source location is not found -> None + // 2. an exact unique location is found (very rare) -> Some(opcode_location) + // 3. an exact but not unique location is found, ie. a source location may + // be mapped to multiple opcodes, and those may be disjoint, for example for + // functions called multiple times throughout the program + // -> return the first opcode in program order that matches the source location + // 4. exact location is not found, so an opcode for a nearby source location + // is returned (this again could actually be more than one opcodes) + // -> return the opcode for the next source line that is mapped + pub(super) fn find_opcode_for_source_location( + &self, + file_id: &FileId, + line: i64, + ) -> Option { + let line = line as usize; + let Some(line_to_opcodes) = self.source_to_opcodes.get(file_id) else { + return None; + }; + let found_index = match line_to_opcodes.binary_search_by(|x| x.0.cmp(&line)) { + Ok(index) => { + // move backwards to find the first opcode which matches the line + let mut index = index; + while index > 0 && line_to_opcodes[index - 1].0 == line { + index -= 1; + } + line_to_opcodes[index].1 + } + Err(index) => { + if index >= line_to_opcodes.len() { + return None; + } + line_to_opcodes[index].1 + } + }; + Some(found_index) + } + /// Returns the callstack in source code locations for the currently /// executing opcode. This can be `None` if the execution finished (and /// `get_current_opcode_location()` returns `None`) or if the opcode is not @@ -128,6 +174,9 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { &self, opcode_location: &OpcodeLocation, ) -> Vec { + // TODO: this assumes we're debugging a program (ie. the DebugArtifact + // will contain a single DebugInfo), but this assumption doesn't hold + // for contracts self.debug_artifact.debug_symbols[0] .opcode_location(opcode_location) .map(|source_locations| { @@ -467,10 +516,14 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { } } - pub(super) fn get_variables(&self) -> Vec<(&str, &PrintableValue, &PrintableType)> { + pub(super) fn get_variables(&self) -> Vec { return self.foreign_call_executor.get_variables(); } + pub(super) fn current_stack_frame(&self) -> Option { + return self.foreign_call_executor.current_stack_frame(); + } + fn breakpoint_reached(&self) -> bool { if let Some(location) = self.get_current_opcode_location() { self.breakpoints.contains(&location) @@ -526,6 +579,52 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { } } +fn is_debug_file_in_debug_crate(debug_file: &DebugFile) -> bool { + debug_file.path.starts_with("__debug/") +} + +/// Builds a map from FileId to an ordered vector of tuples with line +/// numbers and opcode locations corresponding to those line numbers +fn build_source_to_opcode_debug_mappings( + debug_artifact: &DebugArtifact, +) -> BTreeMap> { + if debug_artifact.debug_symbols.is_empty() { + return BTreeMap::new(); + } + let locations = &debug_artifact.debug_symbols[0].locations; + let simple_files: BTreeMap<_, _> = debug_artifact + .file_map + .iter() + .filter(|(_, debug_file)| !is_debug_file_in_debug_crate(debug_file)) + .map(|(file_id, debug_file)| { + ( + file_id, + SimpleFile::new(debug_file.path.to_str().unwrap(), debug_file.source.as_str()), + ) + }) + .collect(); + + let mut result: BTreeMap> = BTreeMap::new(); + locations.iter().for_each(|(opcode_location, source_locations)| { + source_locations.iter().for_each(|source_location| { + let span = source_location.span; + let file_id = source_location.file; + let Some(file) = simple_files.get(&file_id) else { + return; + }; + let Ok(line_index) = file.line_index((), span.start() as usize) else { + return; + }; + let line_number = line_index + 1; + + result.entry(file_id).or_default().push((line_number, *opcode_location)); + }); + }); + result.iter_mut().for_each(|(_, file_locations)| file_locations.sort_by_key(|x| (x.0, x.1))); + + result +} + #[cfg(test)] mod tests { use super::*; diff --git a/tooling/debugger/src/dap.rs b/tooling/debugger/src/dap.rs index 7e67a26b257..7c722ed0a61 100644 --- a/tooling/debugger/src/dap.rs +++ b/tooling/debugger/src/dap.rs @@ -5,7 +5,6 @@ use std::str::FromStr; use acvm::acir::circuit::{Circuit, OpcodeLocation}; use acvm::acir::native_types::WitnessMap; use acvm::BlackBoxFunctionSolver; -use codespan_reporting::files::{Files, SimpleFile}; use crate::context::DebugCommandResult; use crate::context::DebugContext; @@ -30,15 +29,16 @@ use nargo::artifacts::debug::DebugArtifact; use fm::FileId; use noirc_driver::CompiledProgram; +type BreakpointId = i64; + pub struct DapSession<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> { server: Server, context: DebugContext<'a, B>, debug_artifact: &'a DebugArtifact, running: bool, - source_to_opcodes: BTreeMap>, - next_breakpoint_id: i64, - instruction_breakpoints: Vec<(OpcodeLocation, i64)>, - source_breakpoints: BTreeMap>, + next_breakpoint_id: BreakpointId, + instruction_breakpoints: Vec<(OpcodeLocation, BreakpointId)>, + source_breakpoints: BTreeMap>, } enum ScopeReferences { @@ -57,8 +57,6 @@ impl From for ScopeReferences { } } -// BTreeMap - impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { pub fn new( server: Server, @@ -67,7 +65,6 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { debug_artifact: &'a DebugArtifact, initial_witness: WitnessMap, ) -> Self { - let source_to_opcodes = Self::build_source_to_opcode_debug_mappings(debug_artifact); let context = DebugContext::new( solver, circuit, @@ -79,7 +76,6 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { server, context, debug_artifact, - source_to_opcodes, running: false, next_breakpoint_id: 1, instruction_breakpoints: vec![], @@ -87,46 +83,6 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { } } - /// Builds a map from FileId to an ordered vector of tuples with line - /// numbers and opcode locations corresponding to those line numbers - fn build_source_to_opcode_debug_mappings( - debug_artifact: &'a DebugArtifact, - ) -> BTreeMap> { - if debug_artifact.debug_symbols.is_empty() { - return BTreeMap::new(); - } - let locations = &debug_artifact.debug_symbols[0].locations; - let simple_files: BTreeMap<_, _> = debug_artifact - .file_map - .iter() - .map(|(file_id, debug_file)| { - ( - file_id, - SimpleFile::new(debug_file.path.to_str().unwrap(), debug_file.source.as_str()), - ) - }) - .collect(); - - let mut result: BTreeMap> = BTreeMap::new(); - locations.iter().for_each(|(opcode_location, source_locations)| { - if source_locations.is_empty() { - return; - } - let source_location = source_locations[0]; - let span = source_location.span; - let file_id = source_location.file; - let Ok(line_index) = &simple_files[&file_id].line_index((), span.start() as usize) - else { - return; - }; - let line_number = line_index + 1; - - result.entry(file_id).or_default().push((line_number, *opcode_location)); - }); - result.iter_mut().for_each(|(_, file_locations)| file_locations.sort_by_key(|x| x.0)); - result - } - fn send_stopped_event(&mut self, reason: StoppedEventReason) -> Result<(), ServerError> { let description = format!("{:?}", &reason); self.server.send_event(Event::Stopped(StoppedEventBody { @@ -230,6 +186,8 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { } fn build_stack_trace(&self) -> Vec { + let stack_frames = self.context.get_variables(); + self.context .get_source_call_stack() .iter() @@ -239,9 +197,15 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { self.debug_artifact.location_line_number(*source_location).unwrap(); let column_number = self.debug_artifact.location_column_number(*source_location).unwrap(); + + let name = match stack_frames.get(index) { + Some(frame) => format!("{} {}", frame.function_name, index), + None => format!("frame #{index}"), + }; + StackFrame { id: index as i64, - name: format!("frame #{index}"), + name, source: Some(Source { path: self.debug_artifact.file_map[&source_location.file] .path @@ -422,7 +386,7 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { Ok(()) } - fn get_next_breakpoint_id(&mut self) -> i64 { + fn get_next_breakpoint_id(&mut self) -> BreakpointId { let id = self.next_breakpoint_id; self.next_breakpoint_id += 1; id @@ -493,36 +457,6 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { found.map(|iter| *iter.0) } - // TODO: there are four possibilities for the return value of this function: - // 1. the source location is not found -> None - // 2. an exact unique location is found -> Some(opcode_location) - // 3. an exact but not unique location is found (ie. a source location may - // be mapped to multiple opcodes, and those may be disjoint, for example for - // functions called multiple times throughout the program) - // 4. exact location is not found, so an opcode for a nearby source location - // is returned (this again could actually be more than one opcodes) - // Case 3 is not supported yet, and 4 is not correctly handled. - fn find_opcode_for_source_location( - &self, - file_id: &FileId, - line: i64, - ) -> Option { - let line = line as usize; - let Some(line_to_opcodes) = self.source_to_opcodes.get(file_id) else { - return None; - }; - let found_index = match line_to_opcodes.binary_search_by(|x| x.0.cmp(&line)) { - Ok(index) => line_to_opcodes[index].1, - Err(index) => { - if index >= line_to_opcodes.len() { - return None; - } - line_to_opcodes[index].1 - } - }; - Some(found_index) - } - fn map_source_breakpoints(&mut self, args: &SetBreakpointsArguments) -> Vec { let Some(ref source) = &args.source.path else { return vec![]; @@ -539,7 +473,8 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { .iter() .map(|breakpoint| { let line = breakpoint.line; - let Some(location) = self.find_opcode_for_source_location(&file_id, line) else { + let Some(location) = self.context.find_opcode_for_source_location(&file_id, line) + else { return Breakpoint { verified: false, message: Some(String::from( @@ -608,16 +543,20 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession<'a, R, W, B> { } fn build_local_variables(&self) -> Vec { - let mut variables: Vec<_> = self - .context - .get_variables() + let Some(current_stack_frame) = self.context.current_stack_frame() else { + return vec![]; + }; + + let mut variables = current_stack_frame + .variables .iter() .map(|(name, value, _var_type)| Variable { name: String::from(*name), value: format!("{:?}", *value), ..Variable::default() }) - .collect(); + .collect::>(); + variables.sort_by(|a, b| a.name.partial_cmp(&b.name).unwrap()); variables } diff --git a/tooling/debugger/src/foreign_calls.rs b/tooling/debugger/src/foreign_calls.rs index 68c4d3947b0..25f126ff490 100644 --- a/tooling/debugger/src/foreign_calls.rs +++ b/tooling/debugger/src/foreign_calls.rs @@ -3,17 +3,19 @@ use acvm::{ pwg::ForeignCallWaitInfo, }; use nargo::{ - artifacts::debug::{DebugArtifact, DebugVars}, + artifacts::debug::{DebugArtifact, DebugVars, StackFrame}, ops::{DefaultForeignCallExecutor, ForeignCallExecutor, NargoForeignCallResult}, }; -use noirc_errors::debug_info::DebugVarId; -use noirc_printable_type::{ForeignCallError, PrintableType, PrintableValue}; +use noirc_errors::debug_info::{DebugFnId, DebugVarId}; +use noirc_printable_type::ForeignCallError; pub(crate) enum DebugForeignCall { VarAssign, VarDrop, MemberAssign(u32), DerefAssign, + FnEnter, + FnExit, } impl DebugForeignCall { @@ -28,13 +30,16 @@ impl DebugForeignCall { "__debug_var_assign" => Some(DebugForeignCall::VarAssign), "__debug_var_drop" => Some(DebugForeignCall::VarDrop), "__debug_deref_assign" => Some(DebugForeignCall::DerefAssign), + "__debug_fn_enter" => Some(DebugForeignCall::FnEnter), + "__debug_fn_exit" => Some(DebugForeignCall::FnExit), _ => None, } } } pub trait DebugForeignCallExecutor: ForeignCallExecutor { - fn get_variables(&self) -> Vec<(&str, &PrintableValue, &PrintableType)>; + fn get_variables(&self) -> Vec; + fn current_stack_frame(&self) -> Option; } pub struct DefaultDebugForeignCallExecutor { @@ -57,23 +62,33 @@ impl DefaultDebugForeignCallExecutor { } pub fn load_artifact(&mut self, artifact: &DebugArtifact) { - artifact.debug_symbols.iter().for_each(|info| { - self.debug_vars.insert_variables(&info.variables); - self.debug_vars.insert_types(&info.types); - }); + // TODO: handle loading from the correct DebugInfo when we support + // debugging contracts + let Some(info) = artifact.debug_symbols.get(0) else { + return; + }; + self.debug_vars.insert_debug_info(info); } } impl DebugForeignCallExecutor for DefaultDebugForeignCallExecutor { - fn get_variables(&self) -> Vec<(&str, &PrintableValue, &PrintableType)> { + fn get_variables(&self) -> Vec { self.debug_vars.get_variables() } + + fn current_stack_frame(&self) -> Option { + self.debug_vars.current_stack_frame() + } } fn debug_var_id(value: &Value) -> DebugVarId { DebugVarId(value.to_u128() as u32) } +fn debug_fn_id(value: &Value) -> DebugFnId { + DebugFnId(value.to_u128() as u32) +} + impl ForeignCallExecutor for DefaultDebugForeignCallExecutor { fn execute( &mut self, @@ -136,6 +151,19 @@ impl ForeignCallExecutor for DefaultDebugForeignCallExecutor { } Ok(ForeignCallResult::default().into()) } + Some(DebugForeignCall::FnEnter) => { + let fcp_fn_id = &foreign_call.inputs[0]; + let ForeignCallParam::Single(fn_id_value) = fcp_fn_id else { + panic!("unexpected foreign call parameter in fn enter: {fcp_fn_id:?}") + }; + let fn_id = debug_fn_id(fn_id_value); + self.debug_vars.push_fn(fn_id); + Ok(ForeignCallResult::default().into()) + } + Some(DebugForeignCall::FnExit) => { + self.debug_vars.pop_fn(); + Ok(ForeignCallResult::default().into()) + } None => self.executor.execute(foreign_call), } } diff --git a/tooling/debugger/src/repl.rs b/tooling/debugger/src/repl.rs index 8441dbde9be..41dbf604f99 100644 --- a/tooling/debugger/src/repl.rs +++ b/tooling/debugger/src/repl.rs @@ -337,11 +337,13 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { } pub fn show_vars(&self) { - let vars = self.context.get_variables(); - for (var_name, value, var_type) in vars.iter() { - let printable_value = - PrintableValueDisplay::Plain((*value).clone(), (*var_type).clone()); - println!("{var_name}:{var_type:?} = {}", printable_value); + for frame in self.context.get_variables() { + println!("{}({})", frame.function_name, frame.function_params.join(", ")); + for (var_name, value, var_type) in frame.variables.iter() { + let printable_value = + PrintableValueDisplay::Plain((*value).clone(), (*var_type).clone()); + println!(" {var_name}:{var_type:?} = {}", printable_value); + } } } @@ -530,7 +532,7 @@ pub fn run( .add( "vars", command! { - "show variable values available at this point in execution", + "show variables for each function scope available at this point in execution", () => || { ref_context.borrow_mut().show_vars(); Ok(CommandStatus::Done) diff --git a/tooling/debugger/src/source_code_printer.rs b/tooling/debugger/src/source_code_printer.rs index b5ffdb12d01..e298eb8aadd 100644 --- a/tooling/debugger/src/source_code_printer.rs +++ b/tooling/debugger/src/source_code_printer.rs @@ -30,7 +30,7 @@ struct LocationPrintContext { // Given a DebugArtifact and an OpcodeLocation, prints all the source code // locations the OpcodeLocation maps to, with some surrounding context and // visual aids to highlight the location itself. -pub(crate) fn print_source_code_location(debug_artifact: &DebugArtifact, locations: &[Location]) { +pub(super) fn print_source_code_location(debug_artifact: &DebugArtifact, locations: &[Location]) { let locations = locations.iter(); for loc in locations { @@ -269,8 +269,12 @@ mod tests { let mut opcode_locations = BTreeMap::>::new(); opcode_locations.insert(OpcodeLocation::Acir(42), vec![loc]); - let debug_symbols = - vec![DebugInfo::new(opcode_locations, BTreeMap::default(), BTreeMap::default())]; + let debug_symbols = vec![DebugInfo::new( + opcode_locations, + BTreeMap::default(), + BTreeMap::default(), + BTreeMap::default(), + )]; let debug_artifact = DebugArtifact::new(debug_symbols, &fm); let location_rendered: Vec<_> = render_location(&debug_artifact, &loc).collect(); diff --git a/tooling/debugger/tests/debug.rs b/tooling/debugger/tests/debug.rs index 4cb678192b8..143ee7987f8 100644 --- a/tooling/debugger/tests/debug.rs +++ b/tooling/debugger/tests/debug.rs @@ -12,7 +12,7 @@ mod tests { let nargo_bin = cargo_bin("nargo").into_os_string().into_string().expect("Cannot parse nargo path"); - let mut dbg_session = spawn_bash(Some(10000)).expect("Could not start bash session"); + let mut dbg_session = spawn_bash(Some(15000)).expect("Could not start bash session"); // Set backend to `/dev/null` to force an error if nargo tries to speak to a backend. dbg_session diff --git a/tooling/nargo/src/artifacts/debug.rs b/tooling/nargo/src/artifacts/debug.rs index a249ecb03ad..fbdf59805c9 100644 --- a/tooling/nargo/src/artifacts/debug.rs +++ b/tooling/nargo/src/artifacts/debug.rs @@ -8,7 +8,7 @@ use std::{ ops::Range, }; -pub use super::debug_vars::DebugVars; +pub use super::debug_vars::{DebugVars, StackFrame}; use fm::{FileId, FileManager, PathString}; /// A Debug Artifact stores, for a given program, the debug info for every function @@ -231,8 +231,12 @@ mod tests { let mut opcode_locations = BTreeMap::>::new(); opcode_locations.insert(OpcodeLocation::Acir(42), vec![loc]); - let debug_symbols = - vec![DebugInfo::new(opcode_locations, BTreeMap::default(), BTreeMap::default())]; + let debug_symbols = vec![DebugInfo::new( + opcode_locations, + BTreeMap::default(), + BTreeMap::default(), + BTreeMap::default(), + )]; let debug_artifact = DebugArtifact::new(debug_symbols, &fm); let location_in_line = debug_artifact.location_in_line(loc).expect("Expected a range"); diff --git a/tooling/nargo/src/artifacts/debug_vars.rs b/tooling/nargo/src/artifacts/debug_vars.rs index 20f2637f7d6..66568bec833 100644 --- a/tooling/nargo/src/artifacts/debug_vars.rs +++ b/tooling/nargo/src/artifacts/debug_vars.rs @@ -1,54 +1,85 @@ use acvm::brillig_vm::brillig::Value; use noirc_errors::debug_info::{ - DebugTypeId, DebugTypes, DebugVarId, DebugVariable, DebugVariables, + DebugFnId, DebugFunction, DebugInfo, DebugTypeId, DebugVarId, DebugVariable, }; use noirc_printable_type::{decode_value, PrintableType, PrintableValue}; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; #[derive(Debug, Default, Clone)] pub struct DebugVars { variables: HashMap, + functions: HashMap, types: HashMap, - active: HashSet, - values: HashMap, + frames: Vec<(DebugFnId, HashMap)>, +} + +pub struct StackFrame<'a> { + pub function_name: &'a str, + pub function_params: Vec<&'a str>, + pub variables: Vec<(&'a str, &'a PrintableValue, &'a PrintableType)>, } impl DebugVars { - pub fn get_variables(&self) -> Vec<(&str, &PrintableValue, &PrintableType)> { - self.active - .iter() - .filter_map(|var_id| { - self.variables.get(var_id).and_then(|debug_var| { - let Some(value) = self.values.get(var_id) else { - return None; - }; - let Some(ptype) = self.types.get(&debug_var.debug_type_id) else { - return None; - }; - Some((debug_var.name.as_str(), value, ptype)) - }) - }) - .collect() + pub fn insert_debug_info(&mut self, info: &DebugInfo) { + self.variables.extend(info.variables.clone()); + self.types.extend(info.types.clone()); + self.functions.extend(info.functions.clone()); + } + + pub fn get_variables(&self) -> Vec { + self.frames.iter().map(|(fn_id, frame)| self.build_stack_frame(fn_id, frame)).collect() } - pub fn insert_variables(&mut self, vars: &DebugVariables) { - self.variables.extend(vars.clone()); + pub fn current_stack_frame(&self) -> Option { + self.frames.last().map(|(fn_id, frame)| self.build_stack_frame(fn_id, frame)) } - pub fn insert_types(&mut self, types: &DebugTypes) { - self.types.extend(types.clone()); + fn lookup_var(&self, var_id: DebugVarId) -> Option<(&str, &PrintableType)> { + self.variables.get(&var_id).and_then(|debug_var| { + let Some(ptype) = self.types.get(&debug_var.debug_type_id) else { + return None; + }; + Some((debug_var.name.as_str(), ptype)) + }) + } + + fn build_stack_frame<'a>( + &'a self, + fn_id: &DebugFnId, + frame: &'a HashMap, + ) -> StackFrame { + let debug_fn = &self.functions.get(fn_id).expect("failed to find function metadata"); + + let params: Vec<&str> = + debug_fn.arg_names.iter().map(|arg_name| arg_name.as_str()).collect(); + let vars: Vec<(&str, &PrintableValue, &PrintableType)> = frame + .iter() + .filter_map(|(var_id, var_value)| { + self.lookup_var(*var_id).map(|(name, typ)| (name, var_value, typ)) + }) + .collect(); + + StackFrame { + function_name: debug_fn.name.as_str(), + function_params: params, + variables: vars, + } } pub fn assign_var(&mut self, var_id: DebugVarId, values: &[Value]) { - self.active.insert(var_id); let type_id = &self.variables.get(&var_id).unwrap().debug_type_id; let ptype = self.types.get(type_id).unwrap(); - self.values.insert(var_id, decode_value(&mut values.iter().map(|v| v.to_field()), ptype)); + + self.frames + .last_mut() + .expect("unexpected empty stack frames") + .1 + .insert(var_id, decode_value(&mut values.iter().map(|v| v.to_field()), ptype)); } pub fn assign_field(&mut self, var_id: DebugVarId, indexes: Vec, values: &[Value]) { - let mut cursor: &mut PrintableValue = self - .values + let current_frame = &mut self.frames.last_mut().expect("unexpected empty stack frames").1; + let mut cursor: &mut PrintableValue = current_frame .get_mut(&var_id) .unwrap_or_else(|| panic!("value unavailable for var_id {var_id:?}")); let cursor_type_id = &self @@ -102,7 +133,6 @@ impl DebugVars { }; } *cursor = decode_value(&mut values.iter().map(|v| v.to_field()), cursor_type); - self.active.insert(var_id); } pub fn assign_deref(&mut self, _var_id: DebugVarId, _values: &[Value]) { @@ -114,6 +144,14 @@ impl DebugVars { } pub fn drop_var(&mut self, var_id: DebugVarId) { - self.active.remove(&var_id); + self.frames.last_mut().expect("unexpected empty stack frames").1.remove(&var_id); + } + + pub fn push_fn(&mut self, fn_id: DebugFnId) { + self.frames.push((fn_id, HashMap::default())); + } + + pub fn pop_fn(&mut self) { + self.frames.pop(); } } From 1510702305f8c14d49f0096395c264eb641b79d5 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Thu, 7 Mar 2024 21:05:36 +0000 Subject: [PATCH 07/10] chore: pass `import_directive` by reference (#4511) # Description ## Problem\* Resolves ## Summary\* We don't need to pass by value here so we can just pass by reference instead. ## 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. --- compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs | 2 +- compiler/noirc_frontend/src/hir/resolution/import.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 4a6ca5c6993..27b1d376f11 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -278,7 +278,7 @@ impl DefCollector { // Resolve unresolved imports collected from the crate, one by one. for collected_import in def_collector.collected_imports { - match resolve_import(crate_id, collected_import, &context.def_maps) { + match resolve_import(crate_id, &collected_import, &context.def_maps) { Ok(resolved_import) => { // Populate module namespaces according to the imports used let current_def_map = context.def_maps.get_mut(&crate_id).unwrap(); diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index e6ac33053a0..9c8418daf80 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -52,7 +52,7 @@ impl From for CustomDiagnostic { pub fn resolve_import( crate_id: CrateId, - import_directive: ImportDirective, + import_directive: &ImportDirective, def_maps: &BTreeMap, ) -> Result { let def_map = &def_maps[&crate_id]; @@ -62,10 +62,10 @@ pub fn resolve_import( let module_scope = import_directive.module_id; let resolved_namespace = - resolve_path_to_ns(&import_directive, def_map, def_maps, allow_contracts) + resolve_path_to_ns(import_directive, def_map, def_maps, allow_contracts) .map_err(|error| (error, module_scope))?; - let name = resolve_path_name(&import_directive); + let name = resolve_path_name(import_directive); Ok(ResolvedImport { name, resolved_namespace, From 169127444e8b16a8aad4acfe29ba812894fd897c Mon Sep 17 00:00:00 2001 From: jfecher Date: Thu, 7 Mar 2024 15:09:50 -0600 Subject: [PATCH 08/10] fix: Force src impl for == on slices (#4507) # Description ## Problem\* Resolves #4506 ## Summary\* `==` on slices previously tried to use the built-in impl we have but this leads to a panic when evaluating binary operators in SSA. We expect both sides of the binary to be non-tuples but this isn't true for slice values. Instead of making this work for tuples I changed the type checker to force slices to use the stdlib impl we have for `==` rather than the built in one. ## 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. --- compiler/noirc_frontend/src/hir/type_check/expr.rs | 8 +++++++- test_programs/execution_success/slices/src/main.nr | 7 +++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/hir/type_check/expr.rs b/compiler/noirc_frontend/src/hir/type_check/expr.rs index 7b854e58fca..c5287d35caf 100644 --- a/compiler/noirc_frontend/src/hir/type_check/expr.rs +++ b/compiler/noirc_frontend/src/hir/type_check/expr.rs @@ -863,7 +863,13 @@ impl<'interner> TypeChecker<'interner> { span: op.location.span, }); - self.comparator_operand_type_rules(x_type, y_type, op, span) + let (_, use_impl) = self.comparator_operand_type_rules(x_type, y_type, op, span)?; + + // If the size is not constant, we must fall back to a user-provided impl for + // equality on slices. + let size = x_size.follow_bindings(); + let use_impl = use_impl || size.evaluate_to_u64().is_none(); + Ok((Bool, use_impl)) } (String(x_size), String(y_size)) => { diff --git a/test_programs/execution_success/slices/src/main.nr b/test_programs/execution_success/slices/src/main.nr index eca42a660c4..6823bf05d96 100644 --- a/test_programs/execution_success/slices/src/main.nr +++ b/test_programs/execution_success/slices/src/main.nr @@ -50,6 +50,8 @@ fn main(x: Field, y: pub Field) { // The parameters to this function must come from witness values (inputs to main) regression_merge_slices(x, y); regression_2370(); + + regression_4506(); } // Ensure that slices of struct/tuple values work. fn regression_2083() { @@ -297,3 +299,8 @@ fn regression_2370() { let mut slice = []; slice = [1, 2, 3]; } + +fn regression_4506() { + let slice: [Field] = [1, 2, 3]; + assert(slice == slice); +} From 3163d81a357161979ea63ecb9ea09df20502ff0b Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Thu, 7 Mar 2024 21:42:48 +0000 Subject: [PATCH 09/10] chore: add `ModuleDeclaration` struct (#4512) # Description ## Problem\* Resolves ## Summary\* This is some groundwork pulled out from https://github.com/noir-lang/noir/pull/4491 We're going to want to apply visibility modifiers to module declarations so this PR creates a proper struct for these so it's easier for us to add an extra field to hold the visibility. ## 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. --- compiler/noirc_frontend/src/ast/statement.rs | 11 +++++++++++ .../src/hir/def_collector/dc_mod.rs | 18 +++++++++--------- compiler/noirc_frontend/src/parser/mod.rs | 18 +++++++++--------- compiler/noirc_frontend/src/parser/parser.rs | 10 ++++++---- 4 files changed, 35 insertions(+), 22 deletions(-) diff --git a/compiler/noirc_frontend/src/ast/statement.rs b/compiler/noirc_frontend/src/ast/statement.rs index f39b71405d3..387840b57c4 100644 --- a/compiler/noirc_frontend/src/ast/statement.rs +++ b/compiler/noirc_frontend/src/ast/statement.rs @@ -240,6 +240,17 @@ pub trait Recoverable { fn error(span: Span) -> Self; } +#[derive(Debug, PartialEq, Eq, Clone)] +pub struct ModuleDeclaration { + pub ident: Ident, +} + +impl std::fmt::Display for ModuleDeclaration { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "mod {}", self.ident) + } +} + #[derive(Debug, PartialEq, Eq, Clone)] pub struct ImportStatement { pub path: Path, diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 77224cc311c..5b2f815d636 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -9,8 +9,8 @@ use crate::{ hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait}, node_interner::{FunctionModifiers, TraitId, TypeAliasId}, parser::{SortedModule, SortedSubModule}, - FunctionDefinition, Ident, LetStatement, NoirFunction, NoirStruct, NoirTrait, NoirTraitImpl, - NoirTypeAlias, TraitImplItem, TraitItem, TypeImpl, + FunctionDefinition, Ident, LetStatement, ModuleDeclaration, NoirFunction, NoirStruct, + NoirTrait, NoirTraitImpl, NoirTypeAlias, TraitImplItem, TraitItem, TypeImpl, }; use super::{ @@ -522,15 +522,15 @@ impl<'a> ModCollector<'a> { fn parse_module_declaration( &mut self, context: &mut Context, - mod_name: &Ident, + mod_decl: &ModuleDeclaration, crate_id: CrateId, ) -> Vec<(CompilationError, FileId)> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; let child_file_id = - match find_module(&context.file_manager, self.file_id, &mod_name.0.contents) { + match find_module(&context.file_manager, self.file_id, &mod_decl.ident.0.contents) { Ok(child_file_id) => child_file_id, Err(expected_path) => { - let mod_name = mod_name.clone(); + let mod_name = mod_decl.ident.clone(); let err = DefCollectorErrorKind::UnresolvedModuleDecl { mod_name, expected_path }; errors.push((err.into(), self.file_id)); @@ -538,17 +538,17 @@ impl<'a> ModCollector<'a> { } }; - let location = Location { file: self.file_id, span: mod_name.span() }; + let location = Location { file: self.file_id, span: mod_decl.ident.span() }; if let Some(old_location) = context.visited_files.get(&child_file_id) { let error = DefCollectorErrorKind::ModuleAlreadyPartOfCrate { - mod_name: mod_name.clone(), + mod_name: mod_decl.ident.clone(), span: location.span, }; errors.push((error.into(), location.file)); let error = DefCollectorErrorKind::ModuleOriginallyDefined { - mod_name: mod_name.clone(), + mod_name: mod_decl.ident.clone(), span: old_location.span, }; errors.push((error.into(), old_location.file)); @@ -566,7 +566,7 @@ impl<'a> ModCollector<'a> { ); // Add module into def collector and get a ModuleId - match self.push_child_module(mod_name, child_file_id, true, false) { + match self.push_child_module(&mod_decl.ident, child_file_id, true, false) { Ok(child_mod_id) => { errors.extend(collect_defs( self.def_collector, diff --git a/compiler/noirc_frontend/src/parser/mod.rs b/compiler/noirc_frontend/src/parser/mod.rs index 0ff7819c00f..ea96dee8a47 100644 --- a/compiler/noirc_frontend/src/parser/mod.rs +++ b/compiler/noirc_frontend/src/parser/mod.rs @@ -14,8 +14,8 @@ mod parser; use crate::token::{Keyword, Token}; use crate::{ast::ImportStatement, Expression, NoirStruct}; use crate::{ - Ident, LetStatement, NoirFunction, NoirTrait, NoirTraitImpl, NoirTypeAlias, Recoverable, - StatementKind, TypeImpl, UseTree, + Ident, LetStatement, ModuleDeclaration, NoirFunction, NoirTrait, NoirTraitImpl, NoirTypeAlias, + Recoverable, StatementKind, TypeImpl, UseTree, }; use chumsky::prelude::*; @@ -28,7 +28,7 @@ pub use parser::parse_program; #[derive(Debug, Clone)] pub(crate) enum TopLevelStatement { Function(NoirFunction), - Module(Ident), + Module(ModuleDeclaration), Import(UseTree), Struct(NoirStruct), Trait(NoirTrait), @@ -220,7 +220,7 @@ pub struct SortedModule { pub globals: Vec, /// Module declarations like `mod foo;` - pub module_decls: Vec, + pub module_decls: Vec, /// Full submodules as in `mod foo { ... definitions ... }` pub submodules: Vec, @@ -229,7 +229,7 @@ pub struct SortedModule { impl std::fmt::Display for SortedModule { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { for decl in &self.module_decls { - writeln!(f, "mod {decl};")?; + writeln!(f, "{decl};")?; } for import in &self.imports { @@ -309,7 +309,7 @@ pub enum ItemKind { Impl(TypeImpl), TypeAlias(NoirTypeAlias), Global(LetStatement), - ModuleDecl(Ident), + ModuleDecl(ModuleDeclaration), Submodules(ParsedSubModule), } @@ -380,8 +380,8 @@ impl SortedModule { self.imports.extend(import_stmt.desugar(None)); } - fn push_module_decl(&mut self, mod_name: Ident) { - self.module_decls.push(mod_name); + fn push_module_decl(&mut self, mod_decl: ModuleDeclaration) { + self.module_decls.push(mod_decl); } fn push_submodule(&mut self, submodule: SortedSubModule) { @@ -474,7 +474,7 @@ impl std::fmt::Display for TopLevelStatement { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { TopLevelStatement::Function(fun) => fun.fmt(f), - TopLevelStatement::Module(m) => write!(f, "mod {m}"), + TopLevelStatement::Module(m) => m.fmt(f), TopLevelStatement::Import(tree) => write!(f, "use {tree}"), TopLevelStatement::Trait(t) => t.fmt(f), TopLevelStatement::TraitImpl(i) => i.fmt(f), diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index 75f4a6359bf..383a1ffafc9 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -40,9 +40,9 @@ use crate::parser::{force, ignore_then_commit, statement_recovery}; use crate::token::{Keyword, Token, TokenKind}; use crate::{ BinaryOp, BinaryOpKind, BlockExpression, Distinctness, ForLoopStatement, ForRange, - FunctionReturnType, Ident, IfExpression, InfixExpression, LValue, Literal, NoirTypeAlias, - Param, Path, Pattern, Recoverable, Statement, TraitBound, TypeImpl, UnresolvedTraitConstraint, - UnresolvedTypeExpression, UseTree, UseTreeKind, Visibility, + FunctionReturnType, Ident, IfExpression, InfixExpression, LValue, Literal, ModuleDeclaration, + NoirTypeAlias, Param, Path, Pattern, Recoverable, Statement, TraitBound, TypeImpl, + UnresolvedTraitConstraint, UnresolvedTypeExpression, UseTree, UseTreeKind, Visibility, }; use chumsky::prelude::*; @@ -370,7 +370,9 @@ fn optional_type_annotation<'a>() -> impl NoirParser + 'a { } fn module_declaration() -> impl NoirParser { - keyword(Keyword::Mod).ignore_then(ident()).map(TopLevelStatement::Module) + keyword(Keyword::Mod) + .ignore_then(ident()) + .map(|ident| TopLevelStatement::Module(ModuleDeclaration { ident })) } fn use_statement() -> impl NoirParser { From 8a5359c012579e54c2766de1074482a36ecada32 Mon Sep 17 00:00:00 2001 From: jfecher Date: Thu, 7 Mar 2024 16:01:08 -0600 Subject: [PATCH 10/10] fix: Allow type aliases in main (#4505) # Description ## Problem\* Resolves #4500 ## Summary\* This was due to this check https://github.com/noir-lang/noir/blob/master/compiler/noirc_frontend/src/hir_def/types.rs#L706-L709 which was needed since we checked if types were valid for main during name resolution - which is before we know type aliases are not cyclic. I've moved this check to type checking instead. ## 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. --- .../src/hir/resolution/errors.rs | 5 -- .../src/hir/resolution/resolver.rs | 88 +------------------ .../src/hir/type_check/errors.rs | 5 ++ .../noirc_frontend/src/hir/type_check/mod.rs | 20 ++++- .../noirc_frontend/src/hir_def/function.rs | 4 + compiler/noirc_frontend/src/hir_def/types.rs | 8 +- compiler/noirc_frontend/src/tests.rs | 9 ++ 7 files changed, 42 insertions(+), 97 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index d2fe67da38c..1049599f079 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -80,8 +80,6 @@ pub enum ResolverError { PrivateFunctionCalled { name: String, span: Span }, #[error("{name} is not visible from the current crate")] NonCrateFunctionCalled { name: String, span: Span }, - #[error("Only sized types may be used in the entry point to a program")] - InvalidTypeForEntryPoint { span: Span }, #[error("Nested slices are not supported")] NestedSlices { span: Span }, #[error("#[recursive] attribute is only allowed on entry points to a program")] @@ -309,9 +307,6 @@ impl From for Diagnostic { ResolverError::NonCrateFunctionCalled { span, name } => Diagnostic::simple_warning( format!("{name} is not visible from the current crate"), format!("{name} is only visible within its crate"), span), - ResolverError::InvalidTypeForEntryPoint { span } => Diagnostic::simple_error( - "Only sized types may be used in the entry point to a program".to_string(), - "Slices, references, or any type containing them may not be used in main or a contract function".to_string(), span), ResolverError::NestedSlices { span } => Diagnostic::simple_error( "Nested slices are not supported".into(), "Try to use a constant sized array instead".into(), diff --git a/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/compiler/noirc_frontend/src/hir/resolution/resolver.rs index 7789c06ca69..322891f0ae9 100644 --- a/compiler/noirc_frontend/src/hir/resolution/resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/resolver.rs @@ -911,10 +911,6 @@ impl<'a> Resolver<'a> { }); } - if self.is_entry_point_function(func) { - self.verify_type_valid_for_program_input(&typ); - } - let pattern = self.resolve_pattern(pattern, DefinitionKind::Local(None)); let typ = self.resolve_type_inner(typ, &mut generics); @@ -991,6 +987,7 @@ impl<'a> Resolver<'a> { return_distinctness: func.def.return_distinctness, has_body: !func.def.body.is_empty(), trait_constraints: self.resolve_trait_constraints(&func.def.where_clause), + is_entry_point: self.is_entry_point_function(func), } } @@ -2003,89 +2000,6 @@ impl<'a> Resolver<'a> { } HirLiteral::FmtStr(str, fmt_str_idents) } - - /// Only sized types are valid to be used as main's parameters or the parameters to a contract - /// function. If the given type is not sized (e.g. contains a slice or NamedGeneric type), an - /// error is issued. - fn verify_type_valid_for_program_input(&mut self, typ: &UnresolvedType) { - match &typ.typ { - UnresolvedTypeData::FieldElement - | UnresolvedTypeData::Integer(_, _) - | UnresolvedTypeData::Bool - | UnresolvedTypeData::Unit - | UnresolvedTypeData::Error => (), - - UnresolvedTypeData::MutableReference(_) - | UnresolvedTypeData::Function(_, _, _) - | UnresolvedTypeData::FormatString(_, _) - | UnresolvedTypeData::TraitAsType(..) - | UnresolvedTypeData::Unspecified => { - let span = typ.span.expect("Function parameters should always have spans"); - self.push_err(ResolverError::InvalidTypeForEntryPoint { span }); - } - - UnresolvedTypeData::Array(length, element) => { - if let Some(length) = length { - self.verify_type_expression_valid_for_program_input(length); - } else { - let span = typ.span.expect("Function parameters should always have spans"); - self.push_err(ResolverError::InvalidTypeForEntryPoint { span }); - } - self.verify_type_valid_for_program_input(element); - } - UnresolvedTypeData::Expression(expression) => { - self.verify_type_expression_valid_for_program_input(expression); - } - UnresolvedTypeData::String(length) => { - if let Some(length) = length { - self.verify_type_expression_valid_for_program_input(length); - } else { - let span = typ.span.expect("Function parameters should always have spans"); - self.push_err(ResolverError::InvalidTypeForEntryPoint { span }); - } - } - UnresolvedTypeData::Named(path, generics, _) => { - // Since the type is named, we need to resolve it to see what it actually refers to - // in order to check whether it is valid. Since resolving it may lead to a - // resolution error, we have to truncate our error count to the previous count just - // in case. This is to ensure resolution errors are not issued twice when this type - // is later resolved properly. - let error_count = self.errors.len(); - let resolved = self.resolve_named_type(path.clone(), generics.clone(), &mut vec![]); - self.errors.truncate(error_count); - - if !resolved.is_valid_for_program_input() { - let span = typ.span.expect("Function parameters should always have spans"); - self.push_err(ResolverError::InvalidTypeForEntryPoint { span }); - } - } - UnresolvedTypeData::Tuple(elements) => { - for element in elements { - self.verify_type_valid_for_program_input(element); - } - } - UnresolvedTypeData::Parenthesized(typ) => self.verify_type_valid_for_program_input(typ), - } - } - - fn verify_type_expression_valid_for_program_input(&mut self, expr: &UnresolvedTypeExpression) { - match expr { - UnresolvedTypeExpression::Constant(_, _) => (), - UnresolvedTypeExpression::Variable(path) => { - let error_count = self.errors.len(); - let resolved = self.resolve_named_type(path.clone(), vec![], &mut vec![]); - self.errors.truncate(error_count); - - if !resolved.is_valid_for_program_input() { - self.push_err(ResolverError::InvalidTypeForEntryPoint { span: path.span() }); - } - } - UnresolvedTypeExpression::BinaryOperation(lhs, _, rhs, _) => { - self.verify_type_expression_valid_for_program_input(lhs); - self.verify_type_expression_valid_for_program_input(rhs); - } - } - } } /// Gives an error if a user tries to create a mutable reference diff --git a/compiler/noirc_frontend/src/hir/type_check/errors.rs b/compiler/noirc_frontend/src/hir/type_check/errors.rs index 96d30100d8b..cba2400441f 100644 --- a/compiler/noirc_frontend/src/hir/type_check/errors.rs +++ b/compiler/noirc_frontend/src/hir/type_check/errors.rs @@ -122,6 +122,8 @@ pub enum TypeCheckError { ConstrainedReferenceToUnconstrained { span: Span }, #[error("Slices cannot be returned from an unconstrained runtime to a constrained runtime")] UnconstrainedSliceReturnToConstrained { span: Span }, + #[error("Only sized types may be used in the entry point to a program")] + InvalidTypeForEntryPoint { span: Span }, } impl TypeCheckError { @@ -284,6 +286,9 @@ impl From for Diagnostic { let msg = format!("Constraint for `{typ}: {trait_name}` is not needed, another matching impl is already in scope"); Diagnostic::simple_warning(msg, "Unnecessary trait constraint in where clause".into(), span) } + TypeCheckError::InvalidTypeForEntryPoint { span } => Diagnostic::simple_error( + "Only sized types may be used in the entry point to a program".to_string(), + "Slices, references, or any type containing them may not be used in main or a contract function".to_string(), span), } } } diff --git a/compiler/noirc_frontend/src/hir/type_check/mod.rs b/compiler/noirc_frontend/src/hir/type_check/mod.rs index 21d1c75a0f2..aab793ec867 100644 --- a/compiler/noirc_frontend/src/hir/type_check/mod.rs +++ b/compiler/noirc_frontend/src/hir/type_check/mod.rs @@ -14,7 +14,7 @@ mod stmt; pub use errors::TypeCheckError; use crate::{ - hir_def::{expr::HirExpression, stmt::HirStatement, traits::TraitConstraint}, + hir_def::{expr::HirExpression, function::Param, stmt::HirStatement, traits::TraitConstraint}, node_interner::{ExprId, FuncId, GlobalId, NodeInterner}, Type, }; @@ -74,6 +74,7 @@ pub fn type_check_func(interner: &mut NodeInterner, func_id: FuncId) -> Vec Vec, + func_id: FuncId, + param: &Param, + errors: &mut Vec, +) { + let meta = type_checker.interner.function_meta(&func_id); + if meta.is_entry_point && !param.1.is_valid_for_program_input() { + let span = param.0.span(); + errors.push(TypeCheckError::InvalidTypeForEntryPoint { span }); + } +} + fn function_info(interner: &NodeInterner, function_body_id: &ExprId) -> (noirc_errors::Span, bool) { let (expr_span, empty_function) = if let HirExpression::Block(block) = interner.expression(function_body_id) { @@ -329,6 +346,7 @@ mod test { trait_impl: None, return_type: FunctionReturnType::Default(Span::default()), trait_constraints: Vec::new(), + is_entry_point: true, }; interner.push_fn_meta(func_meta, func_id); diff --git a/compiler/noirc_frontend/src/hir_def/function.rs b/compiler/noirc_frontend/src/hir_def/function.rs index d3ab2a9393b..82bbe1aa5b6 100644 --- a/compiler/noirc_frontend/src/hir_def/function.rs +++ b/compiler/noirc_frontend/src/hir_def/function.rs @@ -112,6 +112,10 @@ pub struct FuncMeta { /// The trait impl this function belongs to, if any pub trait_impl: Option, + + /// True if this function is an entry point to the program. + /// For non-contracts, this means the function is `main`. + pub is_entry_point: bool, } impl FuncMeta { diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 13fa41733cf..5ab036eef5b 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -703,10 +703,10 @@ impl Type { | Type::TraitAsType(..) | Type::NotConstant => false, - // This function is called during name resolution before we've verified aliases - // are not cyclic. As a result, it wouldn't be safe to check this alias' definition - // to see if the aliased type is valid. - Type::Alias(..) => false, + Type::Alias(alias, generics) => { + let alias = alias.borrow(); + alias.get_type(generics).is_valid_for_program_input() + } Type::Array(length, element) => { length.is_valid_for_program_input() && element.is_valid_for_program_input() diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 0c8c677d9af..9be6252b10a 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1206,4 +1206,13 @@ fn lambda$f1(mut env$l1: (Field)) -> Field { "#; assert_eq!(get_program_errors(src).len(), 1); } + + #[test] + fn type_aliases_in_entry_point() { + let src = r#" + type Foo = u8; + fn main(_x: Foo) {} + "#; + assert_eq!(get_program_errors(src).len(), 0); + } }