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

Implement constant support in MIR. #33130

Merged
merged 12 commits into from
May 8, 2016
Merged

Implement constant support in MIR. #33130

merged 12 commits into from
May 8, 2016

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Apr 21, 2016

All of the intended features in trans::consts are now supported by mir::constant.
The implementation is considered a temporary measure until miri replaces it.

A -Z orbit bootstrap build will only translate LLVM IR from AST for #[rustc_no_mir] functions.

Furthermore, almost all checks of constant expressions have been moved to MIR.
In non-const functions, trees of temporaries are promoted, as per RFC 1414 (rvalue promotion).
Promotion before MIR borrowck would allow reasoning about promoted values' lifetimes.

The improved checking comes at the cost of four [breaking-change]s:

  • repeat counts must contain a constant expression, e.g.:
    let arr = [0; { println!("foo"); 5 }]; used to be allowed (it behaved like let arr = [0; 5];)
  • dereference of a reference to a static cannot be used in another static, e.g.:
    static X: [u8; 1] = [1]; static Y: u8 = (&X)[0]; was unintentionally allowed before
  • the type of a static must be Sync, irrespective of the initializer, e.g.
    static FOO: *const T = &BAR; worked as &T is Sync, but it shouldn't because *const T isn't
  • a static cannot wrap UnsafeCell around a type that may need drop, e.g.
    static X: MakeSync<UnsafeCell<Option<String>>> = MakeSync(UnsafeCell::new(None));
    was previously allowed based on the fact None alone doesn't need drop, but in UnsafeCell
    it can be later changed to Some(String) which does need dropping

