Infinite loop detection for const evaluation #51702

ecstatic-morse · 2018-06-22T08:01:08Z

Resolves #50637.

An EvalContext stores the transient state (stack, heap, etc.) of the MIRI virtual machine while it executing code. As long as MIRI only executes pure functions, we can detect if a program is in a state where it will never terminate by periodically taking a "snapshot" of this transient state and comparing it to previous ones. If any two states are exactly equal, the machine must be in an infinite loop.

Instead of fully cloning a snapshot every time the detector is run, we store a snapshot's hash. Only when a hash collision occurs do we fully clone the interpreter state. Future snapshots which cause a collision will be compared against this clone, causing the interpreter to abort if they are equal.

At the moment, snapshots are not taken until MIRI has progressed a certain amount. After this threshold, snapshots are taken every DETECTOR_SNAPSHOT_PERIOD steps. This means that an infinite loop with period P will be detected after a maximum of 2 * P * DETECTOR_SNAPSHOT_PERIOD interpreter steps. The factor of 2 arises because we only clone a snapshot after it causes a hash collision.

rust-highfive · 2018-06-22T08:01:19Z

r? @michaelwoerister

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

ecstatic-morse · 2018-06-22T08:01:50Z

r? @oli-obk

oli-obk

This is already pretty solid for the current state of const eval. Seems very hard to test though ;)

oli-obk · 2018-06-22T08:25:34Z

src/librustc_mir/interpret/eval_context.rs

@@ -33,11 +33,8 @@ pub struct EvalContext<'a, 'mir, 'tcx: 'a + 'mir, M: Machine<'mir, 'tcx>> {
    /// Bounds in scope for polymorphic evaluations.
    pub param_env: ty::ParamEnv<'tcx>,

-    /// The virtual memory system.


The machine field also needs to go into the EvalState. It's unused in the compiler, but miri the tool needs it

Got it, when I first started on this, I assumed that I'd need to provide some custom implementations of common methods on EvalState, as well as pass it by reference to something like HashSet::get. However, since I implemented custom Eq/Hash on Frame and Memory, observe could simply accept references to each individual field. Right now, whenever I look up an EvalState in a hash table, I'm either inserting it right after, or aborting const eval due to an infinite loop, so there's no need to get through a reference.

This means I could revert my first commit and not break everything, but if someday we need to look up an EvalState in a hash table without inserting it, we'd have to clone it.

Also, I could use #[derive] more as well as remove a lot of where bounds if I add Eq + Hash + Clone to the supertraits of Machine. What's rustc's general philosophy on this? I try to avoid needlessly adding supertraits, but here it would save a lot of boilerplate.

oli-obk · 2018-06-22T08:29:11Z

src/librustc_mir/interpret/memory.rs

@@ -992,12 +992,12 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> HasMemory<'a, 'mir, 'tcx, M> for Me
 impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> HasMemory<'a, 'mir, 'tcx, M> for EvalContext<'a, 'mir, 'tcx, M> {
    #[inline]
    fn memory_mut(&mut self) -> &mut Memory<'a, 'mir, 'tcx, M> {
-        &mut self.memory
+        self.memory_mut()


This feels very infinite recursion dangerous. Or at least surprising to read. Can we just get rid of the memory_mut and memory inherent methods in favour of this trait impl?

HasMemory is private to this crate, and tools/miri would need to call these methods.

The solution above would obviate this problem.

oli-obk · 2018-06-22T08:38:15Z

src/librustc_mir/interpret/eval_context.rs

+            stmt,
+        } = self;
+
+        ptr::eq(mir, &other.mir)


You need to check the instance for equality, because it contains the substs, which make up the generic arguments of the current frame's function. It's probably not problematic due to the upper frame's stmt field, but we should still have this PartialEq impl be correct.

This means you also don't have to check the Mir, because the Mir depends directly on the DefId inside the Instance.

Got it, I've read the rustc book, but some of the finer points eluded me.

oli-obk · 2018-06-22T08:39:06Z

src/librustc_mir/interpret/eval_context.rs

+        } = self;
+
+        ptr::eq(mir, &other.mir)
+            && *return_to_block == other.return_to_block // TODO: Are these two necessary?


Probably not in a global way (due to stmt on the upper stack frame), but still nice to have this impl be very correct.

oli-obk · 2018-06-22T08:39:21Z

src/librustc_mir/interpret/eval_context.rs

+            stmt,
+        } = self;
+
+        (mir as *const _ as usize).hash(state);


