-
Notifications
You must be signed in to change notification settings - Fork 171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Handle ACIR calls in the debugger #5051
base: master
Are you sure you want to change the base?
feat: Handle ACIR calls in the debugger #5051
Conversation
Returning serialized opcode locations is invalid according to the DAP specification and confuses VS.Code.
Make the code consistent with the DAP and enables to start implementing debugging for multiple circuits.
in addition to ACIR and Brillig indices.
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. |
Thank you for the PR @ggiraldez! I plan to look at this soon, just have been taken away by other tasks |
fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
let parts: Vec<_> = s.split(':').collect(); | ||
|
||
if parts.is_empty() || parts.len() > 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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())) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
// Brillig opcodes from all circuits are laid out. | ||
// The first element of the outer vector will contain the addresses at which | ||
// all ACIR opcodes of the first circuit are. The second element will | ||
// contain the second circuit and so on. Brillig functions should are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence isn't super clear, could you fix it up?
.binary_search_by(|addresses| addresses[0].cmp(&address)) | ||
{ | ||
Ok(found_index) => found_index, | ||
Err(insert_index) => insert_index - 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thank you for elaborating. Yeah I do think this would be much easier to interpret if it was extracting into a type.
// At the end of each vector (both outer and inner) there will be an extra | ||
// sentinel element with the address of the next opcode. This is only to | ||
// make bounds checking and working with binary search easier. | ||
acir_opcode_addresses: Vec<Vec<usize>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
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 orgenerateAcir: true
DAP options), the debugger now properly handles programs with multiple circuits and ACIR calls by:Additional Context
Documentation*
Check one:
PR Checklist*
cargo fmt
on default settings.