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

Permit mutable references in all const contexts #78578

Merged
merged 7 commits into from
Jan 25, 2021

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Oct 30, 2020

fixes #71212

cc @rust-lang/wg-const-eval @christianpoveda

@rust-highfive
Copy link
Collaborator

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 30, 2020
@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 30, 2020

r? @ecstatic-morse

@RalfJung
Copy link
Member

RalfJung commented Oct 31, 2020

This also allows (with feature(const_mut_ref)) mutable references in the final value of
immutable statics, which is not a problem, because
accessing a mutable static FOO works via *&FOO, so
the intermediate immutable reference blocks any mis-use.

But what about interior mutability? Also, what is even the problem against which this is defending? Assuming the lifetimes work out, I see no reason that an &mut in a static would cause any issue. The only concern is we need to make sure that we put it into mutable global memory whenever it can actually be mutated. Interning should already get this right, but we should have some tests to ensure that this is the case, but I am not sure what the best way is to test for this.

Constants prevent mutable references in their final value
via two separate means:

So, we are giving up on doing static checks here, and rely on (post-monomorphitation) "dynamic" checks?

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 1, 2020

But what about interior mutability?

We already permit that on stable and it's only possible through wrapper types with unsafe impl Sync, so the user said it's alright.

Also, what is even the problem against which this is defending?

Since we allow static FOO: &mut i32 = &mut 42; now, that could mean users can at any arbitrary point write

let x: &'static mut i32 = FOO;
let y: &'static mut i32 = FOO;

and then they have two mutable references to the same memory. BUT since we actually generate the above as

let x: &'static mut i32 = *&FOO;
let y: &'static mut i32 = *&FOO;

The user will get an error because you can't access a mutable reference behind an immutable referenc as mutable.

Assuming the lifetimes work out, I see no reason that an &mut in a static would cause any issue.

Not in the current way that MIR implements static accesses, and not in the way I think we want to ever use statics, but it still something we need to protect against.

The only concern is we need to make sure that we put it into mutable global memory whenever it can actually be mutated. Interning should already get this right, but we should have some tests to ensure that this is the case, but I am not sure what the best way is to test for this.

Why into mutable memory? You cannot safely access it mutably, and any access will go through an immutable reference, so unsafely accessing it mutably would be UB, right?

So, we are giving up on doing static checks here, and rely on (post-monomorphitation) "dynamic" checks?

hmm... indeed. I'll read through @eddyb 's suggestion in zulip again.

@RalfJung
Copy link
Member

RalfJung commented Nov 1, 2020

We already permit that on stable and it's only possible through wrapper types with unsafe impl Sync, so the user said it's alright.

What I meant is, RefCell<&mut T> should be okay... right? Your OP does not talk about this case at all.

Since we allow static FOO: &mut i32 = &mut 42; now, that could mean users can at any arbitrary point write

Ah, right. So even if we move away again from "statics as const pointers", we have to make sure they are still treated as an immutable place by the borrow checker.

Why into mutable memory? You cannot safely access it mutably, and any access will go through an immutable reference, so unsafely accessing it mutably would be UB, right?

I said "we need to make sure that we put it into mutable global memory whenever it can actually be mutated" (emphasis is new). Like for example in case of RefCell<&mut T>. I think our code does this right, but it is a case worth carefully documenting and thinking through (and testing), that's why I brought it up.

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 2, 2020

What I meant is, RefCell<&mut T> should be okay... right? Your OP does not talk about this case at all.

Well, not RefCell per se, but a custom Sync type could work from a type-system perspective. It will still not work though.

use core::cell::UnsafeCell;
struct NotAMutex<T>(UnsafeCell<T>);

unsafe impl<T> Sync for NotAMutex<T> {}

const FOO: NotAMutex<&i32> = NotAMutex(UnsafeCell::new(&42));

The above only works, because &42 is promoted. I think if you could move to &mut (like in this PR), you'd get borrow errors. You can emulate the effect on nightly with immutable references by using a block:

const FOO: NotAMutex<&i32> = NotAMutex(UnsafeCell::new(&{
    let x = 42;
    x
}));

I'll write appropriate tests and post my findings here.

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 2, 2020

