Miri engine cleanup by RalfJung · Pull Request #53671 · rust-lang/rust

RalfJung · 2018-08-24T14:46:16Z

Unify the two maps in memory to store the allocation and its kind together.
Share the handling of statics between CTFE and miri: The miri engine always
uses "lazy" AllocType::Static when encountering a static. Acessing that
static invokes CTFE (no matter the machine). The machine only has any
influence when writing to a static, which CTFE outright rejects (but miri
makes a copy-on-write).
Add an AllocId to by-ref consts so miri can use them as operands without
making copies.
Move responsibilities around for the eval_fn_call machine hook: The hook
just has to find the MIR (or entirely take care of everything); pushing the
new stack frame is taken care of by the miri engine.
Expose the intrinsics and lang items implemented by CTFE so miri does not
have to reimplement them.
Allow Machine to hook into foreign statics (used by miri to get rid of some other hacks).
Clean up function calling.
Switch const sanity check to work on operands, not mplaces.
Move const_eval out of rustc_mir::interpret, to make sure that it does not access private implementation details.

In particular, we can finally make eval_operand take &self. :-)

Should be merged after #53609, across which I will rebase.

rust-highfive · 2018-08-24T14:46:19Z

r? @varkor

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

rust-highfive · 2018-08-24T14:46:20Z

⚠️Warning⚠️

These commits modify submodules.

RalfJung · 2018-08-24T14:46:33Z

r? @oli-obk

Cc @eddyb

bors · 2018-08-25T09:00:31Z

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

RalfJung · 2018-08-25T16:36:01Z

@eddyb pointed out that the way validation handles user-defined types was wrong. I am working on fixing that. Good news is that the PlaceExtra type is entirely gone (MemPlace just has an Option<Scalar> now), and this fixed some other definitely-wrong previous code as well :D

RalfJung · 2018-08-25T19:35:10Z

Done! PlaceExtra is gone. :) And also function calls are nicer now that @eddyb pointed me at mir.spread_arg.

I will stopp adding stuff to this PR now, I promise. :) (If it ends up green.)

bors · 2018-08-26T03:14:55Z

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

oli-obk · 2018-08-26T10:56:40Z

this is just self.tcx.mk_nil()

D'oh, thanks! "nil" is not the name I was looking for...

Yea this is naming from ancient Rust... We should probably rename all nil in the compiler to unit. I'll open an easy issue

Done (with next push)

oli-obk · 2018-08-26T11:04:09Z

Done (with next push)

oli-obk · 2018-08-26T11:07:12Z

what's wrong with the downcast variant? It should be the same size and alignment

The comment was introduced by the previous PR, where it was needed when I had places pointing into locals through a field field in the Place::local variant.
Is the downcast variant always guaranteed to be like that? And there can also be structs wrapping other structs, maintaining the Scalar layout -- and some of the outer structs might have repr but the place we are seeing here is already for the inner field. That would change the alignment and be relevant here, right?

right. That makes sense. Just expand the comment to mention the inner field case

Done (with next push)

oli-obk · 2018-08-26T11:12:05Z

can you make these if lets a match over the type + method calls instead of having the logic right here?

You want one function per type?

if that's not too much hazzle, yes

Done (with next push)

oli-obk · 2018-08-26T11:17:01Z

This is a slight regression in diagnostics. Can you include the invalid value and expected values in the error variant and report that in the form of found "42" expected one of [5, 6, 7] or similar?
Also the ReadPointerAsBytes error should report that it found a pointer where it expected an integral discriminant

You think those lists of possible discriminant values are useful? The old messages showed a range, giving the lower and largest possible value. That was certainly not useful because intermediate values were also invalid. So in terms of usefulness this is not a regression...
Getting all that information here will be really hard, I think.
In a later commit, this is changed to always say "invalid discriminant", even for ReadPointerAsBytes.

Ok, let's leave this alone for now. Seems like a minor issue, as the user can figure out the possible values themselves. Displaying the found value is still helpful though.

Done (with next push)
Adding/changing a variant in the EvalErrorKind enum is way too much effort...

oli-obk · 2018-08-26T11:22:05Z

