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

Add Rvalue-static-promotion RFC #1414

Merged
merged 4 commits into from
Jan 6, 2017

Conversation

Kimundi
Copy link
Member

@Kimundi Kimundi commented Dec 18, 2015

Promote constexpr rvalues to values in static memory instead of stack slots, and expose those in the language by being able to directly create 'static references to them.

This would allow code like let x: &'static u32 = &42 to work.

Rendered

Credit for idea and impl goes to @nikomatsakis and @eddyb ;)

Closes #827

[motivation]: #motivation

Right now, when dealing with constant values, you have to explicitly define
`const` or `static` items to create references with `'static` lifetime,
Copy link
Member

Choose a reason for hiding this comment

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

This would I believe actually be a bit more powerful than the current workaround with consts and statics since those can't be parameterized over type parameters of the e.g. function they're defined in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, indeed, you can do stuff like &None::<T> with this.

Copy link
Member

Choose a reason for hiding this comment

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

For example, &None::<T> could be returned even if T were a type parameter, or &[T; 0] (but the array case is actually hardcoded from before any attempts at rvalue promotions).

@eddyb
Copy link
Member

eddyb commented Dec 18, 2015

In the MIR, rvalues end up in temporary "lvalues" (as opposed to variable lvalues).
So when we make promotion-to-'static work for the MIR, we can even promote variables, too, if they're eligible (truly immutable, no UnsafeCell).

There are two ways this could be taken further with zero-sized types:

1. Remove the `UnsafeCell` restriction if the type of the rvalue is zero-sized.
2. The above, but also remove the __constexpr__ restriction, applying to any zero-sized rvalue instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

The restriction about not having a destructor should probably stay even for zero-sized types.

Copy link
Member

Choose a reason for hiding this comment

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

Currently types with destructors are not zero-sized but that will change and I agree - there is no reason not to be conservative, for now, IMHO.

For the specific case of destructors, remember that the following are more or less equal:

let x = &EmptyDestructible;
...
let temp = EmptyDestructible;
{
    let x = &temp;
    ...
}
Drop::drop(&mut temp)

Thus, rvalue drops can be considered "mutable accesses".
And even if the destructor doesn't have any bytes to actually mutate, it has to run.
So you would be changing behavior if you remove the destructor call, and otherwise you kind of make a mess out of borrow semantics by allowing the resulting &'static EmptyDestructible to escape the scope its destructor ran in.

In the end, no UB can actually occur, but it still seems troubling to do this dance around destructors.

Copy link
Contributor

Choose a reason for hiding this comment

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

We want to use value-destructor-freedom rather than type-destructor-freedom here.

@steveklabnik
Copy link
Member

I am concerned about this being breaking behavior; I'm not sure what would cause it, exactly, but &42 being a pointer to the stack is at least well-documented behavior.

@petrochenkov
Copy link
Contributor

Consider this function:

fn f() -> *const i32 {
   let a = &10;
   a
}

What type does a have? &'static i32?
Is pointer returned by f valid?
Now consider another function:

fn f() {
   let a = &10;
   use_pure(a);
}

The pointer to 10 doesn't escape from f. Does 10 still have to be kept in static memory or it may be optimized (placed into a code instruction as immediate value, for example)? Does LLVM perform this escape analysis and optimization automatically or Rust front-end will have to do something for it to happen?

@eddyb
Copy link
Member

eddyb commented Dec 18, 2015

@steveklabnik AFAIK no stable (1.x) Rust version has been released where &42 points at the stack: the implementation of rvalue promotion has been around since January or so.
So if there is any documentation claiming references of rvalues always point at the stack, that documentation is outdated and wrong.

@steveklabnik
Copy link
Member

steveklabnik commented Dec 18, 2015 via email

@eddyb
Copy link
Member

eddyb commented Dec 18, 2015

@petrochenkov In the existing (1.x stable and current nightly) compilers, &10 always points at .rodata, no matter what.

It goes deeper than that: let x = constexpr; lowers to effectively let x = *&constexpr;.
There are two exceptions: immediates are loaded directly by rustc_trans (allowing LLVM to remove the global holding the constant) and [constexpr; n] are instantiated in-place because that's more efficient than copying the whole array from .rodata.
This is not unlike what clang does for struct literals.

I assume your performance concerns are about the following scenarios (represented in pseudo-machine-code):

*stack = 10
arg0 = stack
call use_pure
arg0 = rodata_ptr_to_10
call use_pure

You are correct that in variable-length encodings, storing a byte to memory and doing a register-to-register copy can be shorter than placing a constant pointer into a register.

But is it faster? Even if the stack is in cache (which it should be), the memory store is not free.
OTOH, rodata_ptr_to_10 may not be in cache, causing a penalty on the first access.

AFAIK LLVM doesn't do that kind of escape analysis at this moment.
Or if it does, I haven't seen it demote globals to allocas.

@petrochenkov
Copy link
Contributor

@eddyb
Interesting, thanks.
It looks like LLVM indeed does the optimization. In this example the constant A presents in unoptimized IR but is inlined in optimized IR.