I added a new test for your example, and as I expected, it is not possible to do that, so we do not need to check whether interning can handle this.

@RalfJung
Copy link
Member

RalfJung commented Nov 2, 2020

What about using &mut STATIC_MUT?

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 2, 2020

For constants that is not possible anyway (yes, this time it is really a static check ;) ).

For static items, it's not a problem, as the allocation behind the reference is not part of the static currently being interned. For all other concerns, accessing a mutable static requires unsafe, so the user is the one who has to prove correctness.

@RalfJung
Copy link
Member

RalfJung commented Nov 3, 2020

Hm, I am running out of ideas.^^ Would unleashing Miri let us disable some of these restrictions?

I feel like there probably are or will be other ways to get &mut, such as transmuting &UnsafeCell<T> to &mut T or Box::leak or so... I think our code should handle this all correctly, so I would much rather rely on the interner being correct than relying on it being impossible to write such code. You invoked several otherwise independent invariants to crush my counterexamples, which makes this all seem rather fragile.

In particular you invoked the const-may-not-point-to-static restriction, which only exists for the benefit of const-in-pattern and which rejects way more code that it would have to. If, hypothetically, we'd instead dynamically check this only when a const is actually used in a pattern, your scheme here does not work any more. I have a plan for how to make that restriction not load-beaing any more (when constructing a valtree, ensure we only read from immutable allocations; make patterns go through valtrees). I think we should be careful not to add anything else that would make this restriction load-bearing in different ways.

EDIT: Or, did you invoke that restriction? I am losing track of what all has to work together to make this sound, which is a bad sign. :/ But I think yes you did, "For constants that is not possible anyway" -- "that" being &mut STATIC_MUT. But why does the argument that applies to static not also apply to const here?

Interning prevents pointers to allocations except as part of references

As discussed in this PR before, I don't think this check should be relied upon for soundness.

So I propose that for static, the soundness argument is more along the lines of:

  • all accesses go through shared references, so plain &mut cannot be used for mutation anyway
  • when there is an UnsafeCell or "hidden pointers", we correctly intern allocations referenced inside there as mutable

For const, we disallow mutable references to show up directly, as you noted... but for "hidden pointers" the story is much less clear, unless we want to commit to never allowing those.

@RalfJung
Copy link
Member

RalfJung commented Nov 3, 2020

when there is an UnsafeCell or "hidden pointers", we correctly intern allocations referenced inside there as mutable

Btw, there is a funny corner case here where the same allocation is referenced multiple times, so it might end up immutable from the first reference we encounter to it and then later another reference wants it to be mutable... something like

#![feature(const_mut_refs)]
#![feature(const_raw_ptr_deref)]
static mut FOO: i32 = 0;

struct SyncRawPtr<T>(*mut T);
unsafe impl<T> Sync for SyncRawPtr<T> {}

static EVIL: (&i32, SyncRawPtr<i32>) = {
  let x: *mut i32 = unsafe { &mut FOO };
  (unsafe { &*x }, SyncRawPtr(x))
};

or

#![feature(const_mut_refs)]
#![feature(const_raw_ptr_deref)]
static mut FOO: i32 = 0;

struct SyncRawPtr<T>(*mut T);
unsafe impl<T> Sync for SyncRawPtr<T> {}

const fn make_evil() -> (&'static i32, SyncRawPtr<i32>) {
    let x: *mut i32 = unsafe { &mut FOO };
    (unsafe { &*x }, SyncRawPtr(x))
}

static EVIL: (&i32, SyncRawPtr<i32>) = {
    make_evil()
};

The latter seems to hinge only on "constant functions cannot refer to statics" (and one could imagine Box::leak(Box::new(0)) working some day to provide an alternative).

One could argue that if EVIL.0 is never used, writing to EVIL.1 is actually fine...

@RalfJung
Copy link
Member

RalfJung commented Nov 3, 2020

Sorry for all the rambling, but I feel very uneasy about this change.^^ I don't think the required soundness conditions and the checks that uphold them have been documented clearly enough. We shouldn't start with examples, we should start with "here is a list of (dynamic) conditions that, if they are all met, we agree are sufficient to make this all sound", and then we should go on with "here are some (static and/or dynamic) checks that ensure that all conditions are met", and then we can come up with testcases to try to beak them.

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 3, 2020