won't this get us into trouble with extern types? They are unsized but have no extra

@eddyb says they technically have an extra of (). But you are right that this might still not work because their extra is not pointer-sized.
The old code here assumed that Dynamic, Str and Slice are the only possible unsized tails.
What could a testcase look like?

#![feature(extern_types)]extern{typeFoo;}fnmain(){let x:&Foo = unsafe{&*(16as*constFoo)};let y:&Foo = &*x;}
might already trigger it

Hm, you are right, And I have no idea why this worked on old miri...

RalfJung · 2018-08-26T11:51:56Z

I rebased, and also managed to get rid of the IsStatic trait again by instead having an associated const in the machine that says which machine memory kind (if any) to use for mutated statics.

RalfJung · 2018-08-26T13:45:01Z

Let's ask perf...

@bors try

bors · 2018-08-26T13:45:14Z

⌛ Trying commit a11e9ed with merge 3c7dbff...

Miri engine cleanup * Unify the two maps in memory to store the allocation and its kind together. * Share the handling of statics between CTFE and miri: The miri engine always uses "lazy" `AllocType::Static` when encountering a static. Acessing that static invokes CTFE (no matter the machine). The machine only has any influence when writing to a static, which CTFE outright rejects (but miri makes a copy-on-write). * Add an `AllocId` to by-ref consts so miri can use them as operands without making copies. * Move responsibilities around for the `eval_fn_call` machine hook: The hook just has to find the MIR (or entirely take care of everything); pushing the new stack frame is taken care of by the miri engine. * Expose the intrinsics and lang items implemented by CTFE so miri does not have to reimplement them. * Allow Machine to hook into foreign statics (used by miri to get rid of some other hacks). * Clean up function calling. * Switch const sanity check to work on operands, not mplaces. * Move const_eval out of rustc_mir::interpret, to make sure that it does not access private implementation details. In particular, we can finally make `eval_operand` take `&self`. :-) Should be merged after #53609, across which I will rebase.

rust-highfive · 2018-08-26T15:33:32Z

The job x86_64-gnu-llvm-5.0 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.

[00:48:15] ....................................................................................................
[00:48:18] ....................................................................................................
[00:48:20] ......i.............................................................................................
[00:48:24] ....................................................................................................
[00:48:26] .......................................................iiiiiiiii....................................
[00:48:32] ....................................................................................................
[00:48:35] ....................................................................................................
[00:48:38] ....................................i...............................................................
[00:48:41] .....................................................................................i.i..ii........
---
[00:50:31] ....................................................................................................
[00:50:40] ....................................................................................................
[00:50:52] ....................................................................................................
[00:51:01] ....................................................................................................
[00:51:11] .............................................................F......................................
[00:51:25] ....................................................................................................
[00:51:33] ....................................................................................................
[00:51:44] ...........................................................................i........................
[00:51:54] ....................................................................................................
---
[00:54:54] ---- [run-pass] run-pass/issue-15523-big.rs stdout ----
[00:54:54] [00:54:54] error: compilation failed!
[00:54:54] status: exit code: 1
[00:54:54] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/issue-15523-big.rs" "--target=x86_64-unknown-linux-gnu" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/issue-15523-big/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/issue-15523-big/auxiliary"
[00:54:54] ------------------------------------------
[00:54:54] [00:54:54] ------------------------------------------
[00:54:54] stderr:
[00:54:54] stderr:
[00:54:54] ------------------------------------------
[00:54:54] error: this expression will panic at runtime
[00:54:54] --> /checkout/src/test/run-pass/issue-15523-big.rs:31:14
[00:54:54] |
[00:54:54] 31 | PosMax = !(1 << 63),
[00:54:54] | ^^^^^^^^^^ attempt to negate with overflow
[00:54:54] = note: #[deny(const_err)] on by default
[00:54:54] [00:54:54] error: aborting due to previous error
[00:54:54] ---
[00:54:54] [00:54:54] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:497:22
[00:54:54] [00:54:54] [00:54:54] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/run-pass" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "run-pass" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-5.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "5.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:54:54] [00:54:54] [00:54:54] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:54:54] Build completed unsuccessfully in 0:10:40
[00:54:54] Build completed unsuccessfully in 0:10:40
[00:54:54] Makefile:58: recipe for target 'check' failed
[00:54:54] make: *** [check] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:19ff6878
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:22c3ac30:start=1535297572596274392,finish=1535297572605087884,duration=8813492
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:07daf4c0
$ 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 -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:05d2e2a8
travis_time:start:05d2e2a8
$ 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:06f478d4
$ 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)

