Split up miri into the librustc_mir and bin parts #268

oli-obk · 2017-07-21T11:44:22Z

cc @RalfJung this will break your rebasing

I adjusted all the imports so they appear like they would inside librustc_mir. For now there's a miri crate that only reexports the librustc_mir::interpret contents. This is so that I can write a Machine trait which will allow us to rip out everything non CTFE from the librustc_mir and place it back into miri.

Next step: break all lines longer than 100 chars.

fixes #275

RalfJung · 2017-07-22T07:25:13Z

How do you feel about accepting some of my validation stuff even though it is currently dead code because rustc does not yet have the validation commands? Would you be fine with me experimenting in-tree (the design of the validation statements is certainly not yet final)? Also see rust-lang/rust#43403.
Rebasing is getting tiresome.^^

oli-obk · 2017-07-22T07:40:32Z

I am absolutely fine with it, but how can it be done? The matches on Statement will fail, or do you mean just the relevant functions?

Alternatively I can offer running your branch in CI so any miri change has to build a stage 1 rebase your miri over the PR and run the tests. That would keep it out of tree, but ensure that it's the PR author's (my) job to create a new branch with your code rebased over the PR.

Depends on your projected timeline on getting your rustc PR merged. If it's a week or two, let's just get your dead code in here, if it's a few months, the other solution might be better( but for convenience it would also take all the code that compiles into miri)

You might even be able to get all your code merged under a cargo feature. Not sure if cfgs can create match arms

RalfJung · 2017-07-22T21:15:24Z

but how can it be done? The matches on Statement will fail, or do you mean just the relevant functions?

We could merge only the relevant new bits in memory.rs, or we could also merge the new file validation.rs which contains most of the rest of the new stuff. (The git history involves lots of changes in lvalue.rs, but almost all of that is later moved to validation.rs. I am not sure if you prefer a clean or a true history. ;) Once rust-lang/rust#43403 lands the small patch to hook this all up with the statements could be added. Notice that none of this changes existing behavior; rustc only keeps the new statements around when -Z mir-emit-validate is set.

Depends on your projected timeline on getting your rustc PR merged. If it's a week or two, let's just get your dead code in here, if it's a few months, the other solution might be better( but for convenience it would also take all the code that compiles into miri)

That mostly depends on the compiler team. ;)

oli-obk · 2017-07-24T07:27:51Z

I am not sure if you prefer a clean or a true history

true is fine by me.

So I suggest you open a PR with your branch, and we merge that before messing further with the structure of miri.

oli-obk · 2017-07-24T12:25:04Z

@eddyb This PR now has a minimal Machine trait + its miri and const eval impls.

More methods will follow.

eddyb · 2017-07-24T18:47:52Z

src/librustc_mir/interpret/const_eval.rs

@@ -76,3 +78,21 @@ pub fn eval_body_as_integer<'a, 'tcx>(
        _ => return Err(EvalError::NeedsRfc("evaluating anything other than isize/usize during typeck".to_string())),
    })
 }
+
+struct Evaluator;


I'd put Const in its name. Or CompileTime.

OK. Otherwise this is along the lines you had in mind?

Looks good to me!

RalfJung · 2017-07-25T17:21:57Z

miri/src/lib.rs

+}
+
+//struct Memory<'b, 'a: 'b, 'tcx: 'a>(&'b rustc_miri::interpret::Memory<'a, 'tcx, Evaluator>);
+struct MemoryMut<'b, 'a: 'b, 'tcx: 'a>(&'b mut rustc_miri::interpret::Memory<'a, 'tcx, Evaluator>);


What is the purpose of this newtype?

(And same for the other newtype above.)

It allows writing methods for EvalContext. If we wrote an extension trait, we'd have to duplicate every single method on the trait and the impl.

I see, makes sense. I find it a little funny that this ends up passing around &mut &mut ..., but I guess that's needed to get automatic reborrowing.

For consistency, shouldn't this one be called Memory (just like the other one is called EvalContext)?

Ok... I gave up on the newtype idea. It gets messy fast. We're back to extension traits. While those create some duplication of the function headers, they are so much more convenient to use.

RalfJung · 2017-07-25T17:24:53Z

