Stream: t-compiler/wg-mir-opt

Topic: removing static from place


nikomatsakis (Oct 19 2019 at 00:47, on Zulip):

Hey @eddyb @oli @RalfJ --

So @Santiago Pastorino and I were talking about the "Place 2.0" work. Now that the interning of projections is basically done, we were talking about what further simplifications we could do. One thought that @eddyb mentioned, and which I think probably makes sense, is to remove "statics" from PlaceBase, so that all places begin with a local variable. This not only simplifies the definition of a "place", it also means we can reduce them to be (a) Copy and (b) 128 bits. :tada:

We wrote up a hackmd document containing a proposal for how to go about this -- please take a look and see what you think! It includes some rough details of how this would interact with the borrow checker, unsafe code, etc, as well as some possible implementation steps.

RalfJ (Oct 19 2019 at 09:53, on Zulip):

Ah, that's very interesting. I am not sure what exactly the interactions with Stacked Borrows will be but I don't feel the document gives them justice. :D but off the top of my head I think this is a good change, yes. This actually fixes some UB I was worried about with statics and Stacked Borrows :D

RalfJ (Oct 19 2019 at 09:53, on Zulip):

so in the light of the static mut discussion, is it correct that RefStatic would never be used for a mutable static?

RalfJ (Oct 19 2019 at 09:54, on Zulip):

that makes me wonder if it wouldn't be easier to also not have it for normal static

oli (Oct 19 2019 at 09:54, on Zulip):

I dislike the duplication of RefStatic and RawStatic. Maybe we could have a PlaceOrStatic type that we use for these specific operations?

RalfJ (Oct 19 2019 at 09:54, on Zulip):

we could just have only RawStatic

RalfJ (Oct 19 2019 at 09:55, on Zulip):

and desugar &mut S to &mut *RawStatic(S)

RalfJ (Oct 19 2019 at 10:03, on Zulip):

here are some incoherent Stacked Borrows thoughts (could someone add that to the document? I dont seem to have edit permissions):
This moves us towards a model where the static itself does not have a "tag" at all as it isn't a place. There's no "unique root identity" at the bottom of the borrow stack of a static. That's in strong contrast to locals where there's a unique tag at the bottom. This unique tag is what makes local = 5 kill all other pointers to that local. I thought the same should happen for statics and actually already did some implementation work along those lines in Miri (making sure we pick a consistent tag for the static place), but this proposal indicates to me that we maybe don't want a base identity for statics, and that STATIC = 5 should not kill all other pointers (including raw pointers)!

So, this is illegal:

let mut x: u32 = 0;
let ptr = &raw mut x;
x = 2;
let _val = *ptr; // UB

But this is allowed:

static mut X: u32 = 0;
let ptr = &raw mut x;
X = 2;
let _val = *ptr; // all is good

I'd be happy to remove the code I wrote to make statics more like locals, it's somewhat annoyingly complicated (and other people ran into that code like a brick wall when trying to implement shims for an extern static in Miri).

So, summary: The change seems fine to me. It does have some effects on Stacked Borrows, maybe more than you expected, but I think these are good effects. Basically it we could take this to mean that there is an inconsistency between static and locals both in Stacked Borrows and the rustc internal data structure, and it's nice to at least be consistent in these inconsistencies :D

eddyb (Oct 19 2019 at 11:12, on Zulip):

so you'd want to treat statics more like leaked global allocations?

eddyb (Oct 19 2019 at 11:13, on Zulip):

@nikomatsakis @RalfJ I think we can do this without any MIR btw :)

eddyb (Oct 19 2019 at 11:14, on Zulip):

AFAIK we can already represent a reference/raw pointer to a static in a ty::Const (I hope @oli or @RalfJ confirm this)

oli (Oct 19 2019 at 11:14, on Zulip):

omgz

oli (Oct 19 2019 at 11:14, on Zulip):

yes we can

eddyb (Oct 19 2019 at 11:14, on Zulip):

I forget when I first considered this and I'm sorry I didn't write it down before

eddyb (Oct 19 2019 at 11:15, on Zulip):

this is similar to not having an Rvalue for literals

oli (Oct 19 2019 at 11:16, on Zulip):

hmm... how does that fare with generic statics though

