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: assign fresh PointerIds to Ref and AddressOf rvalues #1028

Merged
merged 13 commits into from
Oct 11, 2023

Conversation

spernsteiner
Copy link
Collaborator

Given code like this:

let p: *mut S = ...;
let q: *mut i32 = &mut (*p).field;

Currently, we produce two PointerIds, one on p and one on q. The rvalue &mut (*p).field has the same PointerId as p, on the assumption that it must always have the same permissions as p itself.

In the pointee analysis, p and &mut (*p).field will usually have different pointee sets ({S} and {i32} respectively). But we'd like to use PointerIds to identify pointers in the pointee analysis, rather than inventing a whole new labeling system. This requires that p and &mut (*p).field have different PointerIds (though in the dataflow analysis, we will add a constraint relating their permissions, so that analysis will still produce identical results).

Actually adding PointerIds for the necessary rvalues is fairly trivial (see b57fa64). The majority of this PR consists of rewriter improvements to let it handle the new PointerIds. In particular, it can now apply rewrites to MIR code arising from expr adjustments. For example:

let arr: [u8; 3] = [0; 3];
let p: *const u8 = arr.as_ptr();

This produces the following MIR:

/*1*/   _1 = [const 0_u8; 3];
/*2*/   _4 = &_1;
/*3*/   _3 = move _4 as &[u8] (PointerCoercion(Unsize));
/*4*/   _2 = core::slice::<impl [u8]>::as_ptr(move _3) -> [return: bb1, unwind continue];

MIR line 1 is the initialization of arr. Line 2 applies a borrow adjustment to arr, line 3 applies an unsizing adjustment, and line 4 actually calls as_ptr. The MIR code produced is as if the programmer had written (&arr as &[u8]).as_ptr() rather than arr.as_ptr(). Previously, if the rewrite::expr::mir_op module tried to introduce rewrites on lines 2 or 3, it would result in an error; with this PR, those rewrites are correctly lifted to HIR (after materializing the adjustments so there is a place in the source code to apply those rewrites).

Comment on lines +29 to 32
// FIXME: The initializers for `r1` and `r2` are rewritten incorrectly. Neither `s` nor the
// `r` field have `Cell` type, and an `&mut T -> &Cell<T>` rewrite is not applied.
// XXXXX: let r1: &core::cell::Cell<(R)> = &((*s).r);
let r1: *mut R = &mut (*s).r;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The previous rewrite of this code was already broken, so I figured it was okay that the new code also breaks here.

@@ -166,22 +168,41 @@ impl<'tcx> Visitor<'tcx> for ConvertVisitor<'tcx> {
}
};

let expr_rws = take_prefix_while(&mut mir_rws, |x: &DistRewrite| {
Copy link
Contributor

Choose a reason for hiding this comment

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

what invariant does this assume about the ordering of MIR rewrites? what if MirOriginDesc::Expr is the second rewrite in the list, or they aren't all contiguous and a prefix of mir_rws? A comment on this and why a prefix is the correct subsequence to grab would be helpful

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 a comment: 7c17a85

@spernsteiner spernsteiner force-pushed the analyze-projection-pointerid branch from 266e14d to 60aa23e Compare October 9, 2023 22:47
@spernsteiner
Copy link
Collaborator Author

Addressed all comments so far

@spernsteiner spernsteiner merged commit b8c4a4c into master Oct 11, 2023
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