same here, hash the instance, not the mir.

oli-obk · 2018-06-22T08:40:33Z

src/librustc_mir/interpret/memory.rs

+            alloc_kind: self.alloc_kind.clone(),
+            alloc_map: self.alloc_map.clone(),
+            uninitialized_statics: self.uninitialized_statics.clone(),
+            cur_frame: self.cur_frame.clone(),


no need for .clone() here.

oli-obk · 2018-06-22T08:41:27Z

src/librustc_mir/interpret/memory.rs

+            && *alloc_map == other.alloc_map
+            && *uninitialized_statics == other.uninitialized_statics
+            && *cur_frame == other.cur_frame
+            && ptr::eq(tcx, &other.tcx)


no need to compare the tcx, there is only ever one, and the Span changing is not something we care about.

oli-obk · 2018-06-22T08:41:42Z

src/librustc_mir/interpret/memory.rs

+
+        data.hash(state);
+        cur_frame.hash(state);
+        (tcx as *const _ as usize).hash(state);


also no need to hash the tcx.

oli-obk · 2018-06-22T08:43:26Z

src/librustc_mir/interpret/memory.rs

+        cur_frame.hash(state);
+        (tcx as *const _ as usize).hash(state);
+
+        // We ignore some fields which don't change between evaluation steps.


You aren't really ignoring them, hashing the allocations iterator should get you most of the info.

This ignores uninitialized_statics at the moment, which actually can change during execution, but I think will not affect infinite loop detection, since modifying an uninitialized static would cause it to no longer be uninitialized. I could be way off here though.

That's dangerous. Equality and hashing need to be in sync. I don't remember the exact rules, but you can read up on that in the hash docs

uninitialized_statics (along with another field) is ignored in the hash, but not in the equality check. This maintains the invariant required by Hash: that two objects with different hashes never compare as equal.

This should be fixed anyways, but I need to know how to compare FxHashMap<AllocId, Allocation>s against one another.

Right. That makes sense

oli-obk · 2018-06-22T08:49:41Z

src/librustc_mir/interpret/memory.rs