@eddyb
Copy link
Member

eddyb commented Dec 19, 2015

@petrochenkov That's because you're loading the value and not actually using the pointer itself.

@petrochenkov
Copy link
Contributor

@eddyb
I actually meant this case. If the pointer itself is used then it probably doesn't matter much where it points to (LLVM keeps it in rodata).

@glaebhoerl
Copy link
Contributor

Isn't "promoting to &'static mut T is OK if sizeof(T) = 0" just a special case of "&mut T: Copy is OK if sizeof(T) = 0"? It seems like the same reasoning - you can't violate memory safety if you never follow the pointer, no matter how many "aliases" exist - applies. That said, I wonder if there's user code which relies on the fact that &mut T is never Copy, regardless of T, to enforce some extra-lingual invariants? Does that (potentially) make any sense?

(This also feels eerily similar to "overlapping impls of trait Tr are OK if members(Tr) = 0"... I wonder if there's some deeper connection here.)

@nikomatsakis nikomatsakis self-assigned this Dec 22, 2015
@nikomatsakis
Copy link
Contributor

On Fri, Dec 18, 2015 at 03:02:03PM -0800, Eduard-Mihai Burtescu wrote:

@petrochenkov In the existing (1.x stable and current nightly) compilers, &10 always points at .rodata, no matter what.

It seems to me that the purpose of this RFC (and i've not had time to
read it yet...) is basically to expand (and perhaps define more
precisely) the set of expressions where you can safely rely on values
being moved into static slots, right?

@nikomatsakis
Copy link
Contributor

On Sat, Dec 19, 2015 at 06:06:14AM -0800, Gábor Lehel wrote:

Isn't "&'static mut T is OK if sizeof(T) = 0" just a special case of "&mut T: Copy is OK if sizeof(T) = 0"? It seems like the same reasoning - you can't violate memory safety if you never follow the pointer, no matter how many "aliases" exist - applies. That said, I wonder if there's user code which relies on the fact that &mut T is never Copy, regardless of T, to enforce some extra-lingual invariants? Does that (potentially) make any sense?

Indeed, this kind of special-case reasoning around zero-sized types
makes me sort of uncomfortable. I feel like it's something one is
likely to forget about when reasoning "in the abstract".

@eddyb
Copy link
Member

eddyb commented Dec 22, 2015

@nikomatsakis My point is that there won't be any surprise because the compiler already does this promotion so unsafe code couldn't have relied on &10 to ever point to the stack, unless it was from before 1.0.

@nikomatsakis
Copy link
Contributor

On Tue, Dec 22, 2015 at 09:09:40AM -0800, Eduard-Mihai Burtescu wrote:

@nikomatsakis My point is that there won't be any surprise because the compiler already does this promotion so unsafe code couldn't have relied on &10 to ever point to the stack, unless it was from before 1.0.

Sure, though conceivably it might rely on [22;22] being on the
stack. But in general it seems like we should clarify a range of
expressions where we MAY promote to a static if we so choose, and a
set where we WILL promote. The latter are the set where you can have
lifetime 'static. The former are an optimization.

@eddyb
Copy link
Member

eddyb commented Dec 22, 2015

You're referring to the same thing as my previous comment, right?

Something along the lines of:

let value = 5;
do_something_with(&value); // fine even if promoted
unsafe {
    // happens to work now, page fault if promoted
    *transmute::<&i32, &mut i32>(&value) = 6;
}

@nrc nrc added T-lang Relevant to the language team, which will review and decide on the RFC. T-compiler Relevant to the compiler team, which will review and decide on the RFC. labels Jan 4, 2016
@eddyb
Copy link
Member

eddyb commented Feb 14, 2016

FWIW, in two (1 and 2) places in trans::expr, #1229 resulted in doing the wrong thing when debug assertions are turned off (i.e. Release mode): &(255_u8 + 1) will compile with a warning and point to the stack, instead of .rodata, as it otherwise would (if it wasn't overflowing).
This has to be fixed before rvalue promotion is turned on.

@sfackler
Copy link
Member

What is the status of this?

@nikomatsakis
Copy link
Contributor

cc @rust-lang/lang -- I'd like to see this RFC go forward! Most of the infrastructure in the compiler is laid, and I think the core idea is sound. The key is that when you borrow a constant (e.g., &42) that meets the criteria in the RFC, this will be allocated into static memory, and hence we can assign a &'static 42 lifetime. This eliminates annoying (and unnecessary) errors in practice and also rationalizes some of our other typing rules (e.g., &[] has type &'static [T] for some T).

Reading the thread, I think the major concerns have to do with the runtime guarantees. Since the runtime behavior does not (and should not) depend on lifetime inference, this implies that &42 is guaranteed to be in static memory. You can imagine safe code relying on this. That said, this ought not to be a breaking change -- for optimization reasons, this is largely true today in any case, so it's not really a breaking change -- we just use conservative typing rules. But also, this seems to come back to the unsafe code guidelines as well. =)

That said, I still haven't had time to do a detailed review of the rules -- I have the printout here in front of me though and plan to do it now. But my feeling is that while we may find some minor detail to correct, we should go forward with the general idea.

@rfcbot fcp merge

@nikomatsakis nikomatsakis removed the T-compiler Relevant to the compiler team, which will review and decide on the RFC. label Sep 21, 2016
@nikomatsakis
Copy link
Contributor

Ah, one thought I had that I should raise. This text in the RFC itself:

This workaround also has the limitation of not being able to refer to type parameters of a containing generic functions, eg you can't do this:

fn generic<T>() -> &'static Option<T> {
    const X: &'static Option<T> = &None::<T>;
    X
}

Seems to suggest we ought to also just enable generic statics/constants already. I used to think "that makes no sense!" -- at least for statics -- but now I realize it's just another case of monomorphization of course. That said, it's prob better to defer this to a second RFC.

@rfcbot
Copy link
Collaborator

rfcbot commented Sep 21, 2016

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

create a `&'static mut` to a zero-sized type without involving unsafe code:

```rust
fn return_fn_mut_or_default(&mut self) -> &FnMut(u32, u32) -> u32 {
Copy link
Member

Choose a reason for hiding this comment

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

Is this signature a typo? In particular, did you mean to write that the return value is a &mut FnMut(u32, u32) -> u32 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, that's a typo!

@pnkfelix
Copy link
Member

pnkfelix commented Oct 13, 2016

rfcbot concern I am a little uneasy about the basis for supporting &'static mut of a zero-sized type. Maybe I just am not understanding the expected use case.

(update: sorry for repeating myself; I'm still trying to figure out how to use the rfcbot...; I put a properly constructed comment down below.)

@eddyb
Copy link
Member

eddyb commented Oct 13, 2016

@pnkfelix I agree, in fact, the way it's implemented right now we don't take advantage of it in most cases.
However, do keep in mind &mut [] is already 'static and would remain a special-case.

@eddyb
Copy link
Member

eddyb commented Nov 12, 2016

@rfcbot resolved basis-for-static-mut

(@dikaiosune does this work yet?)

@aturon
Copy link
Member

aturon commented Dec 13, 2016

@eddyb I think only the original comment author can resolve their concern.

@pnkfelix
Copy link
Member

@rfcbot resolved basis-for-static-mut

@pnkfelix
Copy link
Member

@rfcbot reviewed

@rfcbot
Copy link
Collaborator

rfcbot commented Dec 21, 2016

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Dec 21, 2016
@nrc nrc merged commit db24440 into rust-lang:master Jan 6, 2017
@nrc
Copy link
Member

nrc commented Jan 6, 2017

This has been in FCP for 16 days and there have been no new comments. Merging.

@briansmith
Copy link

Does the UnsafeCell stuff also need to be done for atomic types, since they seem to have the same issues?

@sfackler
Copy link
Member

AtomicFoo is already built around UnsafeCell: https://doc.rust-lang.org/src/core/sync/atomic.rs.html#91-93

@eddyb
Copy link
Member

eddyb commented Feb 20, 2017

Atomic types use UnsafeCell inside, there is no other sound way.

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 18, 2017
Add feature gate for rvalue-static-promotion

Probably needs more tests (which ones?) and there may be other things that need to be done. Also not sure whether the version that introduces the flag is really `1.15.1`.

See rust-lang/rfcs#1414.

Updates rust-lang#38865.
arielb1 pushed a commit to arielb1/rust that referenced this pull request Mar 18, 2017
Add feature gate for rvalue-static-promotion

Probably needs more tests (which ones?) and there may be other things that need to be done. Also not sure whether the version that introduces the flag is really `1.15.1`.

See rust-lang/rfcs#1414.

Updates rust-lang#38865.
arielb1 pushed a commit to arielb1/rust that referenced this pull request Mar 19, 2017
Add feature gate for rvalue-static-promotion

Probably needs more tests (which ones?) and there may be other things that need to be done. Also not sure whether the version that introduces the flag is really `1.15.1`.

See rust-lang/rfcs#1414.

Updates rust-lang#38865.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 19, 2017
Add feature gate for rvalue-static-promotion

Probably needs more tests (which ones?) and there may be other things that need to be done. Also not sure whether the version that introduces the flag is really `1.15.1`.

See rust-lang/rfcs#1414.

Updates rust-lang#38865.
@Centril Centril added A-typesystem Type system related proposals & ideas A-borrowck Borrow checker related proposals & ideas A-machine Proposals relating to Rust's abstract machine. A-const-eval Proposals relating to compile time evaluation (CTFE). A-value-promotion Proposals relating to promotion of (r)values. labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrowck Borrow checker related proposals & ideas A-const-eval Proposals relating to compile time evaluation (CTFE). A-machine Proposals relating to Rust's abstract machine. A-typesystem Type system related proposals & ideas A-value-promotion Proposals relating to promotion of (r)values. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

&[1, 2, 3] should always be &'static [<generic integer>]