-
Notifications
You must be signed in to change notification settings - Fork 581
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
Fix bug in local variables aliasing. #5028
Conversation
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.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @gilbens-starkware)
Suggestion:
Fix bug in local variables aliasing.
crates/cairo-lang-sierra-generator/src/local_variables.rs
line 286 at r1 (raw file):
output_vars: &[VariableId], params_signatures: &[ParamSignature], ) -> BranchInfo {
more sensible - as the order is:
info for the inputs.
info for the outputs.
var info for inputs.
var info for outputs.
Suggestion:
fn analyze_branch(
&mut self,
params_signatures: &[ParamSignature],
branch_signature: &BranchSignature,
input_vars: &[VarUsage],
output_vars: &[VariableId],
) -> BranchInfo {
crates/cairo-lang-sierra-generator/src/local_variables.rs
line 291 at r1 (raw file):
match output_info.ref_info { OutputVarReferenceInfo::SameAsParam { param_idx } => { // TODO(Gil): Maintain the input variable state (const, add const, deferred or
add doc as to why - not just a TODO.
crates/cairo-lang-sierra-generator/src/local_variables_test_data/match_extern
line 16 at r1 (raw file):
revoke_ap(); // y is revoked, but it shouldn't, since it's the same as x, which is local. // TODO(Gil): This is because the call to felt252_is_zero revokes, see
change the test to still test the original result.
add this test to keep the todo.
tests/bug_samples/lib.cairo
line 3 at r1 (raw file):
mod ecdsa_completeness; mod indirect_impl_alias; mod revoked_locals;
sort
tests/bug_samples/revoked_locals.cairo
line 17 at r1 (raw file):
} // #[inline(never)]
remove
a5a11da
to
95aedcb
Compare
95aedcb
to
7e20ae3
Compare
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.
Reviewable status: 2 of 6 files reviewed, 6 unresolved discussions (waiting on @orizi)
-- commits
line 2 at r1:
Done.
crates/cairo-lang-sierra-generator/src/local_variables.rs
line 286 at r1 (raw file):
Previously, orizi wrote…
more sensible - as the order is:
info for the inputs.
info for the outputs.
var info for inputs.
var info for outputs.
Done.
crates/cairo-lang-sierra-generator/src/local_variables.rs
line 291 at r1 (raw file):
Previously, orizi wrote…
add doc as to why - not just a TODO.
Done.
crates/cairo-lang-sierra-generator/src/local_variables_test_data/match_extern
line 16 at r1 (raw file):
Previously, orizi wrote…
change the test to still test the original result.
add this test to keep the todo.
Done.
tests/bug_samples/lib.cairo
line 3 at r1 (raw file):
Previously, orizi wrote…
sort
Done.
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.
Reviewed 2 of 4 files at r2.
Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware)
crates/cairo-lang-sierra-generator/src/local_variables.rs
line 299 at r3 (raw file):
if params_signatures[param_idx].allow_const && params_signatures[param_idx].allow_add_const && params_signatures[param_idx].allow_deferred
the same test is required for PartialParam
as well.
This may make you have to do the proper solution instead.
Code quote:
if params_signatures[param_idx].allow_const
&& params_signatures[param_idx].allow_add_const
&& params_signatures[param_idx].allow_deferred
7e20ae3
to
0bd77a4
Compare
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.
Reviewable status: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @orizi)
crates/cairo-lang-sierra-generator/src/local_variables.rs
line 299 at r3 (raw file):
Previously, orizi wrote…
the same test is required for
PartialParam
as well.
This may make you have to do the proper solution instead.
Changed to follow only consts.
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.
Reviewed 1 of 1 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware)
crates/cairo-lang-sierra-generator/src/local_variables.rs
line 237 at r4 (raw file):
var } pub fn peel_const_aliases(&'a self, mut var: &'a VariableId) -> &VariableId {
doc
commit-id:e86ab7d4
0bd77a4
to
feb1a78
Compare
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.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @orizi)
crates/cairo-lang-sierra-generator/src/local_variables.rs
line 237 at r4 (raw file):
Previously, orizi wrote…
doc
Done.
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.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @gilbens-starkware)
Stack:
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)