eddyb (Oct 19 2019 at 11:16, on Zulip):

in terms of stacked borrows, I believe this has the same effect as using RawStatic (except without needing a MIR reborrow if you want a reference)

eddyb (Oct 19 2019 at 11:16, on Zulip):

generic statics are impossible

oli (Oct 19 2019 at 11:16, on Zulip):

promoteds then

oli (Oct 19 2019 at 11:16, on Zulip):

same thing

eddyb (Oct 19 2019 at 11:17, on Zulip):

ooooh I forgot we made those use the same infrastructure ugh

oli (Oct 19 2019 at 11:17, on Zulip):

ah

oli (Oct 19 2019 at 11:17, on Zulip):

no we can fix that

oli (Oct 19 2019 at 11:17, on Zulip):

we just refer to them by value

oli (Oct 19 2019 at 11:17, on Zulip):

like the constant represents the promoted

eddyb (Oct 19 2019 at 11:18, on Zulip):

how do references to statics work in ty::Const today?

eddyb (Oct 19 2019 at 11:18, on Zulip):

is the Instance(-like) information in the ty::Const or behind some sort of ID?

eddyb (Oct 19 2019 at 11:19, on Zulip):

@oli so you'd want to revert promoteds back to not being places?

eddyb (Oct 19 2019 at 11:19, on Zulip):

I think we should do that first hmpf

eddyb (Oct 19 2019 at 11:20, on Zulip):

the only case where we rely on the place-like nature is the &promoted[runtime] replacement which can be &(*promoted)[runtime], I think?

eddyb (Oct 19 2019 at 11:21, on Zulip):

I guess we'd need to agree where we stick the Promoted back into

eddyb (Oct 19 2019 at 11:22, on Zulip):

unevaluated ty::Const perhaps?

oli (Oct 19 2019 at 12:17, on Zulip):

yes

centril (Oct 19 2019 at 14:48, on Zulip):

generic statics are impossible

What does that mean? Once we kill dylibs we could have generic statics, no?

oli (Oct 19 2019 at 14:49, on Zulip):

impossible right now

centril (Oct 19 2019 at 14:49, on Zulip):

that might not remain the case

oli (Oct 19 2019 at 14:50, on Zulip):

it will still be doable with this scheme

oli (Oct 19 2019 at 14:50, on Zulip):

actually it will be trivial

oli (Oct 19 2019 at 14:51, on Zulip):

much better than how it could be done right now

centril (Oct 19 2019 at 14:52, on Zulip):

cool

eddyb (Oct 20 2019 at 14:25, on Zulip):

I'm not sure you need to kill dylibs as much as you want some strong semantics from linkers

eddyb (Oct 20 2019 at 14:25, on Zulip):

to deduplicate based on... symbol name? at least I think that's correct

eddyb (Oct 20 2019 at 14:27, on Zulip):

at least with v0 mangling we were forced to encode the typesystem into symbol names

eddyb (Oct 20 2019 at 14:27, on Zulip):

I've always felt like generic/associated statics are impossible, given that nobody could give me an implementation strategy but I may have overlooked preexisting features

eddyb (Oct 20 2019 at 14:28, on Zulip):

there's ODR stuff for... C++ I think? I wonder if that works

Santiago Pastorino (Oct 23 2019 at 13:25, on Zulip):

hey @nikomatsakis @oli, I was talking to both of you in private about this

Santiago Pastorino (Oct 23 2019 at 13:25, on Zulip):

I have a couple of really minor next things to do related to mir 2.0

Santiago Pastorino (Oct 23 2019 at 13:25, on Zulip):

@oli told me that they are working on this specifically because there is some mess around

Santiago Pastorino (Oct 23 2019 at 13:26, on Zulip):

@oli can you elaborate that a bit more for @nikomatsakis ?

Santiago Pastorino (Oct 23 2019 at 13:26, on Zulip):

and then, question for you both, do you guys see something else that I can go after?

nikomatsakis (Oct 23 2019 at 13:27, on Zulip):

generic statics are impossible

this is just an arbitrary limitation, I don't think we should bank on it

nikomatsakis (Oct 23 2019 at 13:28, on Zulip):

(ah, ok, catching up)

