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

provide statics with hypothetical origins #975

Merged
merged 15 commits into from
Jul 22, 2023
Merged

Conversation

aneksteind
Copy link
Contributor

@aneksteind aneksteind commented Jul 5, 2023

Fixes #943 and fixes #776. Assigns origins to static variables, which provides parity in the number of OriginParams when comparing equality of LHS and RHS of assignments to local variables (which already had the correct number of OriginParams).

Questions:

  • polonius does not appear to issue loans for statics, but it accounts for regions associated with them. Does the assign_origins routine for statics adequately cover this?
    • in a statement such as _2 = const {alloc1: *mut fdnode} the RHS is not issued a loan; is that correct? Polonius appears not to issue any loans here
  • what should be done for string literals?

@aneksteind aneksteind marked this pull request as ready for review July 5, 2023 13:30
Copy link
Contributor

@fw-immunant fw-immunant left a comment

Choose a reason for hiding this comment

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

Mostly looks good, a couple questions inline.

c2rust-analyze/src/borrowck/type_check.rs Outdated Show resolved Hide resolved
c2rust-analyze/src/borrowck/type_check.rs Show resolved Hide resolved
@spernsteiner
Copy link
Collaborator

polonius does not appear to issue loans for statics, but it accounts for regions associated with them

Sounds right. Loans are created on &/&mut operations; for statics, those operations essentially happen before the start of main, and aren't part of any function (so we don't analyze them).

Does the assign_origins routine for statics adequately cover this?

Have you checked what Polonius normally does for statics that contain 'static lifetimes? I could imagine, for example, that there might be only a single origin corresponding to the concrete lifetime 'static, or that it's otherwise treated in some special way.

in a statement such as _2 = const {alloc1: *mut fdnode} the RHS is not issued a loan; is that correct? Polonius appears not to issue any loans here

I'm not sure. What does Polonius do for let x = &MY_STATIC? Does this issue a loan, or is it treated as a sort of no-op projection, like &*some_ref? It might be better to treat the const { alloc: *mut T } as a similar sort of loan or projection (matching whatever Polonius does for &MY_STATIC), since unlike rustc we treat *mut as if it were a ref and give it an origin.

Note this is separate from the previous point, which is about statics containing refs, like BAR here:

static FOO: i32 = 1;
static BAR: &'static i32 = &FOO;

@aneksteind
Copy link
Contributor Author

aneksteind commented Jul 7, 2023

Have you checked what Polonius normally does for statics that contain 'static lifetimes? I could imagine, for example, that there might be only a single origin corresponding to the concrete lifetime 'static, or that it's otherwise treated in some special way.

I'm not sure. What does Polonius do for let x = &MY_STATIC? Does this issue a loan, or is it treated as a sort of no-op projection, like &*some_ref? It might be better to treat the const { alloc: *mut T } as a similar sort of loan or projection (matching whatever Polonius does for &MY_STATIC), since unlike rustc we treat *mut as if it were a ref and give it an origin.

@spernsteiner I've run polonius on the following example:

static mut oneshot_fdn: fdnode = fdnode { ctx: &0 };

pub struct fdnode {
    pub ctx: &'static u8,
}

unsafe extern "C" fn server_free() {
    let x = &oneshot_fdn;
}

fn main() {
    unsafe {
        server_free();
    }
}

and it appears:

  • no loans are issued
  • there are two universal regions
  • there are various subset relations, but none of them are in terms of the universal regions
  • none of the universal regions correspond to anything static or 'static

@aneksteind
Copy link
Contributor Author

aneksteind commented Jul 7, 2023

I'm not sure. What does Polonius do for let x = &MY_STATIC? Does this issue a loan, or is it treated as a sort of no-op projection, like &*some_ref?

It looks like the MIR for this treats as something like that:

        _2 = const {alloc1: *mut fdnode}; // scope 0 at test_static.rs:8:14: 8:25
                                         // mir::Constant
                                         // + span: test_static.rs:8:14: 8:25
                                         // + literal: Const { ty: *mut fdnode, val: Value(Scalar(alloc1)) }
        _1 = &(*_2);                     // scope 0 at test_static.rs:8:13: 8:25

and use_of_var_derefs_origin.facts notes:

"_1"	"'_#4r"

@spernsteiner
Copy link
Collaborator

It looks like rustc -Z nll-facts treats 'static as a universal/placeholder region, as if it was a parameter with the constraint that it outlives all other parameters. E.g. fn f<'a>(...) is treated like fn f<'static: 'a, 'a>(...). So there isn't much special handling for 'static in Polonius itself, aside from generating those extra constraints. As an implementation detail, the first placeholder region '_#0r is always 'static, then each lifetime parameter of the function gets a placeholder region, and finally there's an extra placeholder region at the end (maybe 'empty, or maybe a lifetime representing the duration of the function, or maybe something else). The first region outlives the lifetime parameters, and the parameters outlive the last region.

Accesses of statics (e.g. let x = FOO; where static FOO: i32) produce no loans and have no constraints relating them to 'static/'_#0r, as you noted. For non-mut statics, which produce MIR like _1 = const {alloc2: &'_#3r i32};, rustc seems to create a fresh region to the rvalue &i32, but leaves the region unconstrained, so it can be equal to 'static or any shorter region as needed. (The assignment then sets a constrain between the region on the LHS and the region '_#3r on the RHS.) I think it would also be legal to constrain the region to be exactly 'static (via subset_base('_#3r, '_#0r); subset_base('_#0r, '_#3r)), or to constrain it to be "'static or shorter". For mut statics, the MIR looks like _4 = const {alloc5: *mut i32};, and subsequent "reborrow" operations like _6 = &(*_4); create a fresh region for the Rvalue::Ref but leave it unconstrained.

Since we generally assign regions to raw pointers as well as refs, I think we should treat both the mut and non-mut cases the same: assign a region to the RHS, and leave it unconstrained so it can become 'static if needed (or something shorter if not).

For mentions of 'static within statics (e.g. static FOO_REF: &'static i32), it looks like rustc generates a fresh region each time the static is mentioned, then constrains it to be equal to 'static (x <= '_#0r, '_#0r <= x). This seems like a reasonable approach to me (and in general it's good to stick to the way rustc does things when possible). Instead of assigning regions to the actual static declarations, we would generate new fresh regions and constraints at each of their uses (MIR const { allocN: *mut T } etc.). Real and hypothetical regions can be treated the same in this context.

I think this also simplifies the Operand::Constant case, since we no longer need to look up the actual static DefId - we simply generate a fresh region for each position where one can appear in the type, and (since global allocs can only refer to 'static data) constrain each fresh region to be 'static.

@aneksteind
Copy link
Contributor Author

@spernsteiner I haven't addressed the 'static lifetime piece yet, but I've addressed generating origins for statics in 99adfbb. Does that seem aligned with what we talked about?

c2rust-analyze/src/borrowck/mod.rs Outdated Show resolved Hide resolved
c2rust-analyze/src/borrowck/mod.rs Outdated Show resolved Hide resolved
c2rust-analyze/src/borrowck/type_check.rs Outdated Show resolved Hide resolved
c2rust-analyze/src/borrowck/type_check.rs Show resolved Hide resolved
@aneksteind
Copy link
Contributor Author

@spernsteiner would you mind taking another look?

c2rust-analyze/src/borrowck/mod.rs Outdated Show resolved Hide resolved
if let ItemKind::Fn(_, ref generics, _) = item.kind {
for generic in generics.params.iter() {
if matches!(generic.kind, GenericParamKind::Lifetime { .. }) {
func_lifetime_origins.push(maps.origin())
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are a bunch of relations that rustc would normally add over these origins - please either emit the relevant facts or add comments pointing out that they're missing:

  • My read of the polonius docs is that we should emit universal_region(origin) and placeholder(origin, loan) (where loan is a fresh loan, not connected to anything else) for each of these region parameters and also for 'static (which is treated like a parameter).
  • There should be either known_placeholder_subset or subset_base relations matching each outlives constraint 'a: 'b in tcx.predicates_of(local_def_id).
  • There should be similar subset relations for the implicit outlives constraints 'static: 'a for each parameter 'a.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this match what you suggest? d6220b7

Copy link
Contributor Author

@aneksteind aneksteind Jul 21, 2023

Choose a reason for hiding this comment

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

It seems like there is one remaining known placeholder subset relationship between lifetimes like arg: &'a &'c i32. I can look into what it would take to add it to this PR or we can save it for another PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

That looks about right to me.

The 'c: 'a bound that's implied by well-formedness of arg's type should definitely show up somewhere. I think I've seen bounds like that in the output of either predicates_of or a related query. Maybe try looking at explicit_predicates_of, inferred_outlives_of, and/or predicates_defined_on.

It would also be fine to just leave a comment about the limitation, especially if you can't find an easy way to get the predicate from rustc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

predicates_defined_on seems to encapsulate the former two, and predicates_of is supposed to almost-always be exactly predicates_defined_on (according to the documentation), but I don't see that any of these options capture the implicit relationship. i'll keep digging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created an issue for this: #1002 (comment)

c2rust-analyze/src/borrowck/mod.rs Outdated Show resolved Hide resolved
c2rust-analyze/src/borrowck/type_check.rs Show resolved Hide resolved
c2rust-analyze/src/borrowck/type_check.rs Outdated Show resolved Hide resolved
c2rust-analyze/src/context.rs Outdated Show resolved Hide resolved
@aneksteind aneksteind merged commit 75a0e05 into master Jul 22, 2023
9 checks passed
@aneksteind aneksteind deleted the feat.static.origins branch July 22, 2023 00:37
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