Value visitors for miri #55549

RalfJung · 2018-10-31T18:55:19Z

Generalize the traversal part of validation to a ValueVisitor.

~~This includes #55316, click here for just the new commits.~~

rust-highfive · 2018-10-31T18:55:33Z

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

rust-highfive · 2018-10-31T19:13:59Z

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.

travis_time:end:2b216130:start=1541012208322839323,finish=1541012271118235768,duration=62795396445
$ 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=x86_64-gnu-llvm-5.0
---
[00:14:24]    Compiling rustc_metadata_utils v0.0.0 (/checkout/src/librustc_metadata_utils)
[00:14:24]    Compiling rustc_mir v0.0.0 (/checkout/src/librustc_mir)
[00:14:24]    Compiling rustc_typeck v0.0.0 (/checkout/src/librustc_typeck)
[00:14:25]    Compiling rustc_traits v0.0.0 (/checkout/src/librustc_traits)
[00:14:28] error[E0491]: in type `&'rt mut interpret::validity::RefTracking<'tcx, Tag>`, reference has a longer lifetime than the data it references
[00:14:28]    --> librustc_mir/interpret/validity.rs:142:5
[00:14:28]     |
[00:14:28] 142 |     ref_tracking: Option<&'rt mut RefTracking<'tcx, Tag>>,
[00:14:28]     |
[00:14:28]     |
[00:14:28] note: the pointer is valid for the lifetime 'rt as defined on the struct at 136:24
[00:14:28]    --> librustc_mir/interpret/validity.rs:136:24
[00:14:28]     |
[00:14:28] 136 | struct ValidityVisitor<'rt, 'a, 'tcx, Tag> {
[00:14:28]     |                        ^^^
[00:14:28] note: but the referenced data is only valid for the lifetime 'tcx as defined on the struct at 136:33
[00:14:28]    --> librustc_mir/interpret/validity.rs:136:33
[00:14:28]     |
[00:14:28] 136 | struct ValidityVisitor<'rt, 'a, 'tcx, Tag> {
[00:14:28] 
[00:14:28] 
[00:14:28] error[E0478]: lifetime bound not satisfied
[00:14:28]    --> librustc_mir/interpret/validity.rs:144:5
[00:14:28] 144 |     tcx: TyCtxt<'a, 'tcx, 'tcx>,
[00:14:28]     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[00:14:28]     |
[00:14:28]     |
[00:14:28] note: lifetime parameter instantiated with the lifetime 'tcx as defined on the struct at 136:33
[00:14:28]    --> librustc_mir/interpret/validity.rs:136:33
[00:14:28]     |
[00:14:28] 136 | struct ValidityVisitor<'rt, 'a, 'tcx, Tag> {
[00:14:28]     |                                 ^^^^
[00:14:28] note: but lifetime parameter must outlive the lifetime 'a as defined on the struct at 136:29
[00:14:28]    --> librustc_mir/interpret/validity.rs:136:29
[00:14:28]     |
[00:14:28] 136 | struct ValidityVisitor<'rt, 'a, 'tcx, Tag> {
[00:14:28] 
[00:14:29] error: aborting due to 2 previous errors
[00:14:29] 
[00:14:29] Some errors occurred: E0478, E0491.
[00:14:29] Some errors occurred: E0478, E0491.
[00:14:29] For more information about an error, try `rustc --explain E0478`.
[00:14:29] error: Could not compile `rustc_mir`.
[00:14:29] warning: build failed, waiting for other jobs to finish...
[00:15:53] error: build failed
[00:15:53] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" " jemalloc" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:15:53] expected success, got: exit code: 101
[00:15:53] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1115:9
[00:15:53] travis_fold:end:stage0-rustc

[00:15:53] travis_time:end:stage0-rustc:start=1541012636314096047,finish=1541013234626202982,duration=598312106935


[00:15:53] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:15:53] Build completed unsuccessfully in 0:11:09
[00:15:53] make: *** [all] Error 1
[00:15:53] Makefile:28: recipe for target 'all' failed
1442704 ./obj
1442664 ./obj/build
1194324 ./.git
1072200 ./src
---
151412 ./src/tools/clang
149648 ./obj/build/bootstrap/debug/incremental
149116 ./src/llvm-emscripten/test
134188 ./obj/build/bootstrap/debug/incremental/bootstrap-32pr67l4sa8g0
134184 ./obj/build/bootstrap/debug/incremental/bootstrap-32pr67l4sa8g0/s-f68rcndwwa-1xzoq02-ycmuivyxm2px
111072 ./src/llvm/test/CodeGen
107668 ./obj/build/x86_64-unknown-linux-gnu/stage0/lib/rustlib/x86_64-unknown-linux-gnu/codegen-backends
104700 ./src/tools/lldb

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-10-31T19:54:10Z