We shouldn't start with examples, we should start with "here is a list of (dynamic) conditions that, if they are all met, we agree are sufficient to make this all sound", and then we should go on with "here are some (static and/or dynamic) checks that ensure that all conditions are met", and then we can come up with testcases to try to beak them.

I think part of this is because, as you correctly noted, we are mixing a lot of different concerns. E.g. I consider it ok for the dynamic checks to permit const FOO: *mut i32 = &mut 42; since, that is not a soundness issue, just a footgun. Obviously the static checks must at least cover all dynamic checks, but we don't just need to cover "too hard to statically check" in addition, we can also add static checks to prevent footguns.

One could argue that if EVIL.0 is never used, writing to EVIL.1 is actually fine...

I thought we had to consider all references to be used, no matter if they actually are. I don't see how having a shared reference to some memory that is mutated through other means is fine, but a reference to dangling memory is not.

The only way that example seems doable to me is to use *const i32 instead of &i32, and then we can't intern immutably anyway.

@RalfJung
Copy link
Member

RalfJung commented Nov 3, 2020

I thought we had to consider all references to be used, no matter if they actually are. I don't see how having a shared reference to some memory that is mutated through other means is fine, but a reference to dangling memory is not.

Well, the following is allowed under Stacked Borrows in runtime code:

    let x: *mut i32 = unsafe { &mut FOO };
    let (_, x) = (unsafe { &*x }, SyncRawPtr(x));
    unsafe { *x = 1; }

Basically, the lifetime of the shared reference ends immediately, that's why this is okay. (It would also be permitted to dangle.) The guarantee of a shared ref is that between any two of its uses, nothing has been mutated, but if the shared ref is unused... well, this guarantee doesn't do anything. (The only special case is references passed as function parameters; that is the "protector" stuff in Stacked Borrows.)

I think to obtain the behavior you seem to expect, we'd need to somehow reflect the idea that "the 'static lifetime goes on forever" into Stacked Borrows, making sure the tag of the shared reference never gets popped... but we don't actually have any tags in compile-time computed pointers so this cannot really work.^^ Indeed having provenance somehow connect compile-time and run-time computations seems tricky (and strange), but if we make compile-time-computed pointers have no provenance, that automatically means there is no way to distinguish different compile-time computed pointers to the same data.

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 3, 2020

So... "must be unused" is actually not true here? We could alternately use the reference to read and the SyncRawPtr to write, as long as we don't keep a reference around while using SyncRawPtr? At least from a stacked borrows perspective.

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 3, 2020

Sorry, all of this is getting a bit off topic, but I really want to understand the details even if irrelevant to this PR. Should we take it to some other place?

I fully agree that we should not intern an allocation as immutable if some later place wants to intern it as mutable.

Though, in the case you showed, we won't intern anything anyway, as that is a pointer to a mutable static, which by itself is already a mutable allocation. But for hyptothetical heap allocations, this could definitely occur.

@RalfJung
Copy link
Member

RalfJung commented Nov 3, 2020

Right, sorry, I was somewhat distracted this morning and did not have enough time to reply properly. I still don't really have enough time...

So in terms of soundness, if all "inner" static allocations are interned mutable and hidden pointers are not allowed in const, I think I can agree that this is all sound. I just did not agree with the reasons you gave for soundness above, namely that how &mut can appear in static is restricted... that was never a soundness goal so far, so I do not think it should become one now. But I think it is not necessary, if interning does the right thing.

So my main soundness concerns are around

  • Interning inner static allocations as immutable: we need to be really sure that this is indeed correct. I wonder if validity checking can help here... probably not because of raw pointers.
  • Can we do this in a way that leaves the door open for "hidden pointers" in a const without being a huge footgun? A lint is likely to have many false positives, which is not great.

@RalfJung
Copy link
Member

RalfJung commented Nov 6, 2020

After thinking about this some more, I wonder if there isn't another approach -- given my inability to come up with an example that has mutable memory in an inner static allocation. We could say that all inner allocations of static (and static mut) are interned immutably. We'd have to add some tests to make sure that this holds up when with Miri-unleashed (and validation might be able to help by detecting UnsafeCell), but if it does it would let us remove all the type-based interning and instead just intern all inner allocations of everything immutably, which would be a great simplification.