nikomatsakis (Oct 23 2019 at 13:29, on Zulip):

I've always felt like generic/associated statics are impossible, given that nobody could give me an implementation strategy but I may have overlooked preexisting features

(ok, I agree, not entirely arbitrary)

nikomatsakis (Oct 23 2019 at 13:31, on Zulip):

I dislike the duplication of RefStatic and RawStatic. Maybe we could have a PlaceOrStatic type that we use for these specific operations?

so I guess I don't fully understand the plan you have in mind -- it sounds like you are planning to embed the &Foo or *mut Foo that references a static into a constant?

That said, let me explain why I added both RefStatic and RawStatic -- I agree you would only need the raw pointer version, but I was trying to avoid having &X.foo insert a "unsafe" (but blessed) operation. Here by unsafe I mean: something that the unsafe checker would flag. But I argee there's no fundamental reason to do that.

nikomatsakis (Oct 23 2019 at 13:43, on Zulip):

@RalfJ I'll add that to the doc -- I agree that we probably don't want a unique tag for statics. To me at least they feel "different"in that they are globally accessible.

nikomatsakis (Oct 23 2019 at 13:48, on Zulip):

In any case, it seems to me @Santiago Pastorino that regardless of what happens with RawStatic, RefStatic, etc, the first implementation step that I described in the document remains relevant:

Santiago Pastorino (Oct 23 2019 at 13:48, on Zulip):

ok, can try to do that

nikomatsakis (Oct 23 2019 at 13:50, on Zulip):

@oli can you perhaps update the proposal hackmd -- I've saved the RefStatic/RawStatic version as a named revision in hackmd, so feel free to edit away

oli (Oct 23 2019 at 14:59, on Zulip):

I have a branch that passes all compile-pass and run-pass tests since 5 hours ago

oli (Oct 23 2019 at 15:00, on Zulip):

I did not add RawStatic or RefStatic, but instead did the lowering of treating any use of STATIC like *&STATIC where the &STATIC is a constant

oli (Oct 23 2019 at 15:40, on Zulip):

Here's the branch: https://github.com/oli-obk/rust/pull/new/static_unification_theory_teil_eins

oli (Oct 23 2019 at 15:41, on Zulip):

To reintroduce all the checks around statics we've had before, I'll need to check all constants instead of the static places

nikomatsakis (Oct 24 2019 at 15:35, on Zulip):

Skimming the branch now but I feel like I'm somehow missing something

nikomatsakis (Oct 24 2019 at 15:35, on Zulip):

I guess the idea is that we when you have &S in the source code, you get X = Constant(&S) now?

nikomatsakis (Oct 24 2019 at 15:36, on Zulip):

and presumably something like let x = &S.foo becomes

TMP = Constant(&S)
X = &(*TMP).foo

where TMP: &typeof(S)

nikomatsakis (Oct 24 2019 at 15:36, on Zulip):

something like that, @oli?

nikomatsakis (Oct 24 2019 at 15:37, on Zulip):

(This does raise the question of #[thread_local] statics and the current errors that we give there, I see you removed that logic)

nikomatsakis (Oct 24 2019 at 15:37, on Zulip):

(But honestly that logic is a bit weird anyway)

oli (Oct 24 2019 at 15:37, on Zulip):

yea that's how it's implemented right now, and the logic is only removed in the WIP commit

nikomatsakis (Oct 24 2019 at 15:38, on Zulip):

Currently in your branch I see that there is still Promoted as part of a place

oli (Oct 24 2019 at 15:38, on Zulip):

that commit should probably be called TODO

nikomatsakis (Oct 24 2019 at 15:38, on Zulip):

is the plan to remove that?

oli (Oct 24 2019 at 15:38, on Zulip):

yes, promoteds will get reverted back to constants, too

nikomatsakis (Oct 24 2019 at 15:38, on Zulip):

yea that's how it's implemented right now, and the logic is only removed in the WIP commit

I don't know where it will be added back in

nikomatsakis (Oct 24 2019 at 15:38, on Zulip):

under this approach, from the borrow checker's point of view, it seems like borrows of statics are "invisible"

nikomatsakis (Oct 24 2019 at 15:39, on Zulip):

they're part of a constant, they just always existed

Santiago Pastorino (Oct 24 2019 at 15:39, on Zulip):

oh cool that you jumped in Niko, I'm gonna take over @oli's branch and continue

nikomatsakis (Oct 24 2019 at 15:39, on Zulip):

(that seems... ok to me, but it doesn't leave room for that particular thread-local logic)

Santiago Pastorino (Oct 24 2019 at 15:39, on Zulip):

so being all in the same page, seems good :)

oli (Oct 24 2019 at 15:39, on Zulip):

https://github.com/rust-lang/rust/commit/4e51411f71c0c8a2c04ee9031bd3da28bc6e515b is how I did it for const qualif

oli (Oct 24 2019 at 15:40, on Zulip):

I think this can be repeated in most places where I removed the static-place logic

nikomatsakis (Oct 24 2019 at 15:40, on Zulip):

I was afraid you were going to show me something like this :)

