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

Don't emit Unevaluated from const_eval #56723

Merged
merged 16 commits into from
Jan 4, 2019
Merged

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 11, 2018

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2018
@matthewjasper
Copy link
Contributor

r? @RalfJung

@rust-highfive
Copy link
Collaborator

The job mingw-check of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:04b9a33c:start=1544558344989913189,finish=1544558420352388686,duration=75362475497
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=mingw-check
---
[00:07:07] configure: build.locked-deps    := True
[00:07:07] configure: llvm.ccache          := sccache
[00:07:07] configure: build.cargo-native-static := True
[00:07:07] configure: dist.missing-tools   := True
[00:07:07] configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
[00:07:07] configure: writing `config.toml` in current directory
[00:07:07] configure: 
[00:07:07] configure: run `python /checkout/x.py --help`
[00:07:07] configure: 
---
[00:09:44]     Checking syntax_ext v0.0.0 (/checkout/src/libsyntax_ext)
[00:11:09] error[E0080]: could not evaluate static initializer
[00:11:09]     --> src/librustc/ty/sty.rs:1989:1
[00:11:09]      |
[00:11:09] 1989 | static_assert!(MEM_SIZE_OF_LAZY_CONST: ::std::mem::size_of::<LazyConst<'_>>() == 24);
[00:11:09]      | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ index out of bounds: the len is 1 but the index is 1
[00:11:09]      = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
[00:11:09] 
[00:11:10] error: aborting due to previous error
[00:11:10] 
[00:11:10] 
[00:11:10] For more information about this error, try `rustc --explain E0080`.
[00:11:10] error: Could not compile `rustc`.
[00:11:10] 
[00:11:10] To learn more, run the command again with --verbose.
[00:11:10] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "i686-pc-windows-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:11:10] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu
[00:11:10] Build completed unsuccessfully in 0:02:54
travis_time:end:0e63e16a:start=1544558428984192020,finish=1544559099453644431,duration=670469452411
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 1.
---
travis_time:end:02ca9624:start=1544559099902914048,finish=1544559099909545488,duration=6631440
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0ba53794
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1441e3ec
travis_time:start:1441e3ec
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:07329432
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

"_Self".to_owned(),
Some(format!("[{}; _]", self.tcx.type_of(def.did).to_string())),
));
}
Copy link
Member

Choose a reason for hiding this comment

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

So we don't print anything when it is not evaluated? That is different from the previous code.

I have no idea where this code is called, just pointing out a functional change. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops indeed. This is just for producing more diagnostics, but still. I found a nice way to fix it without more duplication.

@RalfJung
Copy link
Member

Were Const interned before? Does this mean we now intern both Const and LazyConst?

src/librustc/ty/sty.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 12, 2018

Were Const interned before? Does this mean we now intern both Const and LazyConst?

Yes they were interned before. Yes we now intern twice. I'll check if we can get away with not interning Const anymore. It's just a size optimization of various enums, but we might not have any Const left, so...

@@ -376,6 +376,11 @@ impl<'a, 'b, 'gcx, 'tcx> TypeVerifier<'a, 'b, 'gcx, 'tcx> {
constant, location
);

let literal = match constant.literal {
ty::LazyConst::Evaluated(lit) => lit,
ty::LazyConst::Unevaluated(..) => return,
Copy link
Member

Choose a reason for hiding this comment

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

Is it really correct to return early here in case of Unevaluated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is #46702

I don't believe this PR makes the situation any more problematic than it was.

Copy link
Member

Choose a reason for hiding this comment

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

Okay.

Cc @eddyb would be good if you could take a quick look at some point, I have no idea what this is doing.^^

@RalfJung
Copy link
Member

we might not have any Const left

If we don't, that makes me wonder about whether this refactoring is worth it. :/

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 13, 2018

If we don't, that makes me wonder about whether this refactoring is worth it. :/

Sorry, I misspoke: We might not have any Const left in the HIR or the MIR, we'd still be using it extensively everywhere else (at least I think so, I wanted to do larger refactorings like that in a separate PR, but I'll do them here now)

@rust-highfive

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 14, 2018

This should theoretically not have a negative perf impact, but I've guessed wrong with this kind of change before.

let's see what perf says

@bors try

@bors
Copy link
Contributor

bors commented Dec 14, 2018

⌛ Trying commit b01765fdfdbec1b70d7d41dccf7db02467721eeb with merge 6ab2e74a91c27b914174a1c9db557e41235bce60...

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 14, 2018

@rust-timer build 6ab2e74a91c27b914174a1c9db557e41235bce60

@rust-timer
Copy link
Collaborator

Success: Queued 6ab2e74a91c27b914174a1c9db557e41235bce60 with parent 7d03617, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 6ab2e74a91c27b914174a1c9db557e41235bce60

@bors
Copy link
Contributor

bors commented Dec 14, 2018

💥 Test timed out

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@nikomatsakis
Copy link
Contributor

r=me modulo rebase

@nikomatsakis nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 2, 2019
@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 3, 2019

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jan 3, 2019

📌 Commit 03b8928 has been approved by nikomatsakis

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 3, 2019
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jan 3, 2019
@bors
Copy link
Contributor

bors commented Jan 4, 2019

⌛ Testing commit 03b8928 with merge d6d32ac...

bors added a commit that referenced this pull request Jan 4, 2019
Don't emit `Unevaluated` from `const_eval`

cc @eddyb @RalfJung
@bors
Copy link
Contributor

bors commented Jan 4, 2019

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing d6d32ac to master...

@bors bors merged commit 03b8928 into rust-lang:master Jan 4, 2019
impl<'a, 'tcx> Lift<'tcx> for &'a List<$ty> {
type Lifted = &'tcx List<$lifted>;
fn lift_to_tcx<'b, 'gcx>(&self, tcx: TyCtxt<'b, 'gcx, 'tcx>) -> Option<Self::Lifted> {
if self.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

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

Wrong indentation.

self.require_sized(constant.ty, traits::ConstSized);
if let ConstValue::Unevaluated(def_id, substs) = constant.val {
fn compute_array_len(&mut self, constant: ty::LazyConst<'tcx>) {
if let ty::LazyConst::Unevaluated(def_id, substs) = constant {
Copy link
Member

Choose a reason for hiding this comment

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

Why was this change made? If I called something const it's because it's meant to be used with const generics in the future.
This needs to be renamed back, and used for all ty::Consts in all Substs.
The lack of WF const handling probably allows a lot of unintended const expressions.
cc @varkor @yodaldevoid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants