[NLL] Add false edges out of infinite loops #47802

sapphire-arches · 2018-01-27T10:20:23Z

Resolves #46036 by adding a cleanup member to the FalseEdges terminator kind. There's also a small doc fix to one of the other comments in into.rs which I can pull out in to another PR if desired =)

This PR should pass CI but the test suite has been relatively unstable on my system so I'm not 100% sure.

r? @nikomatsakis

rust-highfive · 2018-01-27T10:20:36Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

nikomatsakis · 2018-01-29T17:26:11Z

Nice.

nikomatsakis

One thing this is missing is a ui test showing that we generate errors. I'll have to check the original issue, but I think we concocted such a test, right?

UPDATE: This test

nikomatsakis · 2018-01-29T19:22:34Z

src/librustc/mir/mod.rs

@@ -803,9 +803,16 @@ pub enum TerminatorKind<'tcx> {
    /// Indicates the end of the dropping of a generator
    GeneratorDrop,

+    /// A block where control flow only ever takes one real path, but borrowck
+    /// needs to be more conservative.


yay for comments 💯

nikomatsakis · 2018-01-29T19:22:53Z

src/librustc/mir/mod.rs

+        /// The list of blocks control flow could conceptually take, but won't
+        /// in practice
+        imaginary_targets: Vec<BasicBlock>,
+        /// The block we go to if unwinding


this is also false, right? we should clarify that

nikomatsakis · 2018-01-29T19:28:29Z

src/librustc/mir/visit.rs

                        self.visit_branch(block, real_target);
                        for target in imaginary_targets {
                            self.visit_branch(block, *target);
                        }
+                        cleanup.map(|t| self.visit_branch(block, t));


Nit: personally, I'd find if let more readable.

nikomatsakis · 2018-01-29T19:31:30Z

src/librustc_mir/build/expr/into.rs

+                // [block] --> [loop_block] ~~> [loop_block_end] --false link--> [cleanup_block]
+                //                  ^                   |
+                //                  |                   |
+                //                  +-------------------+


hmm, this picture seems a bit wrong (I didn't look at the code yet). In particular, if you have something like this:

loop { continue; }

then I think that loop_block_end is dead-code, and hence the FalseEdges would be stripped out. I think I had expected rather something like:

[loop_block] --> [body_block] ~~> [loop_block_end] | ^ | false +------------------------------ | v cleanup_block

What do you think? (Maybe make a test for this?)

When we don't have a condition (i.e. opt_cond_expr = None) loop_block and body_block are one and the same, which wasn't super clear on the original diagram. This code doesn't deal with continue or break at all, that's handled by some common code in scope.rs. You're correct that the loop end block is dead code, but does dead code elimination occur before borrowcheck? If so I can convert the continue handling code to also emit a false unwind edge; that should handle the loop { continue; } case. Otherwise I don't think we have a problem.

This code doesn't deal with continue or break at all

Well, it sets up the structures that tell that code where to break to. This is what the call to in_breakable_scope is doing:

rust/src/librustc_mir/build/expr/into.rs

Lines 172 to 173 in def3269

this.in_breakable_scope(

Some(loop_block), exit_block, destination.clone(),

In particular, the Some(loop_block) says "on continue, branch here".

You're correct that the loop end block is dead code, but does dead code elimination occur before borrowcheck?

It's not particularly relevant whether dead code is removed; the goal of inserting these false edges is to ensure that every path from the START block reached either an unwind point or the return point (when false edges are included).

If so I can convert the continue handling code to also emit a false unwind edge; that should handle the loop { continue; } case. Otherwise I don't think we have a problem.

You could, but I don't understand why that would be better. It seems like it's a bit more complicated to implement and also results in more blocks/false edges overall. Am I missing something?

nikomatsakis · 2018-01-29T19:34:34Z

src/librustc_mir/transform/inline.rs

@@ -808,11 +808,19 @@ impl<'a, 'tcx> MutVisitor<'tcx> for Integrator<'a, 'tcx> {
            }
            TerminatorKind::Abort => { }
            TerminatorKind::Unreachable => { }
-            TerminatorKind::FalseEdges { ref mut real_target, ref mut imaginary_targets } => {
+            TerminatorKind::FalseEdges { ref mut real_target, ref mut imaginary_targets,
+                                         ref mut cleanup } => {


This is pre-existing, but I wonder if we should just assert that FalseEdges is not encountered when doing inlining. I would think we would strip it out and replace it with a plain Goto before we begin optimizing. But probably best to leave that for a separate PR.

nikomatsakis · 2018-01-29T19:35:00Z

src/librustc_mir/transform/inline.rs

+                } else if !self.in_cleanup_block {
+                    // Unless this assert is in a cleanup block, add an unwind edge to
+                    // the orignal call's cleanup block
+                    *cleanup = self.cleanup_block;


That said, this doesn't seem right. I feel like if cleanup is None, it means that this FalseEdges block is not simulating a false unwind, and hence we should not rewrite this edge to anything at all.

I agree with your assessment here. Now I'm wondering if it's worthwhile creating a completely separate terminator kind for false unwinding versus false edges since I can see this sort of confusion happening in the future as well. On one hand, it could be useful to express both false edges and false unwinds in one terminator, but on the other we don't use that capability anywhere today and it's already causing problems =)

On one hand, it could be useful to express both false edges and false unwinds in one terminator, but on the other we don't use that capability anywhere today and it's already causing problems

Heh. I don't have a strong opinion here, but a Terminator::FalseUnwind variant might feel simpler overall -- I noticed that you rarely seemed to take advantage of the Option<>, but instead wound up writing two distinct arms.

pietroalbini · 2018-02-05T10:50:06Z

@nikomatsakis ping! The PR author addressed your comments, could you review this?

nikomatsakis · 2018-02-05T19:40:22Z

src/test/mir-opt/issue-38669.rs

@@ -25,27 +25,29 @@ fn main() {
 //     bb0: {
 //         StorageLive(_1);
 //         _1 = const false;
-//         goto -> bb1;
+//         goto -> bb2;


This might also be a good spot for a mir-opt test targeting exactly this change. It'd be nice to have fn foo() { loop { } } and then dump the "build" MIR with a comment stating that we are checking that we emit FalseEdges as needed.

nikomatsakis · 2018-02-05T19:40:44Z

src/librustc_mir/build/expr/into.rs

                //
+                // The false link is required in case something results in
+                // unwinding through the body.


this picture looks right, but I'm not sure that the code below is doing that...

nikomatsakis · 2018-02-05T19:41:32Z

src/librustc_mir/build/expr/into.rs

@@ -187,8 +192,15 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                            // we have to do it; this overwrites any `break`-assigned value but it's
                            // always `()` anyway
                            this.cfg.push_assign_unit(exit_block, source_info, destination);
+
+                            out_terminator = TerminatorKind::Goto { target: loop_block };


it would be nice if we indicated what out_terminator means in terms of the diagram above; I find this a bit hard to decipher (kind of a pre-existing problem :)

nikomatsakis · 2018-02-05T19:48:00Z

src/librustc_mir/build/expr/into.rs

@@ -197,7 +209,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                        // Execute the body, branching back to the test.
                        let body_block_end = unpack!(this.into(&tmp, body_block, body));
                        this.cfg.terminate(body_block_end, source_info,


So, in the new diagram above, body_block_end does not appear -- but I presume it is the end of the loop body? In that case, I still think that this is the wrong place for this terminator. I think we want the false unwind to be the terminator for the loop_block. Something like this:

if let Some(cond_expr) = opt_cond_expr { ... // as before } else { body_block = this.cfg.start_new_block(); let diverge_cleanup = this.diverge_cleanup(); this.cfg.terminate( loop_block, TerminatorKind::FalseUnwind { real_target: body_block, unwind: Some(diverge_cleanup) } ); }

then there is no need for the out_terminator variable.

In particular I think that will be important for this test case, which would be good to add:

struct Foo { x: &'static u32 } fn foo() { let a = 3; let foo = Foo { x: &a }; //~ ERROR E0597 loop { continue; } // <-- note the continue here =) } fn main() { }

The comment previously implied that the true branch would result in the false block. Fortunately the implementation is correct.

Sometimes a simple goto misses the cleanup/unwind edges. Specifically, in the case of infinite loops such as those introduced by a loop statement without any other out edges. Analogous to TerminatorKind::FalseEdges; this new terminator kind is used when we want borrowck to consider an unwind path, but real control flow should never actually take it.

Fixes rust-lang#46036

Fix instructions on existing mir-opt tests after introducing false edges from loops. Also, add a test for issue 46036: infinite loops.

As opposed to using weirdness involving pretending the body block is the loop block. This does not pass tests This commit is [ci skip] because I know it doesn't pass tests yet. Somehow this commit introduces nondeterminism into the handling of loops.

nikomatsakis

so what problem are you having exactly?

Fixes the hash test to recognize that MirValidated can change when changing around labels, and add a new test that makes sure we're lowering loop statements correctly.

sapphire-arches · 2018-02-08T01:04:41Z

@nikomatsakis Tests have been fixed and I added a new test for the lowering of loop statements. It seems a little sketchy; but it should do the job.

nikomatsakis · 2018-02-08T17:02:39Z

@bors r+

bors · 2018-02-08T17:02:39Z

📌 Commit 85dfa9d has been approved by nikomatsakis

bors · 2018-02-09T13:04:25Z

⌛ Testing commit 85dfa9d with merge 3bcda48...

@nikomatsakis

[NLL] Add false edges out of infinite loops Resolves #46036 by adding a `cleanup` member to the `FalseEdges` terminator kind. There's also a small doc fix to one of the other comments in `into.rs` which I can pull out in to another PR if desired =) This PR should pass CI but the test suite has been relatively unstable on my system so I'm not 100% sure. r? @nikomatsakis

bors · 2018-02-09T16:42:23Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 3bcda48 to master...

eddyb · 2019-07-01T15:44:11Z

There is missing handling for FalseUnwind in const-checking, which technically broke something: #62272.

@matthewjasper

rustc_mir: follow FalseUnwind's real_target edge in qualify_consts. As far as I can tell, this was accidentally omitted from rust-lang#47802. Fixes rust-lang#62272. r? @matthewjasper or @nikomatsakis

rust-highfive assigned nikomatsakis Jan 27, 2018

sapphire-arches force-pushed the loop_false_edge branch from 54e8833 to 5e06e84 Compare January 27, 2018 10:23

nikomatsakis requested changes Jan 29, 2018

View reviewed changes

nikomatsakis added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 29, 2018

sapphire-arches force-pushed the loop_false_edge branch from 5e06e84 to 01f1a0d Compare February 1, 2018 09:03

sapphire-arches added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 1, 2018

nikomatsakis requested changes Feb 5, 2018

View reviewed changes

sapphire-arches added 4 commits February 5, 2018 15:00

Fix comment in ExprKind::LogicalOp

5ca88ae

The comment previously implied that the true branch would result in the false block. Fortunately the implementation is correct.

mir: Add false edge cleanup out of infinite loops

ed6a2eb

Fixes rust-lang#46036

mir: Add and fix tests for FalseUnwinds

eae1a35

Fix instructions on existing mir-opt tests after introducing false edges from loops. Also, add a test for issue 46036: infinite loops.

nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 6, 2018

sapphire-arches force-pushed the loop_false_edge branch from 01f1a0d to 8e0c3f5 Compare February 7, 2018 19:28

nikomatsakis approved these changes Feb 7, 2018

View reviewed changes

Fix tests for MIR loop lowering

85dfa9d

Fixes the hash test to recognize that MirValidated can change when changing around labels, and add a new test that makes sure we're lowering loop statements correctly.

nikomatsakis approved these changes Feb 8, 2018

View reviewed changes

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 Feb 8, 2018

bors merged commit 85dfa9d into rust-lang:master Feb 9, 2018

bors mentioned this pull request Feb 9, 2018

Replace all const evaluation with miri #46882

Merged

eddyb mentioned this pull request Jul 1, 2019

Unconditionally breaking loops not allowed in constants. #62272

Closed

eddyb mentioned this pull request Jul 1, 2019

rustc_mir: follow FalseUnwind's real_target edge in qualify_consts. #62274

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NLL] Add false edges out of infinite loops #47802

[NLL] Add false edges out of infinite loops #47802

sapphire-arches commented Jan 27, 2018

rust-highfive commented Jan 27, 2018

nikomatsakis commented Jan 29, 2018

nikomatsakis left a comment •

edited

Loading

nikomatsakis Jan 29, 2018

nikomatsakis Jan 29, 2018

nikomatsakis Jan 29, 2018

nikomatsakis Jan 29, 2018

sapphire-arches Jan 30, 2018

nikomatsakis Jan 30, 2018

nikomatsakis Jan 29, 2018

nikomatsakis Jan 29, 2018

sapphire-arches Jan 30, 2018

nikomatsakis Jan 30, 2018

pietroalbini commented Feb 5, 2018

nikomatsakis Feb 5, 2018

nikomatsakis Feb 5, 2018

nikomatsakis Feb 5, 2018

nikomatsakis Feb 5, 2018

nikomatsakis left a comment

sapphire-arches commented Feb 8, 2018

nikomatsakis commented Feb 8, 2018

bors commented Feb 8, 2018

bors commented Feb 9, 2018

bors commented Feb 9, 2018

eddyb commented Jul 1, 2019

	this.in_breakable_scope(
	Some(loop_block), exit_block, destination.clone(),

[NLL] Add false edges out of infinite loops #47802

[NLL] Add false edges out of infinite loops #47802

Conversation

sapphire-arches commented Jan 27, 2018

rust-highfive commented Jan 27, 2018

nikomatsakis commented Jan 29, 2018

nikomatsakis left a comment • 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

pietroalbini commented Feb 5, 2018

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

nikomatsakis left a comment

Choose a reason for hiding this comment

sapphire-arches commented Feb 8, 2018

nikomatsakis commented Feb 8, 2018

bors commented Feb 8, 2018

bors commented Feb 9, 2018

bors commented Feb 9, 2018

eddyb commented Jul 1, 2019

nikomatsakis left a comment •

edited

Loading