oli (Oct 24 2019 at 15:40, on Zulip):

hehehe

oli (Oct 24 2019 at 15:40, on Zulip):

it's mainly diagnostics

oli (Oct 24 2019 at 15:40, on Zulip):

I think everything works

nikomatsakis (Oct 24 2019 at 15:40, on Zulip):

I think what your'e doing is sort of special-casing the case of "a constant that is &S for a static"

oli (Oct 24 2019 at 15:40, on Zulip):

except that some diagnostics have regressed badly

nikomatsakis (Oct 24 2019 at 15:40, on Zulip):

it's mainly diagnostics

it's not

nikomatsakis (Oct 24 2019 at 15:40, on Zulip):

I mean, not for this borrow-checker case?

nikomatsakis (Oct 24 2019 at 15:40, on Zulip):

Like, it determines if we give an error or not

oli (Oct 24 2019 at 15:41, on Zulip):

right, for the thread local check it won't work by default I guess

oli (Oct 24 2019 at 15:41, on Zulip):

because you just have a reference

nikomatsakis (Oct 24 2019 at 15:41, on Zulip):

that said, I do like this approach :)

nikomatsakis (Oct 24 2019 at 15:41, on Zulip):

I guess we could do some hacky thing around the thread-local check, or just .. not do it

oli (Oct 24 2019 at 15:42, on Zulip):

loads of errors are now "can't do X with immutable reference" instead of "can't do X with immutable static"

nikomatsakis (Oct 24 2019 at 15:42, on Zulip):

yeah, no doubt

Santiago Pastorino (Oct 24 2019 at 15:42, on Zulip):

pardon my ignorance but what's the thread-local check you're talking about?

nikomatsakis (Oct 24 2019 at 15:42, on Zulip):

for that kind of thing I think maybe the approach you're using is ok, or else we should maybe just find a "middle wording"

oli (Oct 24 2019 at 15:42, on Zulip):

that's why I did the const qualif change, to improve diagnostics again

nikomatsakis (Oct 24 2019 at 15:42, on Zulip):

I don't love our wording anyway tbh

nikomatsakis (Oct 24 2019 at 15:42, on Zulip):

I've always though we give far too many details

nikomatsakis (Oct 24 2019 at 15:42, on Zulip):

that are often only semi-relevant

nikomatsakis (Oct 24 2019 at 15:43, on Zulip):

besides the term "immutable reference" is in my view just wrong :P

nikomatsakis (Oct 24 2019 at 15:43, on Zulip):

(but obviously this is a separate concern...)

oli (Oct 24 2019 at 15:43, on Zulip):

yea, any ideas on how to unify this and use less specialized diagnostics are very welcome :D

nikomatsakis (Oct 24 2019 at 15:43, on Zulip):

pardon my ignorance but what's the thread-local check you're talking about?

this compiles

static S: u32;

fn foo() -> &'static u32 {
    &S
}
nikomatsakis (Oct 24 2019 at 15:43, on Zulip):

this does not

#[thread_local]
static S: u32;

fn foo() -> &'static u32 {
    &S
}
nikomatsakis (Oct 24 2019 at 15:44, on Zulip):

#[thread_local] is an unstable feature in any case

nikomatsakis (Oct 24 2019 at 15:44, on Zulip):

and not one with a clear path to stabilization, never RFC'd, etc

oli (Oct 24 2019 at 15:44, on Zulip):

