Make sure interned constants are immutable by RalfJung · Pull Request #63955 · rust-lang/rust

RalfJung · 2019-08-27T19:25:55Z

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 · 2019-08-27T19:25:58Z

r? @matthewjasper

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

RalfJung · 2019-08-27T19:26:03Z

r? @oli-obk

src/librustc_mir/const_eval.rs

RalfJung · 2019-08-27T19:27:43Z

src/librustc_mir/interpret/intern.rs

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

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?

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

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?

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")?

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

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

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

"explict static vs not"?

src/librustc_mir/interpret/intern.rs

src/test/ui/consts/miri_unleashed/mutable_const.rs

bors · 2019-09-03T02:02:25Z

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

Alexendoo · 2019-09-11T17:03:28Z

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

RalfJung · 2019-09-11T17:11:19Z

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.

… always intern constants as immutable

… visiting

RalfJung · 2019-09-15T12:29:26Z

Rebased, ready for review.

oli-obk · 2019-09-16T12:42:52Z

@bors r+

bors · 2019-09-16T12:42:53Z

📌 Commit 5462ecb has been approved by 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.

@ghost

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

rust-highfive assigned matthewjasper Aug 27, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 27, 2019

rust-highfive assigned oli-obk and unassigned matthewjasper Aug 27, 2019

RalfJung commented Aug 27, 2019

View reviewed changes

src/librustc_mir/const_eval.rs Outdated Show resolved Hide resolved

RalfJung commented Aug 27, 2019

View reviewed changes

oli-obk reviewed Aug 28, 2019

View reviewed changes

src/librustc_mir/interpret/intern.rs Outdated Show resolved Hide resolved

src/librustc_mir/interpret/intern.rs Outdated

This comment was marked as resolved.

Sign in to view

RalfJung commented Aug 28, 2019

View reviewed changes

src/test/ui/consts/miri_unleashed/mutable_const.rs Outdated Show resolved Hide resolved

oli-obk reviewed Aug 28, 2019

View reviewed changes

src/test/ui/consts/miri_unleashed/mutable_const.rs Outdated Show resolved Hide resolved

This was referenced Aug 28, 2019

Heap allocations in constants rust-lang/const-eval#20

Open

Towards dynamic const-qualify rust-lang/const-eval#27

Merged

update UB rust-lang/reference#659

Merged

RalfJung mentioned this pull request Sep 14, 2019

Refactor the MirPass for QualifyAndPromoteConstants #63994

Merged

RalfJung added 8 commits September 15, 2019 13:57

const interning: move mutability computation into intern_shallow, and…

6137691

… always intern constants as immutable

all memory behind a constant must be immutable

7b8693e

drop redundant ParamEnv, and avoid constructing InternVisitor without…

75c82b4

… visiting

relocations -> allocations

b1d4d2b

note a FIXME

14e3506

assert that nobody asks for mutable constants

3424811

explain ty == None

224e2e5

update intern classification comment

5462ecb

RalfJung force-pushed the intern branch from 045d9fa to 5462ecb Compare September 15, 2019 12:28

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 16, 2019

Centril mentioned this pull request Sep 16, 2019

Rollup of 10 pull requests #64510

Merged

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

RalfJung deleted the intern branch September 17, 2019 07:28

Uh oh!

Conversation

RalfJung commented Aug 27, 2019

Uh oh!

rust-highfive commented Aug 27, 2019

Uh oh!

RalfJung commented Aug 27, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Aug 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

bors commented Sep 3, 2019

Uh oh!

Alexendoo commented Sep 11, 2019

Uh oh!

RalfJung commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Sep 15, 2019

Uh oh!

oli-obk commented Sep 16, 2019

Uh oh!

bors commented Sep 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

RalfJung Aug 29, 2019 •

edited

Loading

RalfJung commented Sep 11, 2019 •

edited

Loading