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

Value visitors for miri #55549

Merged
merged 21 commits into from Nov 7, 2018

Conversation

Projects
None yet
6 participants
@RalfJung
Member

RalfJung commented Oct 31, 2018

Generalize the traversal part of validation to a ValueVisitor.

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

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Oct 31, 2018

r? @oli-obk

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

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Oct 31, 2018

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

This comment has been minimized.

Member

RalfJung commented Oct 31, 2018

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

Show resolved Hide resolved src/librustc_mir/interpret/visitor.rs Outdated
Show resolved Hide resolved src/librustc_mir/interpret/visitor.rs Outdated
// 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>)

This comment has been minimized.

@oli-obk

oli-obk Nov 1, 2018

Contributor

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

This comment has been minimized.

@RalfJung

RalfJung Nov 1, 2018

Member

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).

This comment has been minimized.

@RalfJung

RalfJung Nov 1, 2018

Member

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

This comment has been minimized.

@RalfJung

RalfJung Nov 1, 2018

Member

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

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

This comment has been minimized.

@RalfJung

RalfJung Nov 1, 2018

Member

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

Show resolved Hide resolved src/librustc_mir/interpret/visitor.rs Outdated
Show resolved Hide resolved src/librustc_mir/interpret/visitor.rs Outdated
self.value().layout()
}
// Replace the value by `val`, which must be the `field`th field of `self`, then call

This comment has been minimized.

@oli-obk

oli-obk Nov 1, 2018

Contributor

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.

This comment has been minimized.

@RalfJung

RalfJung Nov 1, 2018

Member

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.

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

This comment has been minimized.

@oli-obk

oli-obk Nov 1, 2018

Contributor

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

This comment has been minimized.

Contributor

bors commented Nov 1, 2018

☔️ 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

Show resolved Hide resolved src/librustc_mir/interpret/visitor.rs Outdated
Show resolved Hide resolved src/librustc_mir/interpret/visitor.rs Outdated
Show resolved Hide resolved src/librustc_mir/interpret/visitor.rs Outdated
Show resolved Hide resolved src/librustc_mir/interpret/visitor.rs Outdated
{
self.walk_value(v)
}
/// Visit the given value as a union.

This comment has been minimized.

@oli-obk

oli-obk Nov 2, 2018

Contributor
Suggested change Beta
/// Visit the given value as a union.
/// Visit the given value as a union. Does not recurse unless overwritten to do so.
Show resolved Hide resolved src/librustc_mir/interpret/visitor.rs Outdated
// Actions on the leaves, ready to be overloaded.
/// Called whenever we reach a value with uninhabited layout.
/// Recursing to fields will continue after this!

This comment has been minimized.

@oli-obk

oli-obk Nov 2, 2018

Contributor

how can an uninhabited type have fields?

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!

This comment has been minimized.

@oli-obk

oli-obk Nov 2, 2018

Contributor

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.

This comment has been minimized.

@RalfJung

RalfJung Nov 2, 2018

Member

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.

This comment has been minimized.

@oli-obk

oli-obk Nov 2, 2018

Contributor

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

This comment has been minimized.

@RalfJung

RalfJung Nov 2, 2018

Member

Like so?

@RalfJung RalfJung force-pushed the RalfJung:miri-visitor branch from e27a04e to bebf782 Nov 2, 2018

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 2, 2018

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

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 2, 2018

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

@RalfJung RalfJung force-pushed the RalfJung:miri-visitor branch 3 times, most recently from 09e4551 to 7f93557 Nov 2, 2018

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 2, 2018

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

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 2, 2018

@bors try

@bors

This comment has been minimized.

Contributor

bors commented Nov 2, 2018

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

bors added a commit that referenced this pull request Nov 2, 2018

Auto merge of #55549 - RalfJung:miri-visitor, r=<try>
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

This comment has been minimized.

Contributor

bors commented Nov 2, 2018

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

@oli-obk

This comment has been minimized.

Contributor

oli-obk commented Nov 2, 2018

@rust-timer

This comment has been minimized.

rust-timer commented Nov 2, 2018

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

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

This comment has been minimized.

@oli-obk

oli-obk Nov 2, 2018

Contributor

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

@oli-obk

oli-obk approved these changes Nov 2, 2018

@bors

This comment was marked as resolved.

Contributor

bors commented Nov 4, 2018

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

@rust-timer

This comment has been minimized.

rust-timer commented Nov 6, 2018

Finished benchmarking try commit 5f8bd0a

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 6, 2018

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

Maybe we should just try again.
@bors try

@bors

This comment has been minimized.

Contributor

bors commented Nov 6, 2018

⌛️ Trying commit 3538532 with merge ad5b1b3...

bors added a commit that referenced this pull request Nov 6, 2018

Auto merge of #55549 - RalfJung:miri-visitor, r=<try>
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.
@kennytm

This comment has been minimized.

Member

kennytm commented Nov 6, 2018

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

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 6, 2018

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

@kennytm

This comment has been minimized.

Member

kennytm commented Nov 6, 2018

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

@bors

This comment has been minimized.

Contributor

bors commented Nov 6, 2018

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

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 6, 2018

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

@RalfJung RalfJung force-pushed the RalfJung:miri-visitor branch from 3538532 to 7b7c6ce Nov 6, 2018

@oli-obk

This comment has been minimized.

Contributor

oli-obk commented Nov 6, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 6, 2018

📌 Commit 7b7c6ce has been approved by oli-obk

@bors

This comment has been minimized.

Contributor

bors commented Nov 7, 2018

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

bors added a commit that referenced this pull request Nov 7, 2018

Auto merge of #55549 - RalfJung:miri-visitor, r=oli-obk
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

This comment has been minimized.

Contributor

bors commented Nov 7, 2018

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

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

2 checks passed

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

@RalfJung RalfJung deleted the RalfJung:miri-visitor branch Nov 9, 2018

bors added a commit that referenced this pull request Nov 15, 2018

Auto merge of #55716 - RalfJung:escape-to-raw, r=oli-obk
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.~~
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment