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

Make sure interned constants are immutable #63955

Merged
merged 8 commits into from Sep 16, 2019

Conversation

@RalfJung
Copy link
Member

commented Aug 27, 2019

This makes sure that interning for constants (not statics) creates only immutable allocations.

Previously, the "main" allocation of const FOO: Cell<i32> = Cell::new(0); was marked as mutable, but I don't think we want that. It can be only copied, not written to.

Also, "leftover" allocations (behind raw pointers etc) were left mutable. I don't think we want to support that. I tried asserting that these are all already immutable (to double-check our static checks), but that failed in this one:

const NON_NULL_PTR2: NonNull<u8> = unsafe { mem::transmute(&0) };

Seems like maybe we want more precise mutability annotation inside Miri for locals (like &0 here) so that this would actually become immutable to begin with?

I also factored intern_shallow out of the visitor so that we don't have to construct a visitor when we do not plan to visit anything. That confused me at first.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 27, 2019

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

@RalfJung

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

) -> InterpResult<'tcx> {
let tcx = ecx.tcx;
// this `mutability` is the mutability of the place, ignoring the type
let (mutability, base_intern_mode) = match tcx.static_mutability(def_id) {
let (base_mutability, base_intern_mode) = match tcx.static_mutability(def_id) {
Some(hir::Mutability::MutImmutable) => (Mutability::Immutable, InternMode::Static),
None => (Mutability::Immutable, InternMode::ConstBase),
// `static mut` doesn't care about interior mutability, it's mutable anyway
Some(hir::Mutability::MutMutable) => (Mutability::Mutable, InternMode::Static),
};

This comment has been minimized.

Copy link
@RalfJung

RalfJung Aug 27, 2019

Author Member

Does this correctly compute ConstBase for things like array sizes and non-Copy array initializers? What does it do for promoteds?

This comment was marked as resolved.

Copy link
@oli-obk

oli-obk Aug 28, 2019

Contributor

for promoteds we also run into the None branch, as the DefId is of the function of the promoted, which doesn't have static_mutability.

The only way we cannot get ConstBase is by the DefId pointing to a static. So not sure what happens to non-Copy array initializers, as that's promotion happening inside a static

This comment has been minimized.

Copy link
@RalfJung

RalfJung Aug 28, 2019

Author Member

the DefId is of the function of the promoted, which

But what if we are promoting inside the body of a static?

not sure what happens to non-Copy array initializers, as that's promotion happening inside a static

@eddyb do you know more here?

This comment has been minimized.

Copy link
@oli-obk

oli-obk Aug 28, 2019

Contributor

Do we actually create implicit promoteds inside statics? I don't think so. I think we just nuke a bunch of storage statements

This comment has been minimized.

Copy link
@RalfJung

RalfJung Aug 28, 2019

Author Member

Hm... seems like that is the case, and also for const:

const fn id<T>(x: T) -> T { x }

const FOO: &'static usize = {
    let v = id(&std::mem::size_of::<i32>());
    v
};

Check the MIR

But... @eddyb isn't that in contradiction to what you told me about the "enclosing scope" stuff?

This comment has been minimized.

Copy link
@RalfJung

RalfJung Aug 29, 2019

Author Member

Looks like you are right.

Are there other cases we have to worry about here in terms of classifying CTFE-evaluated "stuff" into (explicit) statics and consts ("everything but (explicit) statics")?

This comment has been minimized.

Copy link
@oli-obk

oli-obk Sep 11, 2019

Contributor

Kind of related is that consts can't refer to statics

This comment has been minimized.

Copy link
@RalfJung

RalfJung Sep 15, 2019

Author Member

How is that related? Even if they would, that memory would already be interned.

This comment has been minimized.

Copy link
@oli-obk

oli-obk Sep 16, 2019

Contributor

It was the only thing I could come up with where we decide only on explict static vs not

This comment has been minimized.

Copy link
@RalfJung

RalfJung Sep 16, 2019

Author Member

"explict static vs not"?

src/librustc_mir/interpret/intern.rs Show resolved Hide resolved
) -> InterpResult<'tcx> {
let tcx = ecx.tcx;
// this `mutability` is the mutability of the place, ignoring the type
let (mutability, base_intern_mode) = match tcx.static_mutability(def_id) {
let (base_mutability, base_intern_mode) = match tcx.static_mutability(def_id) {
Some(hir::Mutability::MutImmutable) => (Mutability::Immutable, InternMode::Static),
None => (Mutability::Immutable, InternMode::ConstBase),
// `static mut` doesn't care about interior mutability, it's mutable anyway
Some(hir::Mutability::MutMutable) => (Mutability::Mutable, InternMode::Static),
};

This comment was marked as resolved.

Copy link
@oli-obk

oli-obk Aug 28, 2019

Contributor

for promoteds we also run into the None branch, as the DefId is of the function of the promoted, which doesn't have static_mutability.

The only way we cannot get ConstBase is by the DefId pointing to a static. So not sure what happens to non-Copy array initializers, as that's promotion happening inside a static

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

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

@Alexendoo

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Ping from triage, any updates? @RalfJung @oli-obk

@RalfJung

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

I'm on vacation, will rebase when I have time.

Other than that I think I addressed all comments, though this question here for @oli-obk and @eddyb is still open.

@RalfJung RalfJung force-pushed the RalfJung:intern branch from 045d9fa to 5462ecb Sep 15, 2019

@RalfJung

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2019

Rebased, ready for review.

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

📌 Commit 5462ecb has been approved by oli-obk

Centril added a commit to Centril/rust that referenced this pull request Sep 16, 2019
Rollup merge of rust-lang#63955 - RalfJung:intern, r=oli-obk
Make sure interned constants are immutable

This makes sure that interning for constants (not statics) creates only immutable allocations.

Previously, the "main" allocation of `const FOO: Cell<i32> = Cell::new(0);` was marked as mutable, but I don't think we want that. It can be only copied, not written to.

Also, "leftover" allocations (behind raw pointers etc) were left mutable. I don't think we want to support that. I tried asserting that these are all already immutable (to double-check our static checks), but that failed in this one:
```rust
const NON_NULL_PTR2: NonNull<u8> = unsafe { mem::transmute(&0) };
```
Seems like maybe we want more precise mutability annotation inside Miri for locals (like `&0` here) so that this would actually become immutable to begin with?

I also factored `intern_shallow` out of the visitor so that we don't have to construct a visitor when we do not plan to visit anything. That confused me at first.
bors added a commit that referenced this pull request Sep 16, 2019
Auto merge of #64510 - Centril:rollup-m03zsq8, r=Centril
Rollup of 10 pull requests

Successful merges:

 - #63955 (Make sure interned constants are immutable)
 - #64028 (Stabilize `Vec::new` and `String::new` as `const fn`s)
 - #64119 (ci: ensure all tool maintainers are assignable on issues)
 - #64444 (fix building libstd without backtrace feature)
 - #64446 (Fix build script sanitizer check.)
 - #64451 (when Miri tests are not passing, do not add Miri component)
 - #64467 (Hide diagnostics emitted during --cfg parsing)
 - #64497 (Don't print the "total" `-Ztime-passes` output if `--prints=...` is also given)
 - #64499 (Use `Symbol` in two more functions.)
 - #64504 (use println!() instead of println!(""))

Failed merges:

r? @ghost

@bors bors merged commit 5462ecb into rust-lang:master Sep 16, 2019

4 checks passed

pr Build #20190915.12 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details

@RalfJung RalfJung deleted the RalfJung:intern branch Sep 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.