[MIR] Add Storage{Live,Dead} statements to emit llvm.lifetime.{start,end}. #35409

eddyb · 2016-08-06T02:16:18Z

Storage live ranges are tracked for all MIR variables and temporaries with a drop scope.
StorageLive is lowered to llvm.lifetime.start and StorageDead to llvm.lifetime.end.

There are some improvements possible here, such as:

pack multiple storage liveness statements by using the index of first local + u64 bitset
enforce that locals are not directly accessed outside their storage live range
shrink storage live ranges for never-borrowed locals to initialization -> last use
emit storage liveness statements for all temporaries
- however, the remaining ones are always SSA immediates, so they'd be noop in MIR trans
- could have a flag on the temporary that its storage is irrelevant (a la C's old register)
  - would also deny borrows if necessary
  - this seems like an overcompliation and with packing & optimizations it may be pointless

Even in the current state, it helps stage2 rustc compile boiler without overflowing (see #35408).

A later addition fixes #26764 and closes #27372 by emitting .section directives for dylib metadata to avoid them being allocated into memory or read as .note. For this PR, those bugs were tripping valgrind.

rust-highfive · 2016-08-06T02:16:29Z

r? @nikomatsakis

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

nagisa · 2016-08-06T12:54:51Z

src/librustc_mir/build/expr/as_temp.rs

+        if temp_lifetime.is_some() {
+            this.cfg.push(block, Statement {
+                source_info: source_info,
+                kind: StatementKind::StorageLive(temp.clone())


Make a helper function in cfg, since you’re constructing this multiple times manually.

This should become this.cfg.push_storage_live(source_info, temp);

arielb1 · 2016-08-06T15:43:36Z

Test please

nikomatsakis · 2016-08-08T13:18:56Z

src/librustc/mir/repr.rs

    Assign(Lvalue<'tcx>, Rvalue<'tcx>),
+
+    /// Start a live range for the storage of the local.
+    StorageLive(Lvalue<'tcx>),


Hmm, should this be restricted to base lvalues or something like that? Meh, I guess it's ok to use Lvalue<'tcx> here.

Local would be great, but even then, it would not be applicable to ReturnPointer or args.

nikomatsakis · 2016-08-08T13:33:22Z

Test please

Seems like a good use for @scottcarr's MIR testing framework.

scottcarr · 2016-08-08T21:41:40Z

src/librustc_trans/mir/statement.rs

+                    }
+                }
+                bcx
+            }


Is there some way to refactor the arms? The only difference is End.call or Start.call, right?

EDIT: (I edited my original note which said "why not call lvalue_trans?" -- which we do not need)

We need to do nothing if the local is not an Lvalue.
Using a method taking lvalue and the base::Lifetime would work, yeah.

nikomatsakis · 2016-08-09T14:05:41Z

OK so @eddyb convinced me that most of my concerns were moot. r=me modulo the following:

needs a test
need a few more comments
make things DRY, as @scottcarr suggested
maybe remove storage from "constant MIR"?
super minor nits (1, 2) to address if you agree

eddyb · 2016-08-10T23:10:22Z

@bors r=nikomatsakis p=1 (if this gets into the nightly, it may fix a couple crates)

bors · 2016-08-10T23:10:25Z

📌 Commit 7def9f5 has been approved by nikomatsakis

bors · 2016-08-10T23:24:49Z

⌛ Testing commit 7def9f5 with merge a5973b6...

bors · 2016-08-11T01:03:07Z

💔 Test failed - auto-linux-64-opt

nikomatsakis · 2016-08-11T19:01:21Z

Failures are valgrind failures:

---- [run-pass-valgrind] run-pass-valgrind/coerce-match-calls.rs stdout ----

error: test run failed!
status: exit code: 100
command: /usr/bin/valgrind --error-exitcode=100 --fair-sched=try --quiet --soname-synonyms=somalloc=NONE --suppressions=/buildslave/rust-buildbot/slave/auto-linux-64-opt/build/src/etc/x86.supp --tool=memcheck --leak-check=full x86_64-unknown-linux-gnu/test/run-pass-valgrind/coerce-match-calls.stage2-x86_64-unknown-linux-gnu
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
==20600== Syscall param read(buf) points to unaddressable byte(s)
==20600==    at 0x401B177: read (syscall-template.S:84)
==20600==    by 0x40059FF: open_verify.constprop.7 (dl-load.c:1949)
==20600==    by 0x4005CD9: open_path (dl-load.c:2066)
==20600==    by 0x4008BE0: _dl_map_object (dl-load.c:2307)
==20600==    by 0x400D9D1: openaux (dl-deps.c:63)
==20600==    by 0x4010393: _dl_catch_error (dl-error.c:187)
==20600==    by 0x400E011: _dl_map_object_deps (dl-deps.c:254)
==20600==    by 0x40034F9: dl_main (rtld.c:1610)
==20600==    by 0x4019461: _dl_sysdep_start (dl-sysdep.c:249)
==20600==    by 0x4004E79: _dl_start_final (rtld.c:307)
==20600==    by 0x4004E79: _dl_start (rtld.c:413)
==20600==    by 0x4000CC7: ??? (in /lib/x86_64-linux-gnu/ld-2.23.so)
==20600==  Address 0xffee10a00 is on thread 1's stack
==20600==  in frame #1, created by open_verify.constprop.7 (dl-load.c:1695)
==20600== 

------------------------------------------

Could be real!

eddyb · 2016-08-11T19:04:11Z

@nikomatsakis I pinged @alexcrichton but I don't think he had time to look over them yet.
AFAICT, those are in rtld, which could only be a problem at the end of execution, if we've ruined any rtld state - but the stack trace makes me believe it's actually at the start of execution.

eddyb · 2016-08-11T22:29:22Z

@bors retry (hoping this is spurious)

eddyb · 2016-08-11T22:29:42Z

@bors p=0

bors · 2016-08-13T13:12:05Z

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

…end}.

eddyb · 2016-08-14T03:34:59Z

@bors r=nikomatsakis

bors · 2016-08-14T03:35:00Z

📌 Commit 02aec40 has been approved by nikomatsakis

bors · 2016-08-14T03:36:54Z

⌛ Testing commit 02aec40 with merge 4d03edd...

bors · 2016-08-14T05:05:06Z

💔 Test failed - auto-linux-64-opt

eddyb · 2016-08-14T06:31:25Z

I ~~can't reproduce~~ don't have valgrind locally at all 😕.

eddyb · 2016-08-14T07:04:29Z

Argh #26764 strikes again, this time libstd's .note.rustc section being too large for just one rtld function to alloca at once, according to valgrind.

eddyb · 2016-08-14T08:19:02Z

@bors r=nikomatsakis

bors · 2016-08-14T08:19:03Z

📌 Commit 1bb1444 has been approved by nikomatsakis

eddyb · 2016-08-14T16:40:44Z

@bors p=2

bors · 2016-08-14T19:28:48Z

⌛ Testing commit 1bb1444 with merge 1d5b758...

[MIR] Add Storage{Live,Dead} statements to emit llvm.lifetime.{start,end}. Storage live ranges are tracked for all MIR variables and temporaries with a drop scope. `StorageLive` is lowered to `llvm.lifetime.start` and `StorageDead` to `llvm.lifetime.end`. There are some improvements possible here, such as: * pack multiple storage liveness statements by using the index of first local + `u64` bitset * enforce that locals are not directly accessed outside their storage live range * shrink storage live ranges for never-borrowed locals to initialization -> last use * emit storage liveness statements for *all* temporaries * however, the remaining ones are *always* SSA immediates, so they'd be noop in MIR trans * could have a flag on the temporary that its storage is irrelevant (a la C's old `register`) * would also deny borrows if necessary * this seems like an overcompliation and with packing & optimizations it may be pointless Even in the current state, it helps stage2 `rustc` compile `boiler` without overflowing (see #35408). A later addition fixes #26764 and closes #27372 by emitting `.section` directives for dylib metadata to avoid them being allocated into memory or read as `.note`. For this PR, those bugs were tripping valgrind.

bors · 2016-08-14T22:27:13Z

cuviper · 2016-08-14T22:35:53Z

I don't know about the rest, but thanks for the incidental .note cleanup! 😄

rust-highfive assigned nikomatsakis Aug 6, 2016

eddyb force-pushed the mir-storage-stmts branch from f86c06c to 19955cf Compare August 6, 2016 02:22

nagisa reviewed Aug 6, 2016
View reviewed changes

nikomatsakis reviewed Aug 8, 2016
View reviewed changes

scottcarr reviewed Aug 8, 2016
View reviewed changes

alexcrichton mentioned this pull request Aug 9, 2016

Stack overflow compiling serde_codegen 0.8 on OSX nightly serde-rs/serde#486

Closed

eddyb force-pushed the mir-storage-stmts branch from 19955cf to 7def9f5 Compare August 10, 2016 23:09

Manishearth mentioned this pull request Aug 13, 2016

Rollup of 9 pull requests #35639

Closed

[MIR] Add Storage{Live,Dead} statements to emit llvm.lifetime.{start,…

02aec40

…end}.

eddyb force-pushed the mir-storage-stmts branch from 7def9f5 to 02aec40 Compare August 14, 2016 03:34

Get rid of the .note interpretation of rustc dylib metadata.

1bb1444

bors merged commit 1bb1444 into rust-lang:master Aug 14, 2016

eddyb deleted the mir-storage-stmts branch August 14, 2016 22:28

bors mentioned this pull request Aug 14, 2016

[MIR] track Location in MirVisitor, combine Location #35542

Merged

cuviper mentioned this pull request Aug 15, 2016

Move metadata out of dylibs #23366

Open

Mark-Simulacrum mentioned this pull request Aug 20, 2016

LLVM lifetime intrinsics and MIR #35171

Closed

eddyb mentioned this pull request Aug 24, 2016

investigate compile-time regressions from 2016-08-17 #35805

Closed

brson mentioned this pull request Aug 27, 2016

rustup-init 0.6.1 segfaults on x86_64 and armv7, 0.6.0 on OS X 10.10 #36023

Closed

eddyb mentioned this pull request Apr 29, 2017

Don't let .debug_gdb_scripts become loadable into memory. #41627

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIR] Add Storage{Live,Dead} statements to emit llvm.lifetime.{start,end}. #35409

[MIR] Add Storage{Live,Dead} statements to emit llvm.lifetime.{start,end}. #35409

eddyb commented Aug 6, 2016 •

edited

Loading

rust-highfive commented Aug 6, 2016

nagisa Aug 6, 2016

arielb1 commented Aug 6, 2016

nikomatsakis Aug 8, 2016

eddyb Aug 8, 2016

nikomatsakis commented Aug 8, 2016

scottcarr Aug 8, 2016 •

edited

Loading

eddyb Aug 8, 2016

nikomatsakis commented Aug 9, 2016 •

edited by eddyb

Loading

eddyb commented Aug 10, 2016

bors commented Aug 10, 2016

bors commented Aug 10, 2016

bors commented Aug 11, 2016

nikomatsakis commented Aug 11, 2016

eddyb commented Aug 11, 2016

eddyb commented Aug 11, 2016

eddyb commented Aug 11, 2016

bors commented Aug 13, 2016

eddyb commented Aug 14, 2016

bors commented Aug 14, 2016

bors commented Aug 14, 2016

bors commented Aug 14, 2016

eddyb commented Aug 14, 2016 •

edited

Loading

eddyb commented Aug 14, 2016

eddyb commented Aug 14, 2016

bors commented Aug 14, 2016

eddyb commented Aug 14, 2016

bors commented Aug 14, 2016

bors commented Aug 14, 2016

cuviper commented Aug 14, 2016

[MIR] Add Storage{Live,Dead} statements to emit llvm.lifetime.{start,end}. #35409

[MIR] Add Storage{Live,Dead} statements to emit llvm.lifetime.{start,end}. #35409

Conversation

eddyb commented Aug 6, 2016 • edited Loading

rust-highfive commented Aug 6, 2016

nagisa Aug 6, 2016

Choose a reason for hiding this comment

arielb1 commented Aug 6, 2016

nikomatsakis Aug 8, 2016

Choose a reason for hiding this comment

eddyb Aug 8, 2016

Choose a reason for hiding this comment

nikomatsakis commented Aug 8, 2016

scottcarr Aug 8, 2016 • edited Loading

Choose a reason for hiding this comment

eddyb Aug 8, 2016

Choose a reason for hiding this comment

nikomatsakis commented Aug 9, 2016 • edited by eddyb Loading

eddyb commented Aug 10, 2016

bors commented Aug 10, 2016

bors commented Aug 10, 2016

bors commented Aug 11, 2016

nikomatsakis commented Aug 11, 2016

eddyb commented Aug 11, 2016

eddyb commented Aug 11, 2016

eddyb commented Aug 11, 2016

bors commented Aug 13, 2016

eddyb commented Aug 14, 2016

bors commented Aug 14, 2016

bors commented Aug 14, 2016

bors commented Aug 14, 2016

eddyb commented Aug 14, 2016 • edited Loading

eddyb commented Aug 14, 2016

eddyb commented Aug 14, 2016

bors commented Aug 14, 2016

eddyb commented Aug 14, 2016

bors commented Aug 14, 2016

bors commented Aug 14, 2016

cuviper commented Aug 14, 2016

eddyb commented Aug 6, 2016 •

edited

Loading

scottcarr Aug 8, 2016 •

edited

Loading

nikomatsakis commented Aug 9, 2016 •

edited by eddyb

Loading

eddyb commented Aug 14, 2016 •

edited

Loading