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

Validate miri against the HIR const evaluator #45002

Merged
merged 1,591 commits into from Dec 14, 2017

Conversation

Projects
None yet
@oli-obk
Copy link
Contributor

oli-obk commented Oct 3, 2017

r? @eddyb

cc @alexcrichton @arielb1 @RalfJung

The interesting parts are the last few functions in librustc_const_eval/eval.rs

  • We warn if miri produces an error while HIR const eval does not.
  • We warn if miri produces a value that does not match the value produced by HIR const eval
  • if miri succeeds and HIR const eval fails, nothing is emitted, but we still return the HIR error
  • if both error, nothing is emitted and the HIR const eval error is returned

So there are no actual changes, except that miri is forced to produce the same values as the old const eval.

  • This does not touch the const evaluator in trans at all. That will come in a future PR.
  • This does not cause any code to compile that didn't compile before. That will also come in the future

It would be great if someone could start a crater run if travis passes

oli-obk and others added some commits Jul 31, 2017

Merge pull request #274 from RalfJung/packed2
make force_allocation handle packed ByValPair
Reduce the chance of accidentally calling functions in CTFE
previously miri had a check for const fn and other cases that
CTFE requires. Instead the function call is completely
processed inside the machine. This allows CTFE to have full
control over what is called and miri to not have useless
CTFE-checks in normal mode.
Merge pull request #268 from oli-obk/upstream
Split up miri into the librustc_mir and bin parts
Merge pull request #278 from solson/unions
Process untagged unions

bors added a commit that referenced this pull request Dec 14, 2017

Auto merge of #45002 - oli-obk:miri, r=<try>
Validate miri against the HIR const evaluator

r? @eddyb

cc @alexcrichton @arielb1 @RalfJung

The interesting parts are the last few functions in `librustc_const_eval/eval.rs`

* We warn if miri produces an error while HIR const eval does not.
* We warn if miri produces a value that does not match the value produced by HIR const eval
* if miri succeeds and HIR const eval fails, nothing is emitted, but we still return the HIR error
* if both error, nothing is emitted and the HIR const eval error is returned

So there are no actual changes, except that miri is forced to produce the same values as the old const eval.

* This does **not** touch the const evaluator in trans at all. That will come in a future PR.
* This does **not** cause any code to compile that didn't compile before. That will also come in the future

It would be great if someone could start a crater run if travis passes
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 14, 2017

☀️ Test successful - status-travis
State: approved= try=True

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

oli-obk commented Dec 14, 2017

That fired backwards. Analyzing dist packages now

