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: omit unused hypothetical lifetimes during rewriting #1015

Merged
merged 7 commits into from
Aug 23, 2023

Conversation

spernsteiner
Copy link
Collaborator

This change removes hypothetical lifetimes for pointers that are rewritten into non-ref types, such as raw pointers or Box<T>. The AdtMetadataTable is computed normally at first so it can be used in borrowck, but once all pointer permissions have been determined, it's recomputed with an extra filter that skips creating hypothetical region params for pointers whose type_desc::Ownership is not Imm, Cell, or Mut (&T, &Cell<T>, or &mut T).

For example:

struct Foo {
   p: *mut u8,
   #[c2rust_analyze_test::fixed_signature]
   q: *mut u8,
}

Previously, Foo would be rewritten to have two new region parameters, one for p and one for q. But the region for q would unused: q is marked FIXED, so it's left as *mut u8 rather than rewritten to a safe reference type. With this change, Foo is rewritten to have only one region parameter, the one for p. This avoids rustc errors about unused generic parameters in the rewritten code.

Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

For 5a88caa, were you seeing known ptr permissions change? Which ones?

@spernsteiner
Copy link
Collaborator Author

For 5a88caa, were you seeing known ptr permissions change? Which ones?

Opened #1017 with the specific cases I saw in lighttpd

@aneksteind
Copy link
Contributor

@spernsteiner are we somehow constraining things, resulting in a different set of rewrites, by giving hypothetical lifetimes to polonius that won't end up being used?

@aneksteind
Copy link
Contributor

This apparently also fixes #994

@spernsteiner
Copy link
Collaborator Author

are we somehow constraining things, resulting in a different set of rewrites, by giving hypothetical lifetimes to polonius that won't end up being used?

For variables that get rewritten to Box/Rc, the presence or absence of UNIQUE doesn't affect the rewrite. So I guess the concern here would be that including these lifetimes could add constraints on other variables where UNIQUE does matter. I don't know for sure whether or not this can happen, but I would guess not - borrowing the data inside a box is pretty similar from a borrowck perspective to borrowing the data behind an &mut.

If this does turn out to be an issue, we can think about borrowck improvements then (and having a real example to look at will help).

aneksteind added a commit that referenced this pull request Aug 22, 2023
@spernsteiner spernsteiner changed the base branch from analyze-actual-lifetimes-base to master August 23, 2023 17:07
@spernsteiner spernsteiner merged commit ad0c269 into master Aug 23, 2023
8 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
3 participants