I think I like the overall design, but I am not sure because I cannot see any structure in miri/src/lib.rs. ;) That file is rather... messy. I haven't even found the place yet where it says that this particular Machine impl is supposed to be used.

RalfJung · 2017-07-25T22:15:14Z

So the folder structure is such that we have src/librustc_mir and miri/src? That looks a little funny.
It'd be great if there could be some folder that has a Cargo.toml and that contains everything. For example, what about putting miri's Cargo.toml into the root?

RalfJung · 2017-07-25T23:17:03Z

I was thinking of something like this: RalfJung@c019e50

oli-obk · 2017-07-26T05:54:05Z

The librustc_Mir folder structure is mirroring the rustc repository in order to simplify the merger into rustc. I'd rather not change the folder structure, but I can place a Cargo.toml into the root instead of in the miri folder

RalfJung · 2017-07-26T18:05:08Z

The librustc_Mir folder structure is mirroring the rustc repository in order to simplify the merger into rustc.

Oh, I see. So you plan to use git-filter or so to then extract just this subfolder? That would still kill most of the history though.

RalfJung · 2017-07-26T18:14:27Z

So what about RalfJung@fa9a15f? The idea is also to have the folder structure as much as possible already like how we'd like to have it after the merge -- the only exception being that the miri source is in miri, not src. It could be in src, but that'd be rather confusing with librustc_mir also being there.

oli-obk · 2017-07-26T21:42:12Z

Sgtm. I already have this rebased over master locally. I'll apply the move from your commit tomorrow and push it here.

but I am not sure because I cannot see any structure in miri/src/lib.rs. ;) That file is rather... messy. I haven't even found the place yet where it says that this particular Machine impl is supposed to be used.

I have split the file into clearer components. Will also include it in this PR

RalfJung · 2017-07-26T21:43:33Z

Sgtm. I already have this rebased over master locally. I'll apply the move from your commit tomorrow and push it here.

Alternatively, push this somewhere and I can do the move again.

oli-obk · 2017-07-28T13:39:28Z

Down to 7k lines of code in interpret. Slowly moving towards a manageable size for the rustc merger.

oli-obk · 2017-07-28T14:52:58Z

Ok. I think we have reached everything for the merger.

RalfJung

Other than that, this looks good! It's a large patch though, so there's probably things I missed.

RalfJung · 2017-07-28T17:51:23Z

miri/lib.rs

+        // FIXME: replace loop by some structure that works with stepping
+        while let Some((instance, ptr, key)) = dtor {
+            trace!("Running TLS dtor {:?} on {:?}", instance, ptr);
+            // TODO: Potentially, this has to support all the other possible instances? See eval_fn_call in terminator/mod.rs


This file name reference needs updating, I presume?

RalfJung · 2017-07-28T17:51:58Z

miri/lib.rs

+    fn load_tls(&mut self, key: TlsKey) -> EvalResult<'tcx, Pointer>;
+    fn store_tls(&mut self, key: TlsKey, new_data: Pointer) -> EvalResult<'tcx>;
+    fn fetch_tls_dtor(&mut self, key: Option<TlsKey>) -> EvalResult<'tcx, Option<(ty::Instance<'tcx>, Pointer, TlsKey)>>;
+}


IMHO the TLS stuff should get its own file.

it has its own file now

RalfJung · 2017-07-28T17:52:38Z

miri/lib.rs

+        let size = ecx.type_size(ty)?.expect("box only works with sized types");
+        let align = ecx.type_align(ty)?;
+        if size == 0 {
+            Ok(PrimVal::Bytes(align.into()))


What happened with the FIXME to call the lang item instead?

got lost, readded

RalfJung · 2017-07-28T17:54:29Z

miri/missing_fns.rs

+
+        match &path[..] {
+            // Allocators are magic.  They have no MIR, even when the rest of libstd does.
+            "alloc::heap::::__rust_alloc" => {


This is an exact duplicate of some code further up in call_fn.

whoops, copy paste

RalfJung · 2017-07-28T17:56:25Z

src/librustc_mir/interpret/eval_context.rs


    /// The virtual memory system.
-    pub(crate) memory: Memory<'a, 'tcx>,
+    pub memory: Memory<'a, 'tcx, M>,

    #[allow(dead_code)]
    // FIXME(@RalfJung): validation branch
    /// Lvalues that were suspended by the validation subsystem, and will be recovered later
    pub(crate) suspended: HashMap<DynamicLifetime, Vec<ValidationQuery<'tcx>>>,


So the plan is to keep the validation stuff for CTFE, or to factor it out later?

I'd leave it in, I don't think we want to allow UB in CTFE ;)

RalfJung · 2017-07-28T17:59:34Z

src/librustc_mir/interpret/operator.rs

@@ -266,6 +203,9 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
            (Div, F64) => f64_arithmetic!(/, l, r),
            (Rem, F64) => f64_arithmetic!(%, l, r),

+            (Eq, _) => PrimVal::from_bool(l == r),
+            (Ne, _) => PrimVal::from_bool(l != r),


Why is this needed now?

Related to your other comment, ptr_op doesn't handle integer ops anymore.

I see. ptr_op still pretty much has to support them (well, at least in its type), but I guess for the purpose of CTFE it makes sense to split this.

RalfJung · 2017-07-28T18:01:04Z

src/librustc_mir/interpret/const_eval.rs

+        _left_ty: Ty<'tcx>,
+        _right: PrimVal,
+        _right_ty: Ty<'tcx>,
+    ) -> EvalResult<'tcx, Option<(PrimVal, bool)>> {


If I read the code in operator.rs correctly, this function is always called, not just on pointers (and after all, integers could be pointers). So isn't this failing too much?

I think what I'd expect is a test whether _left or _right is a Ptr value. If both are Bytes, we should return Ok(None) rather than Err.

I decided to change the API that either left or right must be a Ptr.

I think my reasoning was that in the future you'll want integers to go through the ptr_op API to properly handle the intptr model

Oh... we can't do this. libstd calls e.g. Offset on two Bytes for zsts. There's more ops that would fail.... And now I'm just realizing I misunderstood your original meaning. Fixing properly

RalfJung · 2017-07-28T18:02:41Z

src/librustc_mir/interpret/step.rs

+// this includes any method that might access the stack
+// basically don't call anything other than `load_mir`, `alloc_ptr`, `push_stack_frame`
+// The reason for this is, that `push_stack_frame` modifies the stack out of obvious reasons
+struct ConstantExtractor<'a, 'b: 'a, 'tcx: 'b, M: Machine<'tcx> + 'a> {


I never really understood this comment. Why does the fact that push_stack_frame push a stack frame entail that all methods on this type (?!?) should not access the stack? What exactly goes wrong if they do?

(This doesn't have to be solved by this PR.)

Well... the issue is with functions like type_size, which access frame().instance.substs. If you keep expecting such a function to do something useful, you're gonna get suprised.

RalfJung · 2017-07-28T18:07:58Z

Cargo.toml

@@ -35,4 +35,4 @@ compiletest_rs = "0.2.6"

 [workspace]
 members = ["src/librustc_mir"]
-exclude = ["xargo"]
+exclude = ["xargo", "cargo-miri-test"]


The members line is probably redundant; everything below this folder is in the workspace automatically.

RalfJung

I like how miri/lib.rs is getting short and only concerned with plumbing. I think that's how it should be. :)

RalfJung · 2017-08-01T03:37:43Z

miri/lib.rs

+                dtor @ Some(_) => dtor,
+                None => self.memory.fetch_tls_dtor(None)?,
+            };
+        }


This loop is TLS code, should also move to the TLS file.

RalfJung · 2017-08-01T03:42:36Z

src/librustc_mir/interpret/operator.rs

-                }
-                _ => {}
+            if let Some(handled) = M::ptr_op(self, bin_op, left, left_ty, right, right_ty)? {
+                return Ok(handled);


The name doesn't really fit right now, does it? This is called for all int and/or ptr ops after all.

I went through the same trouble when I extended miri to handle more ptr ops while trying to keep the code clean. Separating ptr from int really doesn't make too much sense, except for CTFE.

Maybe a better alternative would be to call it only if either operand is a pointer or the operation is Offset (this being the only primitive operation that takes arguments of pointer type)? The function should then probably not return an Option, but for intptrcast it would need to have a chance to mutate the values (i.e., turning them into integers). Not sure though. (Note that, as far as I can tell, the current API would require code duplication for intptrcast -- the code below handling integer operations could not be reused.)

Alternatively, the comment documenting ptr_op in trait Machine should say that this is always called, not just when pointers are involved.

Note that, as far as I can tell, the current API would require code duplication for intptrcast -- the code below handling integer operations could not be reused.

I'd assume that PrimVal::to_bytes would take a Memory argument and do the logic in there.

I renamed the method to try_ptr_op and changed the documentation to reflect the changes.

I'd assume that PrimVal::to_bytes would take a Memory argument and do the logic in there.

Hm. Thinking about it, to model intptrcast faithfully we probably want to eagerly do the allocation when the cast happens, or when the transmute happens -- arithmetic operations should never have to do that, as that would mean they have side-effects and optimizations become horrible.

Anyway, we can discuss that in the intptrcast PR, whenever that happens.

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.

RalfJung

LGTM!

oli-obk force-pushed the upstream branch from 6e67607 to 123a203 Compare July 21, 2017 12:04

oli-obk force-pushed the upstream branch from 123a203 to 84281e8 Compare July 22, 2017 14:16

oli-obk force-pushed the upstream branch from 84281e8 to 6441257 Compare July 24, 2017 11:29

eddyb reviewed Jul 24, 2017

View reviewed changes

RalfJung reviewed Jul 25, 2017

View reviewed changes

oli-obk force-pushed the upstream branch 3 times, most recently from 3e7218b to 93a7aeb Compare July 28, 2017 11:09

oli-obk force-pushed the upstream branch from 6d4a436 to 45cd6b4 Compare July 28, 2017 14:57

RalfJung requested changes Jul 28, 2017

View reviewed changes

RalfJung reviewed Jul 28, 2017

View reviewed changes

oli-obk force-pushed the upstream branch 2 times, most recently from fb80f81 to e4212a6 Compare July 31, 2017 13:51

RalfJung requested changes Aug 1, 2017

View reviewed changes

Split up miri into the librustc_mir and bin parts

11e30a6

oli-obk and others added 18 commits August 1, 2017 09:56

Implement the first machine function

36502a3

Split "new" miri crate into modules

dc6e877

Move more non-CTFE operations to the Machine

4a4640a

Rename const_eval::Evaluator to CompileTimeFunctionEvaluator

960dca1

Move the global_item function to the EvalContext

728bb87

Fix doc text

e15d374

Move a method used everywhere out of intrinsic.rs

df7c42b

Update env_logger in order to share more dependencies

45b7cfd

Move all intrinsics out of interpret and fail CTFE on intrinsic calls

f16b9e2

Adjust travis to the directory changes

4383fc2

Update Cargo.toml

7ed706d

Allow machines to create new memory kinds

adfea61

Move tls code to its own file

23d351d

Remove leftover unused file

54ecdf1

Remove redundant members field

3d82b66

Properly process ptr_op for const eval

8de8580

Address comments

d05784f

oli-obk force-pushed the upstream branch from 62d763e to d05784f Compare August 1, 2017 09:12

RalfJung approved these changes Aug 2, 2017

View reviewed changes

oli-obk merged commit 7700bd0 into rust-lang:master Aug 2, 2017

oli-obk deleted the upstream branch August 2, 2017 07:30

Split up miri into the librustc_mir and bin parts #268

Split up miri into the librustc_mir and bin parts #268

Conversation

oli-obk commented Jul 21, 2017 • edited Loading

RalfJung commented Jul 22, 2017

oli-obk commented Jul 22, 2017

RalfJung commented Jul 22, 2017

oli-obk commented Jul 24, 2017

oli-obk commented Jul 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb Jul 24, 2017 • edited Loading

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 Jul 25, 2017

RalfJung commented Jul 25, 2017

RalfJung commented Jul 25, 2017 • edited Loading

oli-obk commented Jul 26, 2017

RalfJung commented Jul 26, 2017

RalfJung commented Jul 26, 2017

oli-obk commented Jul 26, 2017

RalfJung commented Jul 26, 2017

oli-obk commented Jul 28, 2017

oli-obk commented Jul 28, 2017

RalfJung 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

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

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

RalfJung left a comment

Choose a reason for hiding this comment

oli-obk commented Jul 21, 2017 •

edited

Loading

eddyb Jul 24, 2017 •

edited

Loading

RalfJung commented Jul 25, 2017 •

edited

Loading