RalfJung · 2018-08-26T16:02:57Z

@rust-timer build 3c7dbff

rust-timer · 2018-08-26T16:02:58Z

Success: Queued 3c7dbff with parent caed80b, comparison URL.

RalfJung · 2018-08-27T06:52:50Z

Surprisingly there are perf differences. Seems the only things that got noticeably slower are the CTFE stress tests; OTOH coercions got significantly faster -- likely because strings are now validated all at once using read_str, instead of validating every byte individually.

RalfJung · 2018-08-28T17:56:23Z

Ah yes... getting miri to work again on all platforms will take a while.

I will back out the miri update part of this, and solve that in the miri repo.

…th unary '-' overflowing

RalfJung · 2018-08-28T17:57:57Z

@bors r=oli-obk

bors · 2018-08-28T17:57:58Z

📌 Commit c9b5fac has been approved by oli-obk

Mark-Simulacrum · 2018-08-28T22:20:26Z

@bors p=6

Miri engine cleanup * Unify the two maps in memory to store the allocation and its kind together. * Share the handling of statics between CTFE and miri: The miri engine always uses "lazy" `AllocType::Static` when encountering a static. Acessing that static invokes CTFE (no matter the machine). The machine only has any influence when writing to a static, which CTFE outright rejects (but miri makes a copy-on-write). * Add an `AllocId` to by-ref consts so miri can use them as operands without making copies. * Move responsibilities around for the `eval_fn_call` machine hook: The hook just has to find the MIR (or entirely take care of everything); pushing the new stack frame is taken care of by the miri engine. * Expose the intrinsics and lang items implemented by CTFE so miri does not have to reimplement them. * Allow Machine to hook into foreign statics (used by miri to get rid of some other hacks). * Clean up function calling. * Switch const sanity check to work on operands, not mplaces. * Move const_eval out of rustc_mir::interpret, to make sure that it does not access private implementation details. In particular, we can finally make `eval_operand` take `&self`. :-) Should be merged after #53609, across which I will rebase.

bors · 2018-08-29T00:02:49Z

⌛ Testing commit c9b5fac with merge f1d02c3...

bors · 2018-08-29T02:07:56Z

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing f1d02c3 to master...

nnethercote · 2018-08-31T02:49:52Z

This was a big (~10%) compile speed win for coercions:
https://perf.rust-lang.org/compare.html?start=7061b2775782bb48c0a70d3c79ec711134396687&end=f1d02c307348057fd0554ad934006b186f8b6826&stat=instructions:u

Other changes are smaller, and might just be noise, it's a bit hard to tell.

RalfJung · 2018-08-31T06:34:19Z

@nnethercote Yes, constants containing strings got faster because we now validate the entire string at once, instead of byte-for-byte.

@oli-obk I suppose we could make similar optimizations for slices of simple types (integer, float). Not sure if it's worth it? Probably worth writing down somewhere, at least.

oli-obk · 2018-08-31T06:58:05Z

Done #53845

nnethercote · 2018-09-22T08:42:35Z

This was also a huge memory win for the old ctfe-stress-force-alloc benchmark:
https://perf.rust-lang.org/compare.html?start=7061b2775782bb48c0a70d3c79ec711134396687&end=f1d02c307348057fd0554ad934006b186f8b6826&stat=max-rss

eddyb · 2018-09-28T08:21:35Z

