Stream: t-compiler/const-eval

Topic: promoteds are not places


oli (Dec 07 2019 at 13:10, on Zulip):

@spastorino is working on making implicit promoteds not be a mir::Place anymore, but just an Operand::Constant by having the MIR of the promoted essentially be a reference to what used to be the value. Now the user may be writing e.g. &[(SOME_CELL, 42)][i].1 and this would get promoted, because we'd statically know that no thing with interior mutability will ever be available to the user without some serious unsafe code (probably fine in stacked borrows though, afaict).
Now this means that our promoted is actually &[(SOME_CELL, 42)] and the MIR using it will do the &promoted[i].1 projection because the index may be a runtime value.

This all works on stable right now, so we can't suddenly start erroring and figure this out later. The problem is that now there are promoteds of &[(Cell<T>, i32); 1] type, which interning will correctly reject as an invalid constant. Interning did not reject this before because we had [(Cell<T>, i32); 1] which is a legal constant. The only way I see right now on how to fix this is to add a field to mir::Body that is read before https://github.com/rust-lang/rust/blob/0a953cd9aa8b4c8b821bc672a4408900758e7e63/src/librustc_mir/const_eval.rs#L171 and passed to that function in order to make interning only validate the given projection (including all possible indices for Index projections).

One good thing that would come out of all of this is that we can now mark any promoted's memory as immutable, even if it technically contains mutable memory (at which point miri would also detect it if the user did unsafe shenanigans to get at the cell that they don't have a pointer to).

cc @RalfJ @eddyb

oli (Dec 07 2019 at 14:13, on Zulip):

cc @Santiago Pastorino

RalfJ (Dec 07 2019 at 15:55, on Zulip):

Now the user may be writing e.g. &[(SOME_CELL, 42)][i].1 and this would get promoted

can we please not expand the scope of what is being promoted? we are already promoting way too much and really shouldn't get feature creep here.

oli (Dec 07 2019 at 15:57, on Zulip):

We aren't expanding anything

RalfJ (Dec 07 2019 at 15:57, on Zulip):

oh wait we already accept the bad thing? "this would get promoted" sounds like we are not currently promoting it

oli (Dec 07 2019 at 15:57, on Zulip):

This works on stable

RalfJ (Dec 07 2019 at 15:57, on Zulip):

the hypothetical "would" is confusing me then

oli (Dec 07 2019 at 15:57, on Zulip):

Well it does get promoted

RalfJ (Dec 07 2019 at 15:58, on Zulip):

