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

analyze: add NON_NULL rewrites #1095

Merged
merged 30 commits into from
Jun 25, 2024
Merged

analyze: add NON_NULL rewrites #1095

merged 30 commits into from
Jun 25, 2024

Conversation

spernsteiner
Copy link
Collaborator

This branch implements rewriting of nullable pointers (those lacking PermissionSet::NON_NULL) to Option. This includes the following kinds of rewrites:

  • Type annotations: Option<&mut T> instead of &mut T
  • Casts between nullable and non-nullable via p.unwrap() and Some(p). For most permissions, we implement only one direction because the other is invalid/nonsensical, but here we implement both in order to support "unsound" rewrites involving overridden NON_NULL flags (as in analyze: allow overriding dataflow for specific permissions #1088).
  • Borrows: when casting p: Option<&mut T>, we use p.as_deref_mut().unwrap() instead of p.unwrap() to avoid consuming the original p. (In the code, this is called a "downgrade", since it allows borrowing Option<Box<T>> as Option<&T> and similar.)
  • Pointer projections on nullable pointers. Where NON_NULL pointers would use &p[0], nullable ones use p.map(|ptr| &ptr[0]). Internally, this is represented similar to Some(&p.unwrap()[0]), but it's handled specially by rewrite::expr::convert to produce a map call instead, which passes through None without a panic.
  • unwrap() calls on derefs. *p is rewritten to *p.unwrap(), or to *p.as_deref().unwrap() if a downgrade/borrow is necessary to avoid moving p.
  • ptr::null() and 0 as *const _ to None, and p.is_null() to p.is_none().

The new non_null_rewrites.rs test case passes, and the rewritten code compiles.

This might be easier to review commit-by-commit.

@spernsteiner
Copy link
Collaborator Author

I realized while writing the PR that I never tested field projections (q = &(*p).field). I'll test it but will leave any fixes to a separate PR, since this one is already fairly complex.

@ahomescu
Copy link
Contributor

Left a couple of comments. This is a large PR, it will take me a while to get through.

c2rust-analyze/src/rewrite/expr/convert.rs Show resolved Hide resolved
c2rust-analyze/src/rewrite/expr/convert.rs Show resolved Hide resolved
c2rust-analyze/src/rewrite/expr/convert.rs Outdated Show resolved Hide resolved
for (i, mir_rw) in mir_rws.iter().enumerate() {
match mir_rw.rw {
// Bail out if we see nested delimiters.
mir_op::RewriteKind::OptionMapBegin => return None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we log this, somewhere in the call chain? It might be useful for users to know that this case is not supported, and where it occurs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added some clarification to the comment in ff20263. Bailing out here is not a hard error - it is legal, but suboptimal, to rewrite OptionMapBegin as p -> p.unwrap(). Also, currently mir_op should never generate rewrites that trigger this.


mir_op::RewriteKind::OptionMapBegin => {
// `p` -> `p.unwrap()`
Rewrite::MethodCall("unwrap /*map_begin*/".to_string(), Box::new(hir_rw), vec![])
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a TODO?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I initially added the /*map_begin*/ comment for debugging, and I think it's actually useful to leave it in place to help us catch any cases where the Option::map rewrite is not working properly.

c2rust-analyze/src/rewrite/expr/mir_op.rs Outdated Show resolved Hide resolved
c2rust-analyze/src/rewrite/expr/mir_op.rs Show resolved Hide resolved
c2rust-analyze/src/rewrite/expr/mir_op.rs Outdated Show resolved Hide resolved
c2rust-analyze/src/rewrite/expr/mir_op.rs Outdated Show resolved Hide resolved
c2rust-analyze/src/rewrite/expr/mir_op.rs Outdated Show resolved Hide resolved
@spernsteiner
Copy link
Collaborator Author

@ahomescu I've addressed all the previous feedback - any objections to merging this now?

@spernsteiner spernsteiner merged commit e526781 into master Jun 25, 2024
9 checks passed
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