+}
+impl<'tcx> PartialEq for OpTy<'tcx> {
+ fn eq(&self, other: &Self) -> bool {
+ self.op == other.op && self.layout.ty == other.layout.ty


This is a similar problem as with #53424. Maybe we should just derive PartialEq, Eq, Hash on Layout and TyLayout?!

It is the same problem in fact.
Why have they not been derived in the first place?

(I got a notification for you writing a comment here but now cannot see the comment, @eddyb... wtf?)
FWIW, in the mean time #54936 has landed.

I deleted the comment after seeing that PR.

eddyb · 2018-09-28T08:22:46Z

+ /// Metadata for unsized places. Interpretation is up to the type.
+ /// Must not be present for sized types, but can be missing for unsized types
+ /// (e.g. `extern type`).
+ pub extra: Option<Scalar>,


I'd rename this to "meta".

I added that to #54461

@eddyb

impl Eq+Hash for TyLayout As proposed by @eddyb at rust-lang#53671 (review). I have an upcoming PR that would also significantly benefit from this.

@eddyb

impl Eq+Hash for TyLayout As proposed by @eddyb at rust-lang#53671 (review). I have an upcoming PR that would also significantly benefit from this.

@eddyb

impl Eq+Hash for TyLayout As proposed by @eddyb at rust-lang#53671 (review). I have an upcoming PR that would also significantly benefit from this.

@eddyb

impl Eq+Hash for TyLayout As proposed by @eddyb at rust-lang#53671 (review). I have an upcoming PR that would also significantly benefit from this.

rust-highfive assigned varkor Aug 24, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 24, 2018

rust-highfive assigned oli-obk and unassigned varkor Aug 24, 2018

RalfJung force-pushed the miri-refactor branch 2 times, most recently from 0627c37 to 395a1a7Compare August 24, 2018 17:51

RalfJung force-pushed the miri-refactor branch from 000b906 to 823335eCompare August 25, 2018 19:22

oli-obk requested changes Aug 26, 2018
View reviewed changes

RalfJung force-pushed the miri-refactor branch from 823335e to f86c476Compare August 26, 2018 11:49

oli-obk approved these changes Aug 26, 2018
View reviewed changes

This comment has been minimized.
Sign in to view

RalfJung force-pushed the miri-refactor branch from 8ded044 to 6c10acbCompare August 26, 2018 16:11

RalfJung mentioned this pull request Aug 27, 2018
Fix issue #52475: Make loop detector only consider reachable memory #52626
Merged
3 tasks

RalfJung added 5 commits August 28, 2018 19:57

address nits
f96208c

fix unsized extern types
066d2ee

restructure unary_op to also dispatch on type first; fix promotion wi…
e6a5a94
…th unary '-' overflowing

fix const_prop detecting unary neg underflows
506dd70

first test const-ness, then hook fn call
c9b5fac

RalfJung force-pushed the miri-refactor branch from c4c51be to c9b5facCompare August 28, 2018 17:57

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 Aug 28, 2018

bors merged commit c9b5fac into rust-lang:masterAug 29, 2018

This was referenced Aug 29, 2018
Made std::intrinsics::transmute() const fn. #53535
Merged
Add more const int ops #53697
Merged

oli-obk mentioned this pull request Aug 31, 2018
Optimize miri engine performance for sanity checks of arrays #53845
Closed

eddyb reviewed Sep 28, 2018
View reviewed changes

RalfJung mentioned this pull request Oct 9, 2018
impl Eq+Hash for TyLayout #54936
Merged

Uh oh!

Uh oh!

Conversation

RalfJung commented Aug 24, 2018

Uh oh!

rust-highfive commented Aug 24, 2018

Uh oh!

rust-highfive commented Aug 24, 2018

Uh oh!

RalfJung commented Aug 24, 2018

Uh oh!

bors commented Aug 25, 2018

Uh oh!

RalfJung commented Aug 25, 2018

Uh oh!

RalfJung commented Aug 25, 2018

Uh oh!

bors commented Aug 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Aug 26, 2018

Uh oh!

RalfJung commented Aug 26, 2018

Uh oh!

bors commented Aug 26, 2018

Uh oh!

This comment has been minimized.

rust-highfive commented Aug 26, 2018

Uh oh!

This comment has been minimized.

This comment has been minimized.

RalfJung commented Aug 26, 2018

Uh oh!

rust-timer commented Aug 26, 2018

Uh oh!