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
Merged

Validate miri against the HIR const evaluator #45002

merged 1,591 commits into from
Dec 14, 2017

Conversation

oli-obk
Copy link
Contributor

@oli-obk 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 30 commits July 31, 2017 12:41
make force_allocation handle packed ByValPair
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.
Split up miri into the librustc_mir and bin parts
@oli-obk
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
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
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
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())
Copy link
Contributor Author

@oli-obk oli-obk Dec 14, 2017

Choose a reason for hiding this comment

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

found the issue (I think)

@kennytm
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
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
Copy link
Contributor

bors commented Dec 14, 2017

📌 Commit 7a2bff7 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Dec 14, 2017

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

@bors
Copy link
Contributor

bors commented Dec 14, 2017

📌 Commit 7a2bff7 has been approved by eddyb

@kennytm kennytm 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 Dec 14, 2017
@bors
Copy link
Contributor

bors commented Dec 14, 2017

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

@bors
Copy link
Contributor

bors commented Dec 14, 2017

📌 Commit 7a2bff7 has been approved by eddyb

@bors
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
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
Copy link
Contributor

bors commented Dec 14, 2017

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

@vn971
Copy link
Contributor

vn971 commented Jan 22, 2021

Hi, is there any reason why .editorconfig was removed? See first changed file in acdf83f
CC @oli-obk

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 22, 2021

I don't remember... sorry

@vn971 vn971 mentioned this pull request Jan 22, 2021
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.