Wtf, this compiled fine in stage 2... NLL bug or so?

oli-obk

Preliminary review. I think reviewing the rest only makes sense with the review questions addressed

src/librustc_mir/interpret/visitor.rs

oli-obk · 2018-11-01T12:01:39Z

src/librustc_mir/interpret/visitor.rs

+    // Downcast functions.  These change the value as a side-effect.
+    fn downcast_enum(&mut self, ectx: &EvalContext<'a, 'mir, 'tcx, M>)
+        -> EvalResult<'tcx>;
+    fn downcast_dyn_trait(&mut self, ectx: &EvalContext<'a, 'mir, 'tcx, M>)


I dislike the self mutation. It does not feel right. I think these downcast functions should be recursing via visit_value.

downcast_dyn_trait is gone in a later commit.

For downcast_enum I will do this, but it actually makes the interface larger overall (i.e., one more method to implement).

Oh and then there is an extra hook I need to keep the nice error reporting... of the validity check :/

Okay I did something. Thanks for making me do it, it is now better than it was before. :) Let me know what you think.

RalfJung · 2018-11-01T13:43:51Z

src/test/ui/consts/const-eval/ub-enum.stderr

+  --> $DIR/ub-enum.rs:42:1
+   |
+LL | const BAD_ENUM3 : [Enum2; 2] = [unsafe { TransmuteEnum2 { c: () }.b }; 2];
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ attempted to read undefined bytes


I am aware this is a bad error message. Fixing this will be fairly easy once #55293 lands: Use ScalarMaybeUndef in EvalErrorKind::InvalidDiscriminant.

oli-obk

oh damn, I never pressed the submit review button :/

Anyway, yes, this is getting closer to what I'm imagining.

Take the comments with a grain of salt, because I've not checked how obsolete they have become with your changes

src/librustc_mir/interpret/visitor.rs

oli-obk · 2018-11-01T12:43:24Z

src/librustc_mir/interpret/visitor.rs

+        self.value().layout()
+    }
+
+    // Replace the value by `val`, which must be the `field`th field of `self`, then call


What do you mean by replace? changing a function argument does not persist outside the function call.

I would have assumed this to just forward to visit_value by default, ignoring all the other arguments.

The visitor has in its internal state the current value we are visiting. This function makes it change that state to another value.

I know this is a somewhat strange architecture for a visitor, but it's the best I could come up with so far in terms of code reuse. But the original design also did not have the Value trait; now that I got that maybe I can separate the value from the rest of the visitor state. However, I actually found it quite nice that they are connected because the ValidationVisitor uses that to keep track of its stack.

oli-obk · 2018-11-01T13:11:57Z

src/librustc_mir/interpret/visitor.rs

+
+    // Execute visitor on the current value.  Used for recursing.
+    #[inline]
+    fn visit(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>)


So I think I fundamentally misunderstood the nature of this visitor. I thought it was like the visitor for Mir, which just gives you mutable references to rustc::mir::* types and by default calls walk_*, which destructures the value and calls visit_* on all the values obtainable.

I am not sure what this "visitor" is doing anymore. Would it be possible to write a visitor of the following form, still support your use cases and still copy/allocate as little intermediate state as possible?

trait ValueVisitor<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>: fmt::Debug + Sized { type V: Value<'tcx, M>; fn ecx(&mut self) -> &mut EvalContext<'a, 'mir, 'tcx, M>) fn visit(&mut self, value: &mut V) -> EvalResult<'tcx> { walk_value(self, value) } fn visit_array(&mut self, elements: impl Iterator<Item = &mut V>) -> EvalResult<'tcx> { walk_array(self, elements) } fn visit_enum(&mut self, value: &mut Value) -> EvalResutl<'tcx> { walk_enum(self, value) } // leafs fn visit_scalar(&mut self, _scalar: &mut Scalar, _layout: &layout::Scalar) -> EvalResult<'tcx> { Ok(()) } // ... } fn walk_value.... fn walk_array<VIS: ValueVisitor>(visitor: &mut VIS, elements: impl Iterator<Item = &mut VIS::V>) -> EvalResult<'tcx> { for element in elements { visitor.visit(element)?; } Ok(()) } fn walk_enum<VIS: ValueVisitor>(visitor: &mut VIS, value: &mut VIS::V) -> EvalResult<'tcx> { let variant = value.read_discriminant(self.ecx())?.1; visitor.visit(value.downcast(self.ecx(), variant)?) }

This will increase the amount of recursion happening, but it has a much nicer feel to me, because fewer things are entangled with each other.

bors · 2018-11-01T19:23:12Z

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

oli-obk

The visitor as it is (except for the visit_scalar method as mentioned in the comments) looks good to me now. I'll review the rest of the code independently now

src/librustc_mir/interpret/visitor.rs

oli-obk · 2018-11-02T08:43:05Z

src/librustc_mir/interpret/visitor.rs

+    {
+        self.walk_value(v)
+    }
+    /// Visit the given value as a union.


Suggested change

/// Visit the given value as a union.

/// Visit the given value as a union. Does not recurse unless overwritten to do so.

src/librustc_mir/interpret/visitor.rs

oli-obk · 2018-11-02T08:44:46Z

src/librustc_mir/interpret/visitor.rs

+
+    // Actions on the leaves, ready to be overloaded.
+    /// Called whenever we reach a value with uninhabited layout.
+    /// Recursing to fields will continue after this!


how can an uninhabited type have fields?

oli-obk · 2018-11-02T08:45:59Z

src/librustc_mir/interpret/visitor.rs

+    fn visit_uninhabited(&mut self, _v: Self::V) -> EvalResult<'tcx>
+    { Ok(()) }
+    /// Called whenever we reach a value with scalar layout.
+    /// Recursing to fields will continue after this!


I think the default impl should be calling a walk_scalar method then. Pass all arguments needed to recurse. Implementing a visit_* function as Ok(()) should stop recursion right there.

That does not match the structure we used for the validation visitor, and I think it is the wrong structure. The point is that being a scalar and being an aggregate are orthogonal properties, a value can be neither or both or any combination. Some scalars are primitives, some are not - -we'd have to duplicate all the primitive-vs-aggregate handling. I don't think that would be a good idea.

Oh right. Add a comment that it's not really recursing into the scalar, but the value, so one should overwrite visit_value if they want to not recurse into this case

RalfJung · 2018-11-02T09:52:48Z

I added the iterator-based visit_aggregate. Unfortunately this requires an allocation when visiting a non-array aggregate.

src/librustc_mir/interpret/visitor.rs

RalfJung · 2018-11-02T10:26:59Z

My plan is to ask for perf once #55316 lands, so that we can see just the impact of this refactor.

RalfJung · 2018-11-02T18:54:49Z

Rebased onto master; now this PR can stand for itself.

RalfJung · 2018-11-02T18:55:16Z

@bors try

bors · 2018-11-02T18:55:28Z

⌛ Trying commit 7f93557 with merge 5f8bd0a...

Value visitors for miri Generalize the traversal part of validation to a `ValueVisitor`. ~~This includes #55316, [click here](RalfJung/rust@retagging...RalfJung:miri-visitor) for just the new commits.~~ This PR does not change the interface to miri, so I use the opportunity to update miri.

bors · 2018-11-02T21:48:27Z

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

oli-obk · 2018-11-02T21:49:57Z

@rust-timer build 5f8bd0a

rust-timer · 2018-11-02T21:49:58Z

Success: Queued 5f8bd0a with parent e53a5ff, comparison URL.

oli-obk · 2018-11-02T23:32:56Z

src/librustc_mir/interpret/validity.rs

-        ScalarMaybeUndef::Scalar(Scalar::Bits { bits, .. }) =>
-            bits.to_string(),
+struct ValidityVisitor<'rt, 'a: 'rt, 'mir: 'rt, 'tcx: 'a+'rt+'mir, M: Machine<'a, 'mir, 'tcx>+'rt> {
+    /// The `path` may be pushed to, but the part that is present when a function


we really need to start using the im crate in the compiler

src/librustc/mir/interpret/value.rs

…o implement itself

rust-timer · 2018-11-06T06:37:25Z

Finished benchmarking try commit 5f8bd0a

RalfJung · 2018-11-06T07:39:16Z

Seems like something went wrong? There is no benchmark data. Cc @rust-lang/infra

Maybe we should just try again.
@bors try

bors · 2018-11-06T07:39:28Z

⌛ Trying commit 3538532cb617bcb6c97e304459c2135a5e2240b9 with merge ad5b1b390d95a7a68786df120aefae5118043810...

kennytm · 2018-11-06T07:47:58Z

