Skip to content
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 arbitrary_cpi lint and add documentation to it #75

Merged
merged 3 commits into from
Feb 8, 2024

Conversation

S3v3ru5
Copy link
Contributor

@S3v3ru5 S3v3ru5 commented Feb 1, 2024

The arbitrary_cpi lint implementation had an incorrect match statement of which one of arm is never executed. The lint is updated to fix that.

The PR also How the lint is implemented documentation for the lint.

@S3v3ru5
Copy link
Contributor Author

S3v3ru5 commented Feb 1, 2024

The PR does not change much:
Previously, the find_program_id_for_instru function used to do two things: finding the assignment to the first field of the Instruction and then finding all alias of the program_id.

The PR moves the code finding the alias of program_id to a separate function.

Copy link
Member

@smoelius smoelius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just a couple nits. This PR introduces commented code, which appears to be incomplete, e.g.:

// println!("5. {:?}, {:?}", stmt, stmt.kind);
// println!("2. {:?}, {:?}, {:?}", assign_place, inst_arg, rvalue);
// println!("4. {:?}", inst_arg);
// println!("x. {:?}", pl);

/// - If one of the arg accesses `program_id`, check if the basic block containing the comparison
/// dominates the basic block containing call to `invoke` ensuring the `program_id` is checked in all execution
/// paths.
/// - If basic block does not dominate or there is no such comparison report the call to `invoke`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this sentence incomplete?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the sentence is complete. Is anything not clear?

@S3v3ru5
Copy link
Contributor Author

S3v3ru5 commented Feb 8, 2024

I have clarified documentation and update the PR. I have also updated to follow the "caller" to "callee" topological order for functions.

@smoelius smoelius added this pull request to the merge queue Feb 8, 2024
Merged via the queue into master with commit a6a1a97 Feb 8, 2024
3 checks passed
@smoelius smoelius deleted the fix-arbitrary-cpi branch February 8, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants