MIR EndRegion Statements (was MIR dataflow for Borrows) #39409

pnkfelix · 2017-01-30T14:17:33Z

This PR adds an EndRegion statement to MIR (where the EndRegion statement is what terminates a borrow).

An earlier version of the PR implemented a dataflow analysis on borrow expressions, but I am now factoring that into a follow-up PR so that reviewing this one is easier. (And also because there are some revisions I want to make to that dataflow code, but I want this PR to get out of WIP status...)

This is a baby step towards MIR borrowck. I just want to get the review process going while I independently work on the remaining steps.

rust-highfive · 2017-01-30T14:17:38Z

r? @eddyb

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

pnkfelix · 2017-01-30T14:17:44Z

r? @arielb1

eddyb · 2017-01-30T14:51:11Z

So what happened to the plans for keeping this information in a side table? cc @nikomatsakis

nagisa · 2017-01-30T15:53:50Z

src/librustc_mir/build/scope.rs

        let rv = unpack!(block = f(self));
-        unpack!(block = self.pop_scope(extent, block));
+        unpack!(block = self.pop_scope(extent.0, block));
+        if self.seen_borrows.contains(&extent.0) {


Doesn’t early exit from the scope (i.e. exit_scope down below) need a similar treatment?

You're right, I had overlooked that. Will fix.

(I had been hoping to avoid attempting to write unit tests for this, but maybe I need to do so.)

nagisa · 2017-01-30T15:54:05Z

So what happened to the plans for keeping this information in a side table?

It makes sense to me to keep these inline, even if just because we already keep StorageLiveDead inline.

bors · 2017-01-31T02:32:30Z

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

nikomatsakis · 2017-02-01T21:21:36Z

@eddyb

So what happened to the plans for keeping this information in a side table?

Well, I think I turned sour on this idea because it's hard to keep such a thing 'in sync' if the MIR is changed in any way. It's very fragile. Also, I think these instructions are temporary (once we adopt the NLL strategy, we'll be able to drop them) and there should be relatively few (you only need them for regions that appear in a loan, though I'm not sure if @pnkfelix handles that since I've not read the PR yet). But it is I suppose an option.

bors · 2017-02-04T01:38:06Z

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

pnkfelix · 2017-02-06T17:24:46Z

@arielb1 has convinced me that I need to add some code-gen tests, even if their initial state is based solely on eyeballing the output to confirm that it is sane.

arielb1 · 2017-02-06T21:44:27Z

src/librustc_borrowck/borrowck/mir/dataflow/impls.rs

+// (In principle BorrowIdx need not be pub, but since it is associated
+// type of BitDenotation impl, hand is forced by privacy rules.)
+
+#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]


Use the new_index macro?

ah I forgot about that; will fix.

pnkfelix · 2017-02-08T08:25:05Z

Bisection reports that commit 039f5f95ea6646741f9fdd7d6cb9335366d5c82b injected a bug... investigating

update: oh, I see it. This distinguished moves from overwrites. It was also meant to be a refactoring, but it started skipping some important code for the overwrite case.

pnkfelix · 2017-02-10T11:29:05Z

@arielb1 has suggested that I had a static check that no regions reach mir return nor resume aka a "no leak" check. This is a good idea; it remains to be implemented.

pnkfelix · 2017-02-13T12:58:37Z

(Testing reveals that I missed EndRegion's on the unwind path ... fixing in tandem with adding the above suggested static check.)

nikomatsakis · 2017-02-17T01:17:04Z

src/librustc/util/ppaux.rs

@@ -540,6 +545,12 @@ impl fmt::Display for ty::Region {
            ty::ReSkolemized(_, br) => {
                write!(f, "{}", br)
            }
+            ty::ReScope(code_extent) if identify_regions() => {
+                write!(f, "'{}ce", code_extent.index())


does -Zverbose not do it for you? Too verbose?

Yes, -Zverbose is far too verbose, especially when it is embedded within a &<region> <lvalue> borrow-expression.

I don't want or care about the internal structure of the region value for these cases; it is much more convenient to look at the unique id's being generated here (by taking advantage of the fact that one cannot write a lifetime of the form '<number><alphanum>

bors · 2017-02-18T07:21:55Z

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

bors · 2017-03-15T16:47:24Z

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

pnkfelix · 2017-06-13T14:44:20Z

Narrowed problem down via opt -opt-bisect-limit. Seems like the execution problem is injected by the Tail Call Elimination pass. I still need to review the LLVM IR to try to understand whether this is an LLVM codegen bug (as I suspect) or if something I have done has caused the Tail Call Elimination pass to "legitimately" mis-optimize this code.

nikomatsakis · 2017-06-13T16:27:00Z

@pnkfelix nice hunting

arielb1 · 2017-06-15T22:27:18Z

Looks like the bug is fixed by llvm-mirror/llvm@f45bea4. I'll have the fix in my Grand ARM Fix Rollup, which I should really get done Real Soon.

pnkfelix · 2017-06-16T10:16:27Z

@arielb1 helped me confirm that the LLVM bug occurs on our fork of LLVM, but not on LLVM's master copy. And I have now confirmed that the nearest common ancestor between the two also exhibits the bug.

I am bisecting against the LLVM git history to try to isolate a patch for the bug that we might be able to apply to our fork.

Update: Oops, I didn't reload the page so I missed @arielb1's comment. Okay I guess my bisection is not necessary then.

So ARM had quite a few codegen bugs on LLVM 4.0 which are fixed on LLVM trunk. This backports 5 of them: r297871 - ARM: avoid clobbering register in v6 jump-table expansion. - fixes rust-lang#42248 r294949 - [Thumb-1] TBB generation: spot redefinitions of index r295816 - [ARM] Fix constant islands pass. r300870 - [Thumb-1] Fix corner cases for compressed jump tables r302650 - [IfConversion] Add missing check in IfConversion/canFallThroughTo - unblocks rust-lang#39409

@alexcrichton

Backport fixes to LLVM 4.0 ARM codegen bugs So ARM had quite a few codegen bugs on LLVM 4.0 which are fixed on LLVM trunk. This backports 5 of them: r297871 - ARM: avoid clobbering register in v6 jump-table expansion. - fixes #42248 r294949 - [Thumb-1] TBB generation: spot redefinitions of index r295816 - [ARM] Fix constant islands pass. r300870 - [Thumb-1] Fix corner cases for compressed jump tables r302650 - [IfConversion] Add missing check in IfConversion/canFallThroughTo - unblocks #39409 r? @alexcrichton beta-nominating because this fixes regressions introduced by LLVM 4.0.

arielb1 · 2017-06-19T13:01:22Z

Given that the LLVM bug is fixed:
@bors r=nikomatsakis

bors · 2017-06-19T13:01:23Z

📌 Commit 11f4968 has been approved by nikomatsakis

bors · 2017-06-19T13:01:34Z

⌛ Testing commit 11f4968 with merge 0414594...

MIR EndRegion Statements (was MIR dataflow for Borrows) This PR adds an `EndRegion` statement to MIR (where the `EndRegion` statement is what terminates a borrow). An earlier version of the PR implemented a dataflow analysis on borrow expressions, but I am now factoring that into a follow-up PR so that reviewing this one is easier. (And also because there are some revisions I want to make to that dataflow code, but I want this PR to get out of WIP status...) This is a baby step towards MIR borrowck. I just want to get the review process going while I independently work on the remaining steps.

bors · 2017-06-19T15:23:30Z

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

So ARM had quite a few codegen bugs on LLVM 4.0 which are fixed on LLVM trunk. This backports 5 of them: r297871 - ARM: avoid clobbering register in v6 jump-table expansion. - fixes rust-lang#42248 r294949 - [Thumb-1] TBB generation: spot redefinitions of index r295816 - [ARM] Fix constant islands pass. r300870 - [Thumb-1] Fix corner cases for compressed jump tables r302650 - [IfConversion] Add missing check in IfConversion/canFallThroughTo - unblocks rust-lang#39409

Fix destruction extent lookup during HIR -> HAIR translation My method for finding the destruction extent, if any, from cbed41a (in rust-lang#39409), was buggy in that it sometimes failed to find an extent that was nonetheless present. This fixes that, and is cleaner code to boot. Fix rust-lang#43457

Fix destruction extent lookup during HIR -> HAIR translation My method for finding the destruction extent, if any, from cbed41a (in #39409), was buggy in that it sometimes failed to find an extent that was nonetheless present. This fixes that, and is cleaner code to boot. Fix #43457

rust-highfive assigned eddyb Jan 30, 2017

rust-highfive assigned arielb1 and unassigned eddyb Jan 30, 2017

nagisa reviewed Jan 30, 2017

View reviewed changes

oli-obk mentioned this pull request Jan 31, 2017

use StorageLive and StorageDead to allocate/deallocate stack memory rust-lang/miri#49

Closed

pnkfelix force-pushed the mir-borrowck2 branch from 73080e6 to 46771dc Compare February 2, 2017 14:57

pnkfelix force-pushed the mir-borrowck2 branch from 3418d21 to 8603d92 Compare February 6, 2017 09:15

arielb1 reviewed Feb 6, 2017

View reviewed changes

pnkfelix force-pushed the mir-borrowck2 branch 2 times, most recently from c8e3994 to 4928c37 Compare February 10, 2017 09:07

pnkfelix force-pushed the mir-borrowck2 branch from 4928c37 to 6193740 Compare February 10, 2017 14:49

nikomatsakis reviewed Feb 17, 2017

View reviewed changes

pnkfelix force-pushed the mir-borrowck2 branch from 6193740 to 1293feb Compare February 20, 2017 16:53

pnkfelix changed the title ~~MIR Dataflow for Borrows~~ MIR Dataflow for Borrows (totally WIP) Feb 20, 2017

pnkfelix force-pushed the mir-borrowck2 branch from 1293feb to cdced4f Compare March 14, 2017 13:59

pnkfelix force-pushed the mir-borrowck2 branch 2 times, most recently from ad6c87e to 42ad606 Compare March 23, 2017 17:06

pnkfelix force-pushed the mir-borrowck2 branch from 517a1c9 to 11f4968 Compare June 12, 2017 12:54

arielb1 mentioned this pull request Jun 18, 2017

backport fixes to LLVM 4.0 ARM codegen bugs rust-lang/llvm#83

Merged

arielb1 mentioned this pull request Jun 18, 2017

Backport fixes to LLVM 4.0 ARM codegen bugs #42740

Merged

bors merged commit 11f4968 into rust-lang:master Jun 19, 2017

dwrensha mentioned this pull request Jun 20, 2017

handle EndRegion as no-op rust-lang/miri#199

Merged

TimNN mentioned this pull request Jun 25, 2017

Moved value gets dropped if reassigned #42903

Closed

bluejekyll mentioned this pull request Jun 26, 2017

NSEC validation failing in nightly hickory-dns/hickory-dns#152

Closed

This was referenced Jul 27, 2017

25% regression in compile time of issue-20936-deep-vector benchmark #43511

Closed

Binary size blowup presumably because of allocator integration #42808

Closed

alexcrichton mentioned this pull request Aug 13, 2017

15% regression in compiler memory usage for inflate crate #43834

Closed

TimNN mentioned this pull request Aug 17, 2017

Nightly regression on x86_64_unknown_linux_gnu #43126

Closed

pnkfelix mentioned this pull request Aug 25, 2017

Fix destruction extent lookup during HIR -> HAIR translation #44082

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MIR EndRegion Statements (was MIR dataflow for Borrows) #39409

MIR EndRegion Statements (was MIR dataflow for Borrows) #39409

pnkfelix commented Jan 30, 2017 •

edited

Loading

rust-highfive commented Jan 30, 2017

pnkfelix commented Jan 30, 2017

eddyb commented Jan 30, 2017

nagisa Jan 30, 2017

pnkfelix Jan 31, 2017

nagisa commented Jan 30, 2017

bors commented Jan 31, 2017

nikomatsakis commented Feb 1, 2017 •

edited

Loading

bors commented Feb 4, 2017

pnkfelix commented Feb 6, 2017

arielb1 Feb 6, 2017

pnkfelix Feb 7, 2017

pnkfelix commented Feb 8, 2017 •

edited

Loading

pnkfelix commented Feb 10, 2017

pnkfelix commented Feb 13, 2017

nikomatsakis Feb 17, 2017

pnkfelix Feb 20, 2017

bors commented Feb 18, 2017

bors commented Mar 15, 2017

pnkfelix commented Jun 13, 2017

nikomatsakis commented Jun 13, 2017

arielb1 commented Jun 15, 2017

pnkfelix commented Jun 16, 2017 •

edited

Loading

arielb1 commented Jun 19, 2017

bors commented Jun 19, 2017

bors commented Jun 19, 2017

bors commented Jun 19, 2017

MIR EndRegion Statements (was MIR dataflow for Borrows) #39409

MIR EndRegion Statements (was MIR dataflow for Borrows) #39409

Conversation

pnkfelix commented Jan 30, 2017 • edited Loading

rust-highfive commented Jan 30, 2017

pnkfelix commented Jan 30, 2017

eddyb commented Jan 30, 2017

nagisa Jan 30, 2017

Choose a reason for hiding this comment

pnkfelix Jan 31, 2017

Choose a reason for hiding this comment

nagisa commented Jan 30, 2017

bors commented Jan 31, 2017

nikomatsakis commented Feb 1, 2017 • edited Loading

bors commented Feb 4, 2017

pnkfelix commented Feb 6, 2017

arielb1 Feb 6, 2017

Choose a reason for hiding this comment

pnkfelix Feb 7, 2017

Choose a reason for hiding this comment

pnkfelix commented Feb 8, 2017 • edited Loading

pnkfelix commented Feb 10, 2017

pnkfelix commented Feb 13, 2017

nikomatsakis Feb 17, 2017

Choose a reason for hiding this comment

pnkfelix Feb 20, 2017

Choose a reason for hiding this comment

bors commented Feb 18, 2017

bors commented Mar 15, 2017

pnkfelix commented Jun 13, 2017

nikomatsakis commented Jun 13, 2017

arielb1 commented Jun 15, 2017

pnkfelix commented Jun 16, 2017 • edited Loading

arielb1 commented Jun 19, 2017

bors commented Jun 19, 2017

bors commented Jun 19, 2017

bors commented Jun 19, 2017

pnkfelix commented Jan 30, 2017 •

edited

Loading

nikomatsakis commented Feb 1, 2017 •

edited

Loading

pnkfelix commented Feb 8, 2017 •

edited

Loading

pnkfelix commented Jun 16, 2017 •

edited

Loading