@RalfJung the parent commit e53a5ff is still being benchmarked (see https://perf.rust-lang.org/status.html). There's no need to retry.

RalfJung · 2018-11-06T07:51:24Z

Oh, so the bot was just wrong telling me that everything is done? Curious.

kennytm · 2018-11-06T09:23:12Z

@RalfJung The bot is telling "technically the truth": 5f8bd0a is indeed finished, but e53a5ff wasn't 🙃. The link should now be ready.

bors · 2018-11-06T10:03:12Z

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

RalfJung · 2018-11-06T10:15:38Z

Some small losses, some small gains... @oli-obk seems acceptable?

src/librustc_mir/interpret/visitor.rs

oli-obk · 2018-11-06T15:28:38Z

@bors r+

bors · 2018-11-06T15:28:39Z

📌 Commit 7b7c6ce has been approved by oli-obk

bors · 2018-11-07T09:14:22Z

⌛ Testing commit 7b7c6ce with merge 8315b11...

Value visitors for miri Generalize the traversal part of validation to a `ValueVisitor`. ~~This includes #55316, [click here](RalfJung/rust@retagging...RalfJung:miri-visitor) for just the new commits.~~

bors · 2018-11-07T11:54:56Z

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

Add escape-to-raw MIR statement Add a new MIR "ghost state statement": Escaping a ptr to permit raw accesses. ~~This includes #55549, [click here](RalfJung/rust@miri-visitor...RalfJung:escape-to-raw) for just the new commits.~~

rust-highfive assigned oli-obk Oct 31, 2018

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

oli-obk requested changes Nov 1, 2018

View reviewed changes

RalfJung commented Nov 1, 2018

View reviewed changes

oli-obk reviewed Nov 1, 2018

View reviewed changes

oli-obk requested changes Nov 2, 2018

View reviewed changes

RalfJung force-pushed the miri-visitor branch from e27a04e to bebf782 Compare November 2, 2018 09:46

oli-obk reviewed Nov 2, 2018

View reviewed changes

src/librustc_mir/interpret/visitor.rs Outdated Show resolved Hide resolved

RalfJung force-pushed the miri-visitor branch 3 times, most recently from 09e4551 to 7f93557 Compare November 2, 2018 18:54

oli-obk reviewed Nov 2, 2018

View reviewed changes

oli-obk approved these changes Nov 2, 2018

View reviewed changes

oli-obk reviewed Nov 4, 2018

View reviewed changes

src/librustc/mir/interpret/value.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

RalfJung added 2 commits November 5, 2018 09:15

generalize the traversal part of validation to a ValueVisitor

5b5e076

reduce the amount of traversal/projection code that the visitor has t…

7d7bd9b

…o implement itself

oli-obk reviewed Nov 6, 2018

View reviewed changes

src/librustc_mir/interpret/visitor.rs Outdated Show resolved Hide resolved

oli-obk reviewed Nov 6, 2018

View reviewed changes

src/librustc_mir/interpret/visitor.rs Outdated Show resolved Hide resolved

RalfJung mentioned this pull request Nov 6, 2018

Add escape-to-raw MIR statement #55716

Merged

RalfJung force-pushed the miri-visitor branch from 3538532 to 7b7c6ce Compare November 6, 2018 15:19

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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 6, 2018

RalfJung mentioned this pull request Nov 6, 2018

New Stacked Borrows, now with better support for interior mutability rust-lang/miri#513

Merged

bors mentioned this pull request Nov 7, 2018

Choose predicates without inference variables over those with them #55453

Merged

bors merged commit 7b7c6ce into rust-lang:master Nov 7, 2018

RalfJung deleted the miri-visitor branch November 9, 2018 15:35

	/// Visit the given value as a union.
	/// Visit the given value as a union. Does not recurse unless overwritten to do so.

Value visitors for miri #55549

Value visitors for miri #55549

Conversation

RalfJung commented Oct 31, 2018 • edited Loading

rust-highfive commented Oct 31, 2018

rust-highfive commented Oct 31, 2018

RalfJung commented Oct 31, 2018

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 1, 2018

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Nov 2, 2018

RalfJung commented Nov 2, 2018

RalfJung commented Nov 2, 2018

RalfJung commented Nov 2, 2018

bors commented Nov 2, 2018

bors commented Nov 2, 2018

oli-obk commented Nov 2, 2018

rust-timer commented Nov 2, 2018

oli-obk Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

rust-timer commented Nov 6, 2018

RalfJung commented Nov 6, 2018

bors commented Nov 6, 2018

kennytm commented Nov 6, 2018

RalfJung commented Nov 6, 2018

kennytm commented Nov 6, 2018

bors commented Nov 6, 2018

RalfJung commented Nov 6, 2018

oli-obk commented Nov 6, 2018

bors commented Nov 6, 2018

bors commented Nov 7, 2018

bors commented Nov 7, 2018

RalfJung commented Oct 31, 2018 •

edited

Loading

oli-obk Nov 2, 2018 •

edited

Loading