dang :(

oli (Dec 07 2019 at 15:58, on Zulip):

But after the PR it currently ICEs

RalfJ (Dec 07 2019 at 15:58, on Zulip):

any chance we could not accept that any more (crater)?

oli (Dec 07 2019 at 15:59, on Zulip):

Nope

oli (Dec 07 2019 at 15:59, on Zulip):

Been around forever

RalfJ (Dec 07 2019 at 15:59, on Zulip):

well time for Rust 2.0 then... :P

oli (Dec 07 2019 at 16:00, on Zulip):

Aaanyway

oli (Dec 07 2019 at 16:00, on Zulip):

Can we get back to trying to figure out how to fix it

RalfJ (Dec 07 2019 at 16:00, on Zulip):

so in that &[(SOME_CELL, 42)][i].1, i is a runtime value?

oli (Dec 07 2019 at 16:01, on Zulip):

Yes

oli (Dec 07 2019 at 16:01, on Zulip):

Eddyb suggested to rewire the promoteds value so it doesn't contain the field projection and instead make the array consist just of the fields

RalfJ (Dec 07 2019 at 16:02, on Zulip):

Been around forever

that doesn't mean this particular horror is actually used widely though

RalfJ (Dec 07 2019 at 16:02, on Zulip):

I'd classify this as a soundness fix TBH^^

oli (Dec 07 2019 at 16:04, on Zulip):

Why? It's totally sound

RalfJ (Dec 07 2019 at 16:04, on Zulip):

not if the client does ptr arithmetic to get to the other fields

RalfJ (Dec 07 2019 at 16:05, on Zulip):

that might be illegal but we haven't actually standardized anything along those lines yet

oli (Dec 07 2019 at 16:05, on Zulip):

Right, that's.... can they do that soundly?

RalfJ (Dec 07 2019 at 16:05, on Zulip):

and in particular for statics, Stacked Borrows is quite relaxed wrt aliasing. it has to be.

RalfJ (Dec 07 2019 at 16:06, on Zulip):

and then these are all shared refs so we dont even get auto-reborrows... I might try to weaponize this later.

RalfJ (Dec 07 2019 at 16:06, on Zulip):

in any case I don't have time to think deeply about this right now, sorry.
I'm also quite frustrated by how promotion is just an endless source of really bad behavior. :/

oli (Dec 07 2019 at 16:06, on Zulip):

Yea :frown:

oli (Dec 07 2019 at 17:00, on Zulip):

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=9d85c9e8194e3e61a78e3e657bfae216

oli (Dec 07 2019 at 17:01, on Zulip):

Miri seems to catch it

Christian Poveda (Dec 07 2019 at 18:01, on Zulip):

I'd classify this as a soundness fix TBH^^

I don't know if it is an exact precedent but didn't something like this happened when NLL got released?

RalfJ (Dec 08 2019 at 09:29, on Zulip):

Miri also seems to catch this variant: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=e4d683463eb85f63a74cef59a7aaf8a3
I am not entirely sure why though. And anyway both of these rely on particular details of Stacked Borrows, which is not sufficient -- Stacked Borrows is just an experiment.

RalfJ (Dec 08 2019 at 09:41, on Zulip):

So right now this happens:

[INFO  rustc_mir::interpret::step] _7 = &((promoted[0]: [(std::cell::Cell<i32>, i32); 1])[_10].1: i32)
[2019-12-08T09:35:44Z TRACE miri::stacked_borrows] New allocation AllocId(550) has base tag <2040>
[INFO  rustc_mir::interpret::step] Retag(_7)
[2019-12-08T09:35:44Z TRACE miri::stacked_borrows] reborrow: shared reference <2041> derived from <2040> (pointee i32): AllocId(550).0x4, size 4
[2019-12-08T09:35:44Z TRACE miri::stacked_borrows] reborrow: adding item [SharedReadOnly for <2041>]

IOW, there is a reborrow of the newly created shared ref, and under current Stacked Borrows that reborrowed ref cannot be used for anything else in that allocation. That, however, is a design choice of Stacked Borrows that is is conflict with a bunch of real code, and thus we are wondering if it can be relaxed.

RalfJ (Dec 08 2019 at 09:41, on Zulip):

I am not at all comfortable to rely on this check for our soundness argument here...

oli (Dec 08 2019 at 11:26, on Zulip):

Note that the given PR does not change anything in this regard. It just caused promoteds to not be places anymore and thus validation recognizing the problem (as interior mutability in the base alloc of a constant is ok, but now it's behind a reference).

RalfJ (Dec 11 2019 at 17:36, on Zulip):

well, changing the way we codegen that might change the way stacked borrows hands out tags

RalfJ (Dec 11 2019 at 17:37, on Zulip):

I am not entirely sure if Stacked Borrows will retag _2 = const {{ pointer }}, which AFAIK we are moving to

RalfJ (Dec 11 2019 at 17:37, on Zulip):

I think it does but when and where we retag beyond &/&mut operations is totally up in the air

RalfJ (Dec 18 2019 at 11:09, on Zulip):

is there an issue for this?

oli (Dec 18 2019 at 13:45, on Zulip):

no, I was planning on running miri before and after the change to see differences. I'll open an issue for that

oli (Dec 18 2019 at 13:50, on Zulip):

https://github.com/rust-lang/rust/issues/67395

RalfJ (Dec 22 2019 at 17:32, on Zulip):

sorry, should have been clearer... @oli that's not the issue I meant. I meant the part about validating the entire promoted etc., the concerns you raised here

oli (Dec 22 2019 at 19:31, on Zulip):

Done: https://github.com/rust-lang/rust/issues/67534

RalfJ (Dec 22 2019 at 19:52, on Zulip):

thanks!

nikomatsakis (Jan 06 2020 at 14:32, on Zulip):

So I was trying to read up on this issue and finding myself a bit confused. @oli, you're saying that we the "old" promotion would promote values of types like [(Cell<u32>, u32); 4].. these would get promoted to constants... but then we reference them, right, so we'd ultimately create a static of that type? And we would do this because we know that only the u32 would get accessed?

oli (Jan 06 2020 at 17:13, on Zulip):

Yes

oli (Jan 06 2020 at 17:13, on Zulip):

To both questions

Last update: Jan 28 2020 at 00:45UTC