The drop restrictions are relaxed by RFC 1440 (#33156), which is implemented, but feature-gated.
However, creating UnsafeCell from constants is unstable, so users can just enable the feature gate.

@rust-highfive
Copy link
Collaborator

r? @arielb1

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

@oli-obk
Copy link
Contributor

oli-obk commented Apr 21, 2016

I don't understand the reason for the existance of ce084d5

Won't all of that code go away once miri does the interpretation?

@eddyb
Copy link
Member Author

eddyb commented Apr 21, 2016

Yes it would, I thought it was going to be less code. I just wanted to make sure that we can work independently of old trans, no matter what happens with miri integration.

@eddyb eddyb force-pushed the mir-const branch 2 times, most recently from 0f3c6aa to 17da918 Compare April 21, 2016 13:45
impl<'a, 'tcx> Visitor<'tcx> for BuildMir<'a, 'tcx> {
fn visit_fn(&mut self,
fk: intravisit::FnKind<'tcx>,
decl: &'tcx hir::FnDecl,
body: &'tcx hir::Block,
span: Span,
id: ast::NodeId) {
let implicit_arg_tys = if let intravisit::FnKind::Closure(..) = fk {
vec![closure_self_ty(&self.tcx, id, body.id)]
// fetch the fully liberated fn signature (that is, all bound
Copy link
Contributor

@arielb1 arielb1 Apr 21, 2016

Choose a reason for hiding this comment

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

Shouldn't you .subst(param_env.free_substs)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the same code as before, just moved around.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's the Correct way

@nikomatsakis
Copy link
Contributor

cc @rust-lang/compiler

@nikomatsakis
Copy link
Contributor

I'd like to sign off one this before it is r+'d. Will try to look today.

@@ -16,7 +16,7 @@ impl std::ops::Neg for S {
fn neg(self) -> u32 { 0 }
}

const _MAX: usize = -1;
const _MAX: usize = -(2 - 1);
Copy link
Member

Choose a reason for hiding this comment

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

??

Copy link
Member Author

Choose a reason for hiding this comment

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

MIR errors while building while the test expects an error emitted during lint-checking (but not suppressible AFAICT), and the driver stops on errors somewhere in between.

Copy link
Member

Choose a reason for hiding this comment

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

Should add a FIXME comment to return to the original code once it is possible (i.e. the current consteval code is removed).

@bors
Copy link
Contributor

bors commented Apr 24, 2016

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

@eddyb eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Apr 26, 2016
@eddyb eddyb force-pushed the mir-const branch 3 times, most recently from f9a4c18 to f68fcaf Compare April 27, 2016 02:03
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 27, 2016

Crater run: https://gist.github.com/nikomatsakis/c2c7ecdac8437dc8056b6629ec7e9f83

  • 4063 crates tested: 2631 working / 1322 broken / 3 regressed / 0 fixed / 107 unknown.

@eddyb
Copy link
Member Author

eddyb commented Apr 27, 2016

@nikomatsakis one is a timeout, one is a non-Sync static (I... didn't do anything intentional there, how could that have escaped checking before?) and one (odbc) is a true regression:

src/raw/sqlconsts.rs:272:48: 272:71 error: cannot refer to other statics by value, use the address-of operator or a cons
tant instead [E0394]                                                                                                    
src/raw/sqlconsts.rs:272 pub static SQL_MAXIMUM_USER_NAME_LENGTH: u16 = *&SQL_MAX_USER_NAME_LEN;                        
                                                                        ^~~~~~~~~~~~~~~~~~~~~~~ 

Followed by 22 more of these, which appear to be aliases (use could've worked!) of other statics which should also be const instead.
I consider this to be pathological abuse of a lack of rigor in our checking and I don't think there's a way to have a warning cycle - besides, it can't compile with MIR trans.

I will personally try to fix these two crates, I hope we can merge this soon, since it's blocking both miri and some of my other 'tcx-related work.

@eddyb
Copy link
Member Author

eddyb commented Apr 27, 2016

Actually, the misuse of static from obdc was easy to fix, see Koka/odbc-rs#1 (already merged).

However, the non-Sync static from dispatch is worse, because it is a regression, one I have not considered:

static FOO: *const i32 = &0;

We allow this only because we look at the unadjusted initializer type, which is &'static T.
This type isn't even available in the MIR, so I don't see a point in trying to keep this working, but I'll add it to the list in the PR description.

@eddyb eddyb removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Apr 27, 2016
@bors
Copy link
Contributor

bors commented May 7, 2016

⌛ Testing commit 3001626 with merge 6c7f924...

@bors
Copy link
Contributor

bors commented May 8, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

@eddyb
Copy link
Member Author

eddyb commented May 8, 2016

Actual failure.

@eddyb
Copy link
Member Author

eddyb commented May 8, 2016

@bors r=nikomatsakis

@alexcrichton Remind me to bug you about enabling LLVM assertions on the test bots, something that should've been caught by an assertion ended up in a successful compile (but the resulting program crashed in an edge condition).
This seems very dangerous to have around, as we keep fiddling with trans.

@bors
Copy link
Contributor

bors commented May 8, 2016

📌 Commit 3b0e27c has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented May 8, 2016

⌛ Testing commit 3b0e27c with merge 1ec2217...

bors added a commit that referenced this pull request May 8, 2016
Implement constant support in MIR.

All of the intended features in `trans::consts` are now supported by `mir::constant`.
The implementation is considered a temporary measure until `miri` replaces it.

A `-Z orbit` bootstrap build will only translate LLVM IR from AST for `#[rustc_no_mir]` functions.

Furthermore, almost all checks of constant expressions have been moved to MIR.
In non-`const` functions, trees of temporaries are promoted, as per RFC 1414 (rvalue promotion).
Promotion before MIR borrowck would allow reasoning about promoted values' lifetimes.

The improved checking comes at the cost of four `[breaking-change]`s:
* repeat counts must contain a constant expression, e.g.:
`let arr = [0; { println!("foo"); 5 }];` used to be allowed (it behaved like `let arr = [0; 5];`)
* dereference of a reference to a `static` cannot be used in another `static`, e.g.:
`static X: [u8; 1] = [1]; static Y: u8 = (&X)[0];` was unintentionally allowed before
* the type of a `static` *must* be `Sync`, irrespective of the initializer, e.g.
`static FOO: *const T = &BAR;` worked as `&T` is `Sync`, but it shouldn't because `*const T` isn't
* a `static` cannot wrap `UnsafeCell` around a type that *may* need drop, e.g.
`static X: MakeSync<UnsafeCell<Option<String>>> = MakeSync(UnsafeCell::new(None));`
was previously allowed based on the fact `None` alone doesn't need drop, but in `UnsafeCell`
it can be later changed to `Some(String)` which *does* need dropping

The drop restrictions are relaxed by RFC 1440 (#33156), which is implemented, but feature-gated.
However, creating `UnsafeCell` from constants is unstable, so users can just enable the feature gate.
@alexcrichton
Copy link
Member

@eddyb oh yeah I'd be fine enabling LLVM assertions, we'd just want to time it and ensure it doesn't regress cycle time too much. If it's just a few minutes that's fine though as it's some good testing.

I thought we already did this but apparently not! Looks like we only enable LLVM assertions on nightly.

@eddyb eddyb deleted the mir-const branch May 9, 2016 00:08
bors added a commit that referenced this pull request May 10, 2016
Fix several -Z orbit crater blockers.

Fixes 3 of the issues found by @nikomatsakis' crater run with `-Z orbit` forced on:
https://gist.github.com/nikomatsakis/6688c30a0e5d3fed07cc1ebd4efb1412

Two of the regressions seemed to be fixed by #33130 and the remaining two are timeouts.
@eddyb eddyb added the relnotes Marks issues that should be documented in the release notes of the next release. label May 12, 2016
@Ryman
Copy link
Contributor

Ryman commented May 13, 2016

The following seems to have regressed, but I'm unsure if that's intentional or not? (also not entirely sure if this is the correct PR but it looks most related)

pub static mut freelist: [&'static mut [u8]; 16] =
    [&mut [], &mut [], &mut [], &mut [], &mut [], &mut [], &mut [],
    &mut [], &mut [], &mut [], &mut [], &mut [], &mut [], &mut [],
    &mut [], &mut []];

Runs on stable and beta, fails on nightly: https://is.gd/9tUBN8

This is in the wild but not on crates.io, dropbox/rust-alloc-no-stdlib#1 which is a dependency for https://github.com/dropbox/rust-brotli-no-stdlib

@eddyb
Copy link
Member Author

eddyb commented May 13, 2016

@Ryman That is supposed to be allowed, but I can't see how it happens, because it's supposed to be whitelisted based on the type.

@eddyb
Copy link
Member Author

eddyb commented May 13, 2016

@Ryman Should be fixed in #33620.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants