-
Notifications
You must be signed in to change notification settings - Fork 196
[cairo1] Fix system builtin as implicit #2207
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
base: main
Are you sure you want to change the base?
Conversation
|
Benchmark Results for unmodified programs 🚀
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2207 +/- ##
==========================================
+ Coverage 96.68% 96.69% +0.01%
==========================================
Files 104 104
Lines 44027 44023 -4
==========================================
+ Hits 42566 42567 +1
+ Misses 1461 1456 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Nice work! Looks good
fn got_implicit_builtin(param_types: &[ConcreteTypeId], builtin_name: &str) -> bool { | ||
param_types | ||
.iter() | ||
.any(|ty| ty.debug_name.as_ref().is_some_and(|n| n == builtin_name)) | ||
} |
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 would use BuiltinName
here instead of a &str
, just to be safe. You can convert a ty.debug_name
with BuiltinName::from(s)
Fix system builtin as implicit for cairo1 programs
Description
When executing a cairo1 program that had the system builtin as an implicit and it received input arguments, a memory error would stop execution. This was happening because it was trying to use memory that had been already used.
To execute a program, first an entry code is added (done in
create_entry_code()
). In the case of the System builtin we are adding the following CASM:This allocs one value into memory (because of the
tempvar
instruction).After creating the corresponding entry code we load the input arguments, if any, into the memory (done in
load_arguments()
). However this values are loaded after the ones of the entry code. As the entry code is executed after all of this, its values are still not present so we need to leave gaps in memory for them. The error we were seeing, happened because we were not leaving a gap for the value created in the entry code for the System builtin.Something similar happened when we wanted to use
proof_mode
. With this mode, the arguments also need to be on the output segment, so again if we do not leave the corresponding gaps, when trying to load them we would be trying to use unavailable memory.Added tests
There are different cases where we need to be careful with memory when loading tha arguments:
All these cases can create different combinations of situations were we would need to leave different gaps in the memory. I have added some tests that check some of these combinations:
Closes #1850
Checklist