feat: Handle ACIR calls in the debugger

[ggiraldez]

Description

Problem*

Resolves #4824

With the introduction of ACIR calls to allow non-inlined ACIR functions, the debugger did not have proper support for programs that had them, neither when compiling in ACIR mode, nor in the default mode which compiles the full program to Brillig.

Summary*

This PR depends on #4941 and is built on top of it.

The changes allow compiling a program with #[fold] (and other non-inline) annotated functions in Brillig mode (the default when debugging) by forcing the selection of the Brillig runtime when compiling them. For inline functions, during SSA generation the runtime is not changed in order to allow some optimizations in the stdlib to still work. For example, some constant assertions don't work unless the function is inlined.

When debugging in ACIR mode (with the --acir-mode CLI or generateAcir: true DAP options), the debugger now properly handles programs with multiple circuits and ACIR calls by:

1. keeping a stack of ACVMs and a record of the circuit being executed by each one, and
2. extending the debug location to also indicate in which circuit is the current opcode being executed

Additional Context

Documentation*

Check one:

• No documentation needed.
• Documentation included in this PR.
• [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt on default settings.

[github-actions]

Thank you for your contribution to the Noir language.

Please do not force push to this branch after the Noir team have started review of this PR. Doing so will only delay us merging your PR as we will need to start the review process from scratch.

Thanks for your understanding.

[vezenovm]

Thank you for the PR @ggiraldez! I plan to look at this soon, just have been taken away by other tasks

[vezenovm]

This feels like if could be cleaned up a little bit. We return an error here, panic inside of parse_components, and then do an ok_or that will never be hit because we only return Some(..) or panic.

[vezenovm]

Suggested change

	if parts.is_empty() || parts.len() > 2 {
	return Err(DebugLocationFromStrError::InvalidDebugLocationString(s.to_string()));
	}
	fn parse_components(parts: &Vec<&str>) -> Option<DebugLocation> {
	match parts.len() {
	1 => {
	let opcode_location = OpcodeLocation::from_str(parts[0]).ok()?;
	Some(DebugLocation { circuit_id: 0, opcode_location })
	}
	2 => {
	let circuit_id = parts[0].parse().ok()?;
	let opcode_location = OpcodeLocation::from_str(parts[1]).ok()?;
	Some(DebugLocation { circuit_id, opcode_location })
	}
	_ => unreachable!("`DebugLocation` has too many components"),
	}
	}
	parse_components(&parts)
	.ok_or(DebugLocationFromStrError::InvalidDebugLocationString(s.to_string()))
	}
	match parts.len() {
	1 => {
	let opcode_location = OpcodeLocation::from_str(parts[0]).ok()?;
	Some(DebugLocation { circuit_id: 0, opcode_location })
	}
	2 => {
	let circuit_id = parts[0].parse().ok()?;
	let opcode_location = OpcodeLocation::from_str(parts[1]).ok()?;
	Some(DebugLocation { circuit_id, opcode_location })
	}
	_ => return Err(DebugLocationFromStrError::InvalidDebugLocationString(s.to_string())),
	}

I think just this will work

[vezenovm]

This sentence isn't super clear, could you fix it up?

[vezenovm]

I'm surprised to see us use the insert index from an error. Don't we build all acir opcode addresses when we create the debugger context? Should this not be an internal debugger error?

[ggiraldez]

I see why this is confusing and definitely supports your suggestion about extracting the address mapping to a different type.

We're using the insert index because we're doing a binary search of the addresses of the first opcode of each circuit only. This is to find which circuit the address belongs to.

And in the second binary search below this, it's also necessary because we're tracking the addresses of the ACIR opcodes only. Since the program is flattened for the purposes of computing the addresses, Brillig calls effectively "inline" all the Brillig opcodes in the same address space, so there may be holes in the nested vectors of addresses as well.

[vezenovm]

Ok thank you for elaborating. Yeah I do think this would be much easier to interpret if it was extracting into a type.

[vezenovm]

It would be clearer to have this as its own type so that the address of the next opcode is simply a separate field rather than an extra sentinel element in the acir opcode list.

[ggiraldez]

The sentinel was initially added (before multiple circuits) to more easily handle the case when the last plus one address was being mapped to the opcode location. Without the sentinel, the function above would return a Brillig opcode location with the Brillig index beyond the unconstrained function's opcodes length. That was incorrect and problematic when handling the returned opcode location.

But we now introduced a bounds check, which may as well check against a dedicated sentinel field.