size file
213M rust-nightly-...-.tar.gz
102M rust-nightly-...-.tar.xz
99M rust-std-...-.tar.gz
69M rust-std-...-.tar.xz
-rw-r--r-- 1 travis travis  14014664 Dec 14 11:59 cargo-nightly-x86_64-unknown-linux-gnu.tar.gz
-rw-r--r-- 1 travis travis   9777668 Dec 14 11:59 cargo-nightly-x86_64-unknown-linux-gnu.tar.xz
-rw-r--r-- 1 travis travis  22066225 Dec 14 11:59 rls-nightly-x86_64-unknown-linux-gnu.tar.gz
-rw-r--r-- 1 travis travis  14975372 Dec 14 11:59 rls-nightly-x86_64-unknown-linux-gnu.tar.xz
-rw-r--r-- 1 travis travis    485437 Dec 14 11:59 rust-analysis-nightly-x86_64-unknown-linux-gnu.tar.gz
-rw-r--r-- 1 travis travis    339228 Dec 14 11:59 rust-analysis-nightly-x86_64-unknown-linux-gnu.tar.xz
-rw-r--r-- 1 travis travis  60237709 Dec 14 11:59 rustc-nightly-src.tar.gz
-rw-r--r-- 1 travis travis  38632276 Dec 14 11:59 rustc-nightly-src.tar.xz
-rw-r--r-- 1 travis travis  64878972 Dec 14 11:59 rustc-nightly-x86_64-unknown-linux-gnu.tar.gz
-rw-r--r-- 1 travis travis  49012640 Dec 14 11:59 rustc-nightly-x86_64-unknown-linux-gnu.tar.xz
-rw-r--r-- 1 travis travis  16920283 Dec 14 11:59 rust-docs-nightly-x86_64-unknown-linux-gnu.tar.gz
-rw-r--r-- 1 travis travis   4811900 Dec 14 11:59 rust-docs-nightly-x86_64-unknown-linux-gnu.tar.xz
-rw-r--r-- 1 travis travis   6306484 Dec 14 11:59 rustfmt-nightly-x86_64-unknown-linux-gnu.tar.gz
-rw-r--r-- 1 travis travis   4297024 Dec 14 11:59 rustfmt-nightly-x86_64-unknown-linux-gnu.tar.xz
-rw-r--r-- 1 travis travis 223798237 Dec 14 11:59 rust-nightly-x86_64-unknown-linux-gnu.tar.gz
-rw-r--r-- 1 travis travis 107160156 Dec 14 11:59 rust-nightly-x86_64-unknown-linux-gnu.tar.xz
-rw-r--r-- 1 travis travis   3618394 Dec 14 11:59 rust-src-nightly.tar.gz
-rw-r--r-- 1 travis travis   2431024 Dec 14 11:59 rust-src-nightly.tar.xz
-rw-r--r-- 1 travis travis  99259045 Dec 14 11:59 rust-std-nightly-x86_64-unknown-linux-gnu.tar.gz
-rw-r--r-- 1 travis travis  69615932 Dec 14 11:59 rust-std-nightly-x86_64-unknown-linux-gnu.tar.xz
@oli-obk

This comment has been minimized.

Copy link
Contributor Author

oli-obk commented Dec 14, 2017

Nevermind, looked at the DEPLOY_ALT builder. The DEPLOY builder decreased further in size

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

oli-obk commented Dec 14, 2017

At this point I don't know where the increase in size is coming from. Miri is adding nothing measurable to librustc and like 2MB to librustc_mir. I don't see how the other crates' size increases can come from miri.

It's a 23MB increase, which still is 12%. All I can think of now is that it severly degraded the zippability of the crates, because the unzipped crates seem to not have increased by that much.

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Dec 14, 2017

@oli-obk

zippability of the crates

Since the tarballs are actually uploaded we could download them and check locally.
Before (8954b16) After (a9b425b)
Download URL (URL) (URL)
File size 194,457,881 223,798,237
After gunzip 664,856,576 804,350,976

It doesn't look like related to zippability, but an actual increase in binary size.


Edit:

Comparing top largest files in the tarball (sorted by "after"):
File Before After
rls-preview/bin/rls 18,232,376 97,001,776
cargo/bin/cargo 17,335,552 57,398,408
rustc/lib/librustc_llvm-*.so 56,813,280 56,813,112
rust-std-*/lib/rustlib/*/lib/librustc_llvm-*.so 56,813,280 56,813,112
rust-std-*/lib/rustlib/*/lib/librustc_asan-*.rlib 26,420,558 26,420,544
rustc/lib/librustc-*.so 23,119,928 23,468,920
rust-std-*/lib/rustlib/*/lib/librustc-*.so 23,119,928 23,468,920
rust-std-*/lib/rustlib/*/lib/libcore-*.rlib 18,266,456 18,267,880
rust-std-*/lib/rustlib/*/lib/librustc_tsan-*.rlib 17,178,772 17,178,750
rust-std-*/lib/rustlib/*/lib/libstd-*.rlib 16,608,602 16,626,360
rustfmt-preview/bin/rustfmt 3,257,872 16,353,720

Why did RLS and Cargo becomes so much larger? They contribute almost all of the size increase.

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

oli-obk commented Dec 14, 2017

folder before after
rustfmt-preview 9.0 23.0
cargo 17.6 51.8
rls-preview 18.3 84.9
rust-docs 149.6 149.6
rustc 171.6 159.6
rust-std 303.1 299.4

soo... uhm... I've been looking at the wrong thing the entire time

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Dec 14, 2017

Did we change the situations in which we generate MIR for libraries, or the MIR itself? Are we compiling some new dependencies as rlibs and thus duplicating them in the binaries somehow?

@@ -495,13 +495,11 @@ impl<'a> Builder<'a> {
if let Some(target_linker) = self.build.linker(target) {
cargo.env("RUSTC_TARGET_LINKER", target_linker);
}
cargo.env("RUSTC_DEBUGINFO", self.config.rust_debuginfo.to_string())

This comment has been minimized.

@oli-obk

oli-obk Dec 14, 2017

Author Contributor

found the issue (I think)

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Dec 14, 2017

Specifically checking the size increase of RLS:
Before After
URL (URL) (URL)
Size 4,641,992 14,975,372
After unxz 18,311,680 97,080,832
rls-preview/bin/rls binary size 18,232,376 97,001,776
Comparing the sections with `llvm-readelf -sections rls-preview/bin/rls`:

screenshot_2017-12-14 22 36 36_hxwuye-fs8

It's definitely the debug symbols. All the extra size is occupied by debug symbols.

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

oli-obk commented Dec 14, 2017

While it's not fun to debug the tools without debug info, I can just activate it when needed. (confirmed locally that the binary size of the tools is back to what it's supposed to be)

Launching miri

3...
2...
1...
@bors r=eddyb

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 14, 2017

📌 Commit 7a2bff7 has been approved by eddyb

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 14, 2017

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #46335
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 14, 2017

📌 Commit 7a2bff7 has been approved by eddyb

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 14, 2017

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #46335
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 14, 2017

📌 Commit 7a2bff7 has been approved by eddyb

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 14, 2017

⌛️ Testing commit 7a2bff7 with merge 2974104...

bors added a commit that referenced this pull request Dec 14, 2017

Auto merge of #45002 - oli-obk:miri, r=eddyb
Validate miri against the HIR const evaluator

r? @eddyb

cc @alexcrichton @arielb1 @RalfJung

The interesting parts are the last few functions in `librustc_const_eval/eval.rs`

* We warn if miri produces an error while HIR const eval does not.
* We warn if miri produces a value that does not match the value produced by HIR const eval
* if miri succeeds and HIR const eval fails, nothing is emitted, but we still return the HIR error
* if both error, nothing is emitted and the HIR const eval error is returned

So there are no actual changes, except that miri is forced to produce the same values as the old const eval.

* This does **not** touch the const evaluator in trans at all. That will come in a future PR.
* This does **not** cause any code to compile that didn't compile before. That will also come in the future

It would be great if someone could start a crater run if travis passes
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 14, 2017

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

@bors bors merged commit 7a2bff7 into rust-lang:master Dec 14, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@solson solson referenced this pull request Mar 20, 2018

Closed

upstream to rustc #105

gerd2002 pushed a commit to MegatonHammer/rust that referenced this pull request Apr 2, 2018

Auto merge of rust-lang#45002 - oli-obk:miri, r=eddyb
Validate miri against the HIR const evaluator

r? @eddyb

cc @alexcrichton @arielb1 @RalfJung

The interesting parts are the last few functions in `librustc_const_eval/eval.rs`

* We warn if miri produces an error while HIR const eval does not.
* We warn if miri produces a value that does not match the value produced by HIR const eval
* if miri succeeds and HIR const eval fails, nothing is emitted, but we still return the HIR error
* if both error, nothing is emitted and the HIR const eval error is returned

So there are no actual changes, except that miri is forced to produce the same values as the old const eval.

* This does **not** touch the const evaluator in trans at all. That will come in a future PR.
* This does **not** cause any code to compile that didn't compile before. That will also come in the future

It would be great if someone could start a crater run if travis passes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.