+        self.allocations()
+            .map(|allocs| {
+                let mut h = FxHasher::default();
+                allocs.hash(&mut h);


So this is problematic. Hashing an AllocId (inside a relocation of the alloc) will hash the actual Id. This means that if we ever call a function in a loop, since we keep getting new AllocIds for the function's locals, we'll never realize we're in a loop (ok we will outside the function, but imagine the function returning a raw pointer to its local, and we keep updating a field in the outer function).

I think it's fine for now, and will probably go away once we get around to implementing local AllocIds.

So right now, this hashes both the AllocId and the Allocation (which can be crazy slow btw). Would simply hashing Allocation be correct?

I could maybe improve performance by memoizing the hash of an Allocation. This would involve setting a Cell<Option<u64>> to None whenever an Allocation is mutated, even when the loop detector isn't running. This might not be too bad, since there's no immediate data dependency on the cached value, and it will often be None during regular execution so the branch predictor will be on our side.

We could benchmark it. For now let's do the slow version. We are already in "this might take a while territory"

We might not even have to hash the memory, if we transitively hash through AllocIds by hashing their allocation instead of the id

Agreed, I'm a sucker for a premature optimization :).

transitively hash through AllocIds by hashing their allocation instead of the id

Can you elaborate on this? I don't quite understand.

I'm caught up now; I'll repeat what you said for my own clarity.

alloc_map contains all Allocations which are visible to a given EvalContext indexed by AllocId. The naive approach, comparing the alloc_maps with eq, will correctly identify some infinite loops. However, if an object is reallocated, and all pointers to it are updated to point to the new value, the program states are equivalent but our equality function will fail.

Instead, we can look at the program heap as a graph of allocations whose roots are Value::ByRefs on the stack and whose edges are the AllocIds in Relocations. Equality over EvalSnaphost Frames is the same except when it comes to these ByRef variants. We would push them onto a separate list for each frame, then do a simultaneous BFS/DFS, comparing each Allocation along the way. This would make equality invariant over changing AllocIds.

It is much easier to implement Hash than PartialEq with this approach: we simply omit any AllocIds--either on the stack in a Value::ByRef or the heap in a Relocations--from the hash.

I think I will implement this naively at first (not handling object reallocation), then try to come up with a test case where the more robust case would be necessary to detect infinite loops.

Also, I belive that the result of casting a reference to usize during CTFE is still up in the air. If the AllocId is used for that, the more complex strategy will no longer be correct.

The following test case fails as you predicted.

#![feature(const_fn, const_let)] const fn churn() -> *const u8 { let x = 0u8; &x } fn main() { let _ = [(); { let mut y = 0 as *const u8; loop { y = churn(); } y as usize }]; }

There must be a subtler way of triggering this, but I can't come up with one at the moment. I don't think this class of programs is worth implementing a custom equality function which ignores AllocIds. Let me know if you disagree.

We can at least check for each AllocId that we hash, whether it is dangling, and then just hash some default value (e.g. u64::max_value), unfortunately that requires the same changes as walking down AllocIds...

If you open an issue about the not working example and the explanation of the full fix, we can merge this PR as it is.

I'll open an issue. I plan to keep working on this btw.

bjorn3

Why did you replace stack() with stack_mut() and memory() with memory_mut() everywhere?

bjorn3 · 2018-06-22T12:25:49Z

src/librustc_mir/interpret/memory.rs

@@ -53,6 +54,85 @@ pub struct Memory<'a, 'mir, 'tcx: 'a + 'mir, M: Machine<'mir, 'tcx>> {
    pub tcx: TyCtxtAt<'a, 'tcx, 'tcx>,
 }

+impl<'a, 'mir, 'tcx, M> Clone for Memory<'a, 'mir, 'tcx, M>


Can this be derived?

Should be possible after the changes I suggested where the machine field of the EvalContext becomes part of the state.

@bjorn3 Because I chose to refactor the fields of EvalContext into a separate struct so that it could have a custom hash impl and be looked up by reference in a hash table. See my comment for why this might be overkill. Once you do that, everything that was referring to ctxt.stack had to be changed to either ctxt.stack{,_mut}() or ctxt.state.stack.

I did this explicitly because deriving Clone for Memory would add a where M: Clone bound to the implementation, even though only where M::MemoryData: Clone is needed. As @oli-obk said, any part of the evaluation state must be Clone, so I should be able to cut down on the verbosity.

ecstatic-morse · 2018-06-22T20:52:49Z

I reverted the EvalContext layout changes so that I don't cause so much churn by changing field accesses to method calls everywhere. I can always reapply that patch later on. Now observe_and_analyze just takes a reference to each field which is significant for execution state (this now includes a Machine).

This comment still needs to be resolved regarding what parts of Memory should be hashed.

My next steps are to:

Add an EvalErrorKind::InfiniteLoop variant
Implement Clone, Eq, and Hash on the Evaluator in tools/miri
~~Execute this on every loop iteration instead of once per aggregate rvalue?~~
Actually enable the loop detector in step
Figure out how on earth I'm going to test for false positives :)

oli-obk · 2018-06-23T08:41:00Z

The ADT value increase is fine as it is. You will have a gap in your step IDs, but that should be fine

ecstatic-morse · 2018-06-24T23:15:10Z

So I've implemented, enabled and added a (successful) test for a version of the loop detector without any of the performance improvements mentioned above.

I can't tell if the M: Machine parameter to EvalContext really needs to be a part of the state: the only two implementers are here and here. The first is an empty struct and it's not clear to me why the second one is transient. Since running the loop detector requires M: Eq + Clone + Hash, this PR will break miri until I figure out what the proper equality semantics are for its Machine implementation.

Also, since the loop counter is sometimes incremented by more than one, it's possible for the loop detector to fail to when DETECTOR_SNAPSHOT_PERIOD equals the increment value during the course of an infinite loop. Since this is just a false negative and pretty hard to trigger, it's not a big deal. Being able to increment the step counter by more than one does complicate some optimizations of the loop detector though. I don't quite understand why an interpreter step doesn't always equal evaluating a terminator.

Finally, as @oli-obk mentioned, it's really hard to test for false positives. It's possible that I'm not taking some part of the execution state into account, and two snapshots which compare as equal will not actually cause an infinite loop. I guess the way you usually deal with is a crater run? Not sure if that would be appropriate here...

bjorn3 · 2018-06-25T06:25:45Z

src/librustc_mir/interpret/eval_context.rs

@@ -46,7 +46,7 @@ pub struct EvalContext<'a, 'mir, 'tcx: 'a + 'mir, M: Machine<'mir, 'tcx>> {

    /// The number of terminators to be evaluated before enabling the infinite
    /// loop detector.
-    pub(crate) steps_until_detector_enabled: usize,
+    pub(crate) steps_until_detector_enabled: isize,


Could you please document that negative values represent the amount of steps after the detector is enabled.

Yes, this could be clearer.

My plan is to invert this counter (e.g. steps_since_detector_enabled), so I can use an add, then mask off the loop detector period if steps is greater than 0. The code in inc_step_counter currently only works with a detector period of exactly 1 due to the saturating sub. I was waiting for some clarity on the steps parameter to find the best way to do this.

I'm trying to make the point at which the detector comes on a comparison with 0 since it saves a cycle or two. This is probably pretty dumb :) See previous comments regarding my affinity for premature optimization.

oli-obk · 2018-06-25T08:45:49Z

src/test/ui/const-eval/infinite_loop.stderr

+LL | |         let mut n = 113383; // #20 in A006884
+LL | |         while n != 0 { //~ ERROR constant contains unimplemented expression type
+LL | |             n = if n % 2 == 0 { n/2 } else { 3*n + 1 };
+   | |                    ---------- program will never terminate


I think it would be prudent to add more info here. Like that the highlighted expression is simply the first expression found where the constant evaluator could with certainty say that the evaluation will not terminate.

I agree, this is a bit obtuse. What about expanding the span of the error to be the entire block? I'll need to do some more reading into the diagnostics parts of rustc to implement that. I could also replace it with something like "Infinite loop detected here, const evaluation will never terminate." But it's hard to make clear why this particular statement is being singled out.

We can't really figure out which part is at fault. I think there's no nice solution except explain more things in the message.

oli-obk · 2018-06-25T08:52:26Z

The first is an empty struct and it's not clear to me why the second one is transient.

Well... the environment variables can be changed. So if you're writing a loop by increasing the value inside an environment variable, then you can have a loop where all state in the main memory is the same.

Also, since the loop counter is sometimes incremented by more than one, it's possible for the loop detector to fail to when DETECTOR_SNAPSHOT_PERIOD equals the increment value during the course of an infinite loop. Since this is just a false negative and pretty hard to trigger, it's not a big deal. Being able to increment the step counter by more than one does complicate some optimizations of the loop detector though. I don't quite understand why an interpreter step doesn't always equal evaluating a terminator.

Uhm.... because that's a refactoring mess-up (by me) ;)

Just remove that call in aggregate creation and the usize argument to the stepping method

ecstatic-morse · 2018-06-25T18:32:50Z

@oli-obk You got it!

I saw the comment:

/// Miri does not expose env vars from the host to the emulated program

And thought that meant that no environment variables would be exposed to the emulated program, but I see now that MIRI emulates environment variables.

I'm not sure what all the cases are for MIRI, but it seems like infinite loop detection is only really desirable in CTFE. Do you think I should add a way for MIRI to enable/disable this feature?

oli-obk · 2018-06-25T18:37:12Z

I'm not sure what all the cases are for MIRI, but it seems like infinite loop detection is only really desirable in CTFE. Do you think I should add a way for MIRI to enable/disable this feature?

You mean because it might end up being slow? Miri can just set the counter to u64::max_value(), that should not ever reach 0 xD. We can cross that bridge when it becomes a problem.

bjorn3 · 2018-06-27T06:17:25Z

src/test/ui/const-eval/infinite_loop.stderr

+LL | |         let mut n = 113383; // #20 in A006884
+LL | |         while n != 0 { //~ ERROR constant contains unimplemented expression type
+LL | |             n = if n % 2 == 0 { n/2 } else { 3*n + 1 };
+   | |                    ---------- duplicate interpreter state observed while executing this expression, const evaluation will never terminate


I think this label is too long.

Maybe s/while executing this expression/here

Agreed, that's better

bjorn3 · 2018-06-27T06:19:06Z

src/test/ui/const-eval/infinite_loop.rs

+    let _ = [(); {
+        //~^ WARNING Constant evaluating a complex constant, this might take some time
+        //~| ERROR could not evaluate repeat length
+        let mut n = 113383; // #20 in A006884


What is A006884?

https://oeis.org/A006884. Is this too cute? I wanted something that was more complex than just loop{}.

In my opinion this is a fine example. Could you add the link to the comment though?

rust-highfive · 2018-06-29T22:03:48Z

The job x86_64-gnu-llvm-3.9 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:47:00] 
[00:47:00] running 1535 tests
[00:47:06] ................................................................................................i...
[00:47:13] .............................................................i......................................
[00:47:18] ...................................F................................................................
[00:47:25] ....................................................................................................
[00:47:28] ....................................................................................................
[00:47:34] ....................................................................................................
[00:47:38] ....................................................................................................
---
[00:48:21] ---- [ui] ui/const-eval/infinite_loop.rs stdout ----
[00:48:21] diff of stderr:
[00:48:21] 
[00:48:21] 13    |  __________________^
[00:48:21] 14 LL | |         //~^ WARNING Constant evaluating a complex constant, this might take some time
[00:48:21] 15 LL | |         //~| ERROR could not evaluate repeat length
[00:48:21] - LL | |         let mut n = 113383; // #20 in A006884
[00:48:21] + LL | |         let mut n = 113383; // #20 in https://oeis.org/A006884
[00:48:21] 17 ...  |
[00:48:21] 18 LL | |         n
[00:48:21] 19 LL | |     }];
[00:48:21] 26    |  __________________^
[00:48:21] 26    |  __________________^
[00:48:21] 27 LL | |         //~^ WARNING Constant evaluating a complex constant, this might take some time
[00:48:21] 28 LL | |         //~| ERROR could not evaluate repeat length
[00:48:21] - LL | |         let mut n = 113383; // #20 in A006884
[00:48:21] + LL | |         let mut n = 113383; // #20 in https://oeis.org/A006884
[00:48:21] 30 LL | |         while n != 0 { //~ ERROR constant contains unimplemented expression type
[00:48:21] 31 LL | |             n = if n % 2 == 0 { n/2 } else { 3*n + 1 };
[00:48:21] 32    | |                    ---------- n's value must be known at compile-time. Erroneous code\nexample:\n\n```compile_fail\nenum Test {\n    V1\n}\n\nimpl Test {\n    fn test(&self) -> i32 {\n        12\n    }\n}\n\nfn main() {\n    const FOO: Test = Test::V1;\n\n    const A: i32 = FOO.test(); // You can't call Test::func() here!\n}\n```\n\nRemember: you can't use a function call inside a const's initialization\nexpression! However, you can totally use it anywhere else:\n\n```\nenum Test {\n    V1\n}\n\nimpl Test {\n    fn func(&self) -> i32 {\n        12\n    }\n}\n\nfn main() {\n    const FOO: Test = Test::V1;\n\n    FOO.func(); // here is good\n    let x = FOO.func(); // or even here!\n}\n```\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/const-eval/infinite_loop.rs","byte_start":878,"byte_end":1016,"line_start":20,"line_end":22,"column_start":9,"column_end":10,"is_primary":true,"text":[{"text":"        while n != 0 { //~ ERROR constant contains unimplemented expression type","highlight_start":9,"highlight_end":81},{"text":"            n = if n % 2 == 0 { n/2 } else { 3*n + 1 };","highlight_start":1,"highlight_end":56},{"text":"        }","highlight_start":1,"highlight_end":10}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error[E0019]: constant contains unimplemented expression type\n  --> /checkout/src/test/ui/const-eval/infinite_loop.rs:20:9\n   |\nLL | /         while n != 0 { //~ ERROR constant contains unimplemented expression type\nLL | |             n = if n % 2 == 0 { n/2 } else { 3*n + 1 };\nLL | |         }\n   | |_________^\n\n"}
[00:48:21] {"message":"Constant evaluating a complex constant, this might take some time","code":null,"level":"warning","spans":[{"file_name":"/checkout/src/test/ui/const-eval/infinite_loop.rs","byte_start":666,"byte_end":1032,"line_start":16,"line_end":24,"column_start":18,"column_end":6,"is_primary":true,"text":[{"text":"    let _ = [(); {","highlight_start":18,"highlight_end":19},{"text":"        //~^ WARNING Constant evaluating a complex constant, this might take some time","highlight_start":1,"highlight_end":87},{"text":"        //~| ERROR could not evaluate repeat length","highlight_start":1,"highlight_end":52},{"text":"        let mut n = 113383; // #20 in https://oeis.org/A006884","highlight_start":1,"highlight_end":63},{"text":"        while n != 0 { //~ ERROR constant contains unimplemented expression type","highlight_start":1,"highlight_end":81},{"text":"            n = if n % 2 == 0 { n/2 } else { 3*n + 1 };","highlight_start":1,"highlight_end":56},{"text":"        }","highlight_start":1,"highlight_end":10},{"text":"        n","highlight_start":1,"highlight_end":10},{"text":"    }];","highlight_start":1,"highlight_end":6}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"warning: Constant evaluating a complex constant, this might take some time\n  --> /checkout/src/test/ui/const-eval/infinite_loop.rs:16:18\n   |\nLL |       let _ = [(); {\n   |  __________________^\nLL | |         //~^ WARNING Constant evaluating a complex constant, this might take some time\nLL | |         //~| ERROR could not evaluate repeat length\nLL | |         let mut n = 113383; // #20 in https://oeis.org/A006884\n...  |\nLL | |         n\nLL | |     }];\n   | |_____^\n\n"}
[00:48:21] {"message":"could not evaluate repeat length","code":{"code":"E0080","explanation":"\nThis error indicates that the compiler was unable to sensibly evaluate an\nconstant expression that had to be evaluated. Attempting to divide by 0\nor causing integer overflow are two ways to induce this error. For example:\n\n```compile_fail,E0080\nenum Enum {\n    X = (1 << 500),\n    Y = (1 / 0)\n}\n```\n\nEnsure that the expressions given can be evaluated as the desired integer type.\nSee the FFI section of the Reference for more information about using a custom\ninteger type:\n\nhttps://doc.rust-lang.org/reference.html#ffi-attributes\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/const-eval/infinite_loop.rs","byte_start":970,"byte_end":980,"line_start":21,"line_end":21,"column_start":20,"column_end":30,"is_primary":false,"text":[{"text":"            n = if n % 2 == 0 { n/2 } else { 3*n + 1 };","highlight_start":20,"highlight_end":30}],"label":"duplicate interpreter state observed here, const evaluation will never terminate","suggested_replacement":null,"suggestion_applicability":null,"expansion":null},{"file_name":"/checkout/src/test/ui/const-eval/infinite_loop.rs","byte_start":666,"byte_end":1032,"line_start":16,"line_end":24,"column_start":18,"column_end":6,"is_primary":true,"text":[{"text":"    let _ = [(); {","highlight_start":18,"highlight_end":19},{"text":"        //~^ WARNING Constant evaluating a complex constant, this might take some time","highlight_start":1,"highlight_end":87},{"text":"        //~| ERROR could not evaluoccurred: E0019, E0080.","code":null,"level":"","spans":[],"children":[],"rendered":"Some errors occurred: E0019, E0080.\n"}
[00:48:21] {"message":"For more information about an error, try `rustc --explain E0019`.","code":null,"level":"","spans":[],"children":[],"rendered":"For more information about an error, try `rustc --explain E0019`.\n"}
[00:48:21] ------------------------------------------
[00:48:21] 
[00:48:21] thread '[ui] ui/const-eval/infinite_loop.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3140:9
[00:48:21] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[00:48:21] test result: FAILED. 1529 passed; 1 failed; 5 ignored; 0 measured; 0 filtered out
[00:48:21] 
[00:48:21] 
[00:48:21] 
[00:48:21] 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/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-3.9/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" "3.9.1\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:48:21] 
[00:48:21] 
[00:48:21] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:48:21] Build completed unsuccessfully in 0:02:14
[00:48:21] Build completed unsuccessfully in 0:02:14
[00:48:21] Makefile:58: recipe for target 'check' failed
[00:48:21] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:01741b04
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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)

ecstatic-morse · 2018-06-30T01:09:52Z

Okay, I implemented the required traits in miri, made the changes requested by @bjorn3, and responded to @oli-obk's concern regarding using AllocIds for equality. There's definitely room for optimization as well as to add some smaller features (a hard cap on the number of iterations during const eval?), but I'm satisfied with the majority of this PR, so I'm removing the WIP tag.

Lemme know what you think!

bjorn3 · 2018-06-30T07:43:55Z

src/librustc_mir/interpret/eval_context.rs

-    ) -> EvalResult<'tcx> {
+    ) -> EvalResult<'tcx>
+        where M: Clone + Eq + Hash,
+    {


Maybe add this to Machine itself. Eg:

trait Machine: Clone + Eq + Hash { [...]

That seems fine, Machine isn't really used for anything but this.

bors · 2018-07-03T04:20:51Z

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

RalfJung · 2018-07-03T21:32:11Z

As long as MIRI only executes pure functions, we can detect if a program will never terminate by periodically taking a "snapshot" of this transient state and comparing it to previous ones. If any two states are exactly equal, the machine must be in an infinite loop.

However, it is possible to diverge without ever repeating a state -- if the program just has larger and larger states. So you are not reliably detecting all cases of programs never terminating, you just detect some cases. (Which you may be aware of, but the way you wrote this ["we can detect if a program will never terminate"] made it sound like you are claiming more than this can possibly do. :D )

oli-obk · 2018-07-03T21:44:02Z

We do detect nontermination. It might take us forever or the system ooms before we do, but in all other cases, at some point we've seen all possible States. Oom also happens faster due to the hashset building up.

On a more serious note: as long as we detect simple nontermination it's fine. The complex cases will still produce a warning, so users know something's off. Rustc already isn't guaranteed to terminate in it's typesystem and macro system, so...

ecstatic-morse · 2018-07-04T20:39:27Z

I changed the PR description to "we can detect if a program is in a state where it will never terminate" to avoid accusations that I've claimed to solve the halting problem.

Mark-Simulacrum · 2018-07-10T14:01:26Z

@rust-timer build cf47daa968f12c4f95111ce03aaf02b734cc78d7

Also added you to permissions.

rust-timer · 2018-07-10T14:01:28Z

Success: Queued cf47daa968f12c4f95111ce03aaf02b734cc78d7 with parent 4af9132, comparison URL.

kennytm · 2018-07-10T18:43:44Z

Perf is now ready. The timing changes are... mixed 😕 (inflate clean-opt @ +2.4% and syn clean-opt @ -1.0% are the two extremes).

Mark-Simulacrum · 2018-07-10T21:15:11Z

Probably just noise, those seem to be fairly normal measurements for most noop builds.

oli-obk · 2018-07-10T21:27:46Z

We can always bump the threshold to cause the loop checker to get triggered later

ecstatic-morse · 2018-07-10T21:58:21Z

I recorded the benchmark locally with perf, and interpret::step and its children in the call graph were only 0.01% of total cycles. I doubt that inflate is actually triggering the detector; I think it would take up more time. I'm not sure how common spurious data points are, especially since you're counting instructions, not CPU cycles, but the minimum run for inflate is -0.1%, so there must be some variance there.

oli-obk · 2018-07-11T06:03:02Z

@bors r+

We do emit a warning if things start going slow, so at least ppl will know what's going on.

bors · 2018-07-11T06:03:02Z

📌 Commit cf5eaa7 has been approved by oli-obk

bors · 2018-07-11T06:46:18Z

⌛ Testing commit cf5eaa7 with merge d6364f49d822ba1c5076ac649327c048bc34a96e...

bors · 2018-07-11T07:20:09Z

💔 Test failed - status-travis

oli-obk · 2018-07-11T09:04:04Z

@bors retry

bors · 2018-07-11T16:24:53Z

⌛ Testing commit cf5eaa7 with merge d573fe1...

Infinite loop detection for const evaluation Resolves #50637. An `EvalContext` stores the transient state (stack, heap, etc.) of the MIRI virtual machine while it executing code. As long as MIRI only executes pure functions, we can detect if a program is in a state where it will never terminate by periodically taking a "snapshot" of this transient state and comparing it to previous ones. If any two states are exactly equal, the machine must be in an infinite loop. Instead of fully cloning a snapshot every time the detector is run, we store a snapshot's hash. Only when a hash collision occurs do we fully clone the interpreter state. Future snapshots which cause a collision will be compared against this clone, causing the interpreter to abort if they are equal. At the moment, snapshots are not taken until MIRI has progressed a certain amount. After this threshold, snapshots are taken every `DETECTOR_SNAPSHOT_PERIOD` steps. This means that an infinite loop with period `P` will be detected after a maximum of `2 * P * DETECTOR_SNAPSHOT_PERIOD` interpreter steps. The factor of 2 arises because we only clone a snapshot after it causes a hash collision.

bors · 2018-07-11T18:30:55Z

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

@oli-obk

Fix issue #52475: Make loop detector only consider reachable memory As [suggested](#51702 (comment)) by @oli-obk `alloc_id`s should be ignored by traversing all `Allocation`s in interpreter memory at a given moment in time, beginning by `ByRef` locals in the stack. - [x] Generalize the implementation of `Hash` for `EvalSnapshot` to traverse `Allocation`s - [x] Generalize the implementation of `PartialEq` for `EvalSnapshot` to traverse `Allocation`s - [x] Commit regression tests Fixes #52626 Fixes #52849

rust-highfive assigned michaelwoerister Jun 22, 2018

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

rust-highfive assigned oli-obk and unassigned michaelwoerister Jun 22, 2018

oli-obk requested changes Jun 22, 2018

View reviewed changes

bjorn3 requested changes Jun 22, 2018

View reviewed changes

bjorn3 requested changes Jun 25, 2018

View reviewed changes

oli-obk reviewed Jun 25, 2018

View reviewed changes

ecstatic-morse force-pushed the infinite-loop-detection branch from f2b2fad to 54a9e3a Compare June 27, 2018 06:10

bjorn3 requested changes Jun 27, 2018

View reviewed changes

bjorn3 reviewed Jun 27, 2018

View reviewed changes

ecstatic-morse force-pushed the infinite-loop-detection branch from 0bcba33 to 82b704b Compare June 29, 2018 22:32

ecstatic-morse mentioned this pull request Jun 30, 2018

Implement Eq, Clone and Hash for MemoryData and Evaluator rust-lang/miri#387

Merged

ecstatic-morse changed the title ~~[WIP] Infinite loop detection for const evaluation~~ Infinite loop detection for const evaluation Jun 30, 2018

bjorn3 requested changes Jun 30, 2018

View reviewed changes

ecstatic-morse force-pushed the infinite-loop-detection branch from 49910b6 to a183a35 Compare July 4, 2018 20:32

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 Jul 11, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 11, 2018

This comment has been minimized.

Sign in to view

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 Jul 11, 2018

bors merged commit cf5eaa7 into rust-lang:master Jul 11, 2018

bors mentioned this pull request Jul 11, 2018

Allow panicking with string literal messages inside constants #52011

Merged

ecstatic-morse deleted the infinite-loop-detection branch July 18, 2018 00:28

ecstatic-morse mentioned this pull request Jul 18, 2018

CTFE fails to detect a certain class of infinite loops #52475

Closed

brunocodutra mentioned this pull request Jul 22, 2018

Fix issue #52475: Make loop detector only consider reachable memory #52626

Merged

3 tasks

RalfJung mentioned this pull request Jul 30, 2018

A pretty big performance hit after a recent change (PR #51361) #52849

Closed

Infinite loop detection for const evaluation #51702

Infinite loop detection for const evaluation #51702

Conversation

ecstatic-morse commented Jun 22, 2018 • edited Loading

rust-highfive commented Jun 22, 2018

ecstatic-morse commented Jun 22, 2018

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Jun 22, 2018 • 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

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

ecstatic-morse Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Jun 22, 2018 • 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

Choose a reason for hiding this comment

ecstatic-morse Jun 23, 2018 • edited Loading

Choose a reason for hiding this comment

ecstatic-morse Jun 23, 2018 • 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

bjorn3 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

ecstatic-morse Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

ecstatic-morse commented Jun 22, 2018 • edited Loading

oli-obk commented Jun 23, 2018

ecstatic-morse commented Jun 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Jun 25, 2018

ecstatic-morse commented Jun 25, 2018

oli-obk commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rust-highfive commented Jun 29, 2018

ecstatic-morse commented Jun 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jul 3, 2018

RalfJung commented Jul 3, 2018 • edited Loading

oli-obk commented Jul 3, 2018

ecstatic-morse commented Jul 4, 2018

Mark-Simulacrum commented Jul 10, 2018

rust-timer commented Jul 10, 2018

kennytm commented Jul 10, 2018 • edited Loading

Mark-Simulacrum commented Jul 10, 2018

oli-obk commented Jul 10, 2018

ecstatic-morse commented Jul 10, 2018

oli-obk commented Jul 11, 2018

ecstatic-morse commented Jun 22, 2018 •

edited

Loading

ecstatic-morse Jun 22, 2018 •

edited

Loading

ecstatic-morse Jun 22, 2018 •

edited

Loading

ecstatic-morse Jun 22, 2018 •

edited

Loading

ecstatic-morse Jun 23, 2018 •

edited

Loading

ecstatic-morse Jun 23, 2018 •

edited

Loading

ecstatic-morse Jun 22, 2018 •

edited

Loading

ecstatic-morse commented Jun 22, 2018 •

edited

Loading

ecstatic-morse commented Jun 24, 2018 •

edited

Loading

ecstatic-morse Jun 25, 2018 •

edited

Loading

ecstatic-morse Jun 27, 2018 •

edited

Loading

ecstatic-morse commented Jun 30, 2018 •

edited

Loading

RalfJung commented Jul 3, 2018 •

edited

Loading

kennytm commented Jul 10, 2018 •

edited

Loading