@oli-obk almost convinced me that this could be sound for current CTFE, though I'd need to think about this some more. It would clearly not be sound with heap allocations during CTFE, but those are easily recognized and we could just always intern them mutably.

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 9, 2020

I can unconvince you in a single line that works on stable:

static mut FOO: &mut [u32] = &mut [5, 3, 4];

your mention of static mut reminded me that we have this special case for slices in mutable statics. Maybe I should finally create the future incompat lint removing static mut from the language?

@RalfJung
Copy link
Member

RalfJung commented Nov 9, 2020

I can unconvince you in a single line that works on stable:

Isn't that just because #75585 did not arrive on stable yet?

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jan 17, 2021

☔ The latest upstream changes (presumably #80942) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member

r? @RalfJung
@rustbot modify labels: -S-waiting-on-review +S-waiting-on-author
(There are some unresolved comments at the top of the discussion; as far as I can see they still apply.)

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2021
Comment on lines 21 to 32
const fn helper() -> Option<&'static mut i32> { unsafe {
// Undefined behaviour, who doesn't love tests like this.
// This code never gets executed, because the static checks fail before that.
Some(&mut *(42 as *mut i32))
} }
// Check that we do not look into function bodies.
// We treat all functions as not returning a mutable reference, because there is no way to
// do that without causing the borrow checker to complain (see the B5/helper2 test below).
const B4: Option<&mut i32> = helper();

const fn helper2(x: &mut i32) -> Option<&mut i32> { Some(x) }
const B5: Option<&mut i32> = helper2(&mut 42); //~ ERROR temporary value dropped while borrowed
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'm not too happy with B4... but also not sure what else to do here. We could start running checks on the return type of functions, but I'm not sure how to do that properly, especially once generics are involved.

Copy link
Member

Choose a reason for hiding this comment

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

because the static checks fail before that

Why is there no //~ ERROR here then?
What if you do Some(&mut *(&mut 42 as *mut i32)) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the static checks on other constants fail. This code will work and then error in validation if we were getting this far, but since everything else erros first, and B4 is not used, it isn't evaluated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if you do Some(&mut *(&mut 42 as *mut i32)) instead?

that would just give us a dangling pointer, but still not get to validation. I could put this test in a separate file, then we would get to validation.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, the static checks in other consts fail.

Looks like this test needs its own file then.

that would just give us a dangling pointer, but still not get to validation. I could put this test in a separate file, then we would get to validation.

Sure, I meant to test that in addition to B4, not instead of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@RalfJung
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 24, 2021

📌 Commit 819b008 has been approved by RalfJung

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 24, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 24, 2021
…as-schievink

Rollup of 14 pull requests

Successful merges:

 - rust-lang#75180 (Implement Error for &(impl Error))
 - rust-lang#78578 (Permit mutable references in all const contexts)
 - rust-lang#79174 (Make std::future a re-export of core::future)
 - rust-lang#79884 (Replace magic numbers with existing constants)
 - rust-lang#80855 (Expand assert!(expr, args..) to include $crate for hygiene on 2021.)
 - rust-lang#80933 (Fix sysroot option not being honored across rustc)
 - rust-lang#81259 (Replace version_check dependency with own version parsing code)
 - rust-lang#81264 (Add unstable option to control doctest run directory)
 - rust-lang#81279 (Small refactor in typeck)
 - rust-lang#81297 (Don't provide backend_optimization_level query for extern crates)
 - rust-lang#81302 (Fix rendering of stabilization version for trait implementors)
 - rust-lang#81310 (Do not mark unit variants as used when in path pattern)
 - rust-lang#81320 (Make bad shlex parsing a pretty error)
 - rust-lang#81338 (Clean up `dominators_given_rpo`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d9c177f into rust-lang:master Jan 25, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 25, 2021
@oli-obk oli-obk deleted the const_mut_refs branch January 25, 2021 08:25
@jplatte jplatte mentioned this pull request Jan 26, 2021
65 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. F-const_mut_refs `#![feature(const_mut_refs)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Static checks for &mut in final value of constant with #![feature(const_mut_refs)]
9 participants