heh, I have an idea

Santiago Pastorino (Oct 24 2019 at 15:44, on Zulip):

:+1:

oli (Oct 24 2019 at 15:44, on Zulip):

for thread locals

nikomatsakis (Oct 24 2019 at 15:44, on Zulip):

the reason the second one does not compile is because maybe foo is the "main" of a thread, I think

oli (Oct 24 2019 at 15:44, on Zulip):

the code that creates the constant (in the first commit), could detect that it's a thread local and use a function local lifetime for the reference

nikomatsakis (Oct 24 2019 at 15:45, on Zulip):

that seems weird too -- a constant with a lifetime that is local to the fn?

nikomatsakis (Oct 24 2019 at 15:45, on Zulip):

also, that's not even meaningful

nikomatsakis (Oct 24 2019 at 15:45, on Zulip):

i.e., we literally erase all the lifetimes at the start of NLL

oli (Oct 24 2019 at 15:45, on Zulip):

/me does not know a thing about borrowck

nikomatsakis (Oct 24 2019 at 15:45, on Zulip):

and recompute them from scratch

oli (Oct 24 2019 at 15:45, on Zulip):

oh :frown:

nikomatsakis (Oct 24 2019 at 15:45, on Zulip):

in any case I agree we could hack something up, but it feels...like part of the elegance of this approach comes from not needing a special case :)

nikomatsakis (Oct 24 2019 at 15:46, on Zulip):

I think we should ignore that for now

nikomatsakis (Oct 24 2019 at 15:46, on Zulip):

and continue with your approach

nikomatsakis (Oct 24 2019 at 15:46, on Zulip):

once everything else seems good, we can worry about #[thread_local] and the best way to re-encode that

oli (Oct 24 2019 at 15:47, on Zulip):

so you mean if we changed the wording in const qualif to not refer to references or statics, we could remove all the special casing for statics and just report "cannot mutate immutable value" or sth

nikomatsakis (Oct 24 2019 at 15:47, on Zulip):

right

oli (Oct 24 2019 at 15:47, on Zulip):

yea, for now I'd rather make the PR

oli (Oct 24 2019 at 15:47, on Zulip):

not change behaviour

oli (Oct 24 2019 at 15:47, on Zulip):

and then have follow-up PRs

oli (Oct 24 2019 at 15:47, on Zulip):

(maybe once promoteds are also not places anymore)

nikomatsakis (Oct 24 2019 at 15:48, on Zulip):

yes, it's just kind of annoying if we have to add a bunch of logic and then we wind up removing it. but it seems better to not bring together changing the error message and changing MIR

nikomatsakis (Oct 24 2019 at 15:48, on Zulip):

I don't honestly remember what const-qualif even does

nikomatsakis (Oct 24 2019 at 15:48, on Zulip):

I guess it gives errors for doing things in constants you should not do?

oli (Oct 24 2019 at 15:48, on Zulip):

ensure that we don't have Cell constants behind references

nikomatsakis (Oct 24 2019 at 15:48, on Zulip):

(I don't have a strong opinion about its error messages)

oli (Oct 24 2019 at 15:48, on Zulip):

and fancy variants of that

nikomatsakis (Oct 24 2019 at 15:48, on Zulip):

yeah ok

nikomatsakis (Oct 24 2019 at 15:48, on Zulip):

that's sort of what I imagined

oli (Oct 24 2019 at 15:49, on Zulip):

oh, const qualif does need the static check

oli (Oct 24 2019 at 15:49, on Zulip):

because it also ensures that constants can't refer to statics

nikomatsakis (Oct 24 2019 at 15:52, on Zulip):

ps I think the changes to const qualif might be fine -- I'm not as familiar with the code there --

nikomatsakis (Oct 24 2019 at 15:52, on Zulip):

one thing I do wonder though

nikomatsakis (Oct 24 2019 at 15:52, on Zulip):

if the user writes

static FOO: u32;
const BAR: &u32 = &FOO;
nikomatsakis (Oct 24 2019 at 15:52, on Zulip):

presumably const-qualif can see the difference between BAR and the new constants we are introducing?

oli (Oct 24 2019 at 15:56, on Zulip):

yes

Last update: Nov 17 2019 at 06:55UTC