Put back zeroing; issues 29092, 30018, 30530, 30822 #30823

pnkfelix · 2016-01-11T16:36:56Z

Put back alloca zeroing for issues #29092, #30018, #30530; inject zeroing for #30822.

Background context: fn alloca_zeroed was removed in PR #22969, so we haven't been "zero'ing" (*) the alloca's since at least that point, but the logic behind that PR seems sound, so its not entirely obvious how long the underlying bug has actually been present. In other words, I have not yet done a survey to see when the new alloc_ty and lvalue_scratch_datum calls were introduced that should have had "zero'ing" the alloca's.

I first fixed #30018, then decided to do a survey of alloc_ty calls to see if they needed similar treatment, which quickly led to a rediscovery of #30530.

While making the regression test for the latter, I discovered #30822, which is a slightly different bug (in terms of where the "zero'ing" needs to go), but still relevant.

I haven't finished the aforementioned survey of fn alloc_ty calls, but I decided I wanted to get this up for review in its current state (namely to see if my attempt to force developers to include a justification for passing Uninit can possibly fly, or if I should abandon that path of action).

(*): I am putting quotation marks around "zero'ing" because we no longer use zero as our "dropped" marker value.

Fix #29092
Fix #30018
Fix #30530
Fix #30822

rust-highfive · 2016-01-11T16:37:07Z

r? @brson

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

pnkfelix · 2016-01-11T16:39:17Z

r? @nikomatsakis

dotdash · 2016-01-11T17:42:25Z

src/librustc_trans/trans/datum.rs

                    |this, bcx, llval| {
+                        debug!("populate call for Datum::to_lvalue_datum_in_scope \
+                                self.ty={:?}", this.ty);
                        call_lifetime_start(bcx, llval);


The call to llvm.lifetime.start will happen after the dropped initialization, so LLVM will see that as a dead store.

Hmm it seems like in some cases the only option would be to inject the lifetime start into the start of the function itself then, right after the alloca.

Does LLVM react okay if there exist multiple lifetime start

From a quick glance a the dataflow calculation in the stack coloring pass, it looks like that might cause it to think that the alloca is dead when entering the basic block that has the second lifetime start. I didn't verify that though, because even if that does not happen, the lifetime will be extended to the entry block, so we could as well remove the second lifetime start call. That will reduce the potential for stack slot sharing, but I can't even guess how much of an impact that would have.

Okay; I will take care not to introduce multiple lifetime starts on a given path. (This will complicate the PR a bit but so be it)

dotdash · 2016-01-11T17:49:30Z

Just wondering, did you do any testing how well LLVM can remove those inits in case they're not actually required? Since the initialization happens in the entry block, the memory dependencies will often cross basic blocks which in some cases breaks optimizations. Not a huge concern, since this will probably largely go away with MIR trans, but I'm still curious.

Maybe we are able to move the initialization closer to the point where the alloca is needed? I wonder whether we actually already request allocas in such a way that if we just emitted the memset at the current insertion point, we would guarantee that it dominates the drop call.

pnkfelix · 2016-01-11T19:11:53Z

@dotdash no I did not do any performance / codegen quality analysis

nikomatsakis · 2016-01-11T19:12:24Z

Hmm, so this seems overall good to me. But @dotdash already found some issues I hadn't considered (the lifetime point). So I'm inclined to move the review to him, if he's ok with that. :)

pnkfelix · 2016-01-11T19:26:21Z

@dotdash (I will try to do some analysis; I am hoping that my checking for a destructor will be a sufficient filter in most cases)

brson · 2016-01-12T02:56:18Z

This looks like its important to land before the next beta branch.

pnkfelix · 2016-01-12T10:10:44Z

@brson hmm, I am not sure; given that:

These three bugs are not currently biting too many clients (AFAWK...)
beta is the least tested (I think) of the three standard channels (nightly beta stable), and
I think whatever form this fix takes, it is likely to have somewhat high risk of injecting new bugs of its own (be they performance bugs or correctness bugs), ...

those three factors to me add up to: I doubt we should rush this into nightly to try to make the beta cut. (I mean, if it happens to make it in in time, I won't complain; but I don't want to rush it.)

In particular, bring back the `zero` flag for `lvalue_scratch_datum`, which controls whether the alloca's created immediately at function start are uninitialized at that point or have their embedded drop-flags initialized to "dropped". Then made `to_lvalue_datum_in_scope` pass "dropped" as `zero` flag.

nikomatsakis · 2016-01-12T14:36:35Z

r? @dotdash

(let me know if you don't have time)

dotdash · 2016-01-12T22:12:16Z

src/librustc_trans/trans/tvec.rs

    // Always create an alloca even if zero-sized, to preserve
    // the non-null invariant of the inner slice ptr
-    let llfixed = base::alloc_ty(bcx, fixed_ty, "");
+    let llfixed = base::alloc_ty_init(bcx, fixed_ty, init_alloca, "");
    call_lifetime_start(bcx, llfixed);


I think this also needs to be made conditional.

dotdash · 2016-01-12T22:38:12Z

r=me with the lifetime call made conditional.

Maybe #29092 could be added to the list of issues this fixes, because even though I can't reproduce that issue anymore, I believe it to have (had) the same root cause as #30530 and that it's just hidden by slightly different optimizations.

dotdash · 2016-01-12T23:04:39Z

And the Travis failure actually seems to be caused by the wrong lifetime call, the memset gets eliminated.

dotdash · 2016-01-13T09:53:26Z

Actually, no, it's a different bug, and one that I originally introduced. This fixes it:

diff --git a/src/librustc_trans/trans/tvec.rs b/src/librustc_trans/trans/tvec.rs
index e5ef3b0..ac638d6 100644
--- a/src/librustc_trans/trans/tvec.rs
+++ b/src/librustc_trans/trans/tvec.rs
@@ -219,7 +219,6 @@ fn write_content<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
                         bcx = expr::trans_into(bcx, &**element,
                                                SaveIn(lleltptr));
                         let scope = cleanup::CustomScope(temp_scope);
-                        fcx.schedule_lifetime_end(scope, lleltptr);
                         // Issue #30822: mark memory as dropped after running destructor
                         fcx.schedule_drop_and_fill_mem(scope, lleltptr, vt.unit_ty, None);
                     }

The thing is that the element's memory is not actually dead here, since the slice owns the memory.

pnkfelix · 2016-01-13T11:25:52Z

@dotdash ah great, I'll fold that into this PR.

includes bugfixes pointed out during review: * Only `call_lifetime_start` for an alloca if the function entry does not itself initialize it to "dropped." * Remove `schedule_lifetime_end` after writing an *element* into a borrowed slice. (As explained by [dotdash][irc], "the lifetime end that is being removed was for an element in the slice, which is not an alloca of its own and has no lifetime start of its own") [irc]: https://botbot.me/mozilla/rust-internals/2016-01-13/?msg=57844504&page=3

…st-lang#30530, rust-lang#30822. Note that the test for rust-lang#30822 is folded into the test for rust-lang#30530 (but the file name mentions only 30530).

(The reason this is not factored as far as possible because a subsequent commit is going to need to do construction without having access to a `cx`.)

(This can/should be revisited when drop flags are stored out of band.)

(Note that it might be a good idea to replace *all* calls of `alloc_ty` with calls to `alloc_ty_init`, to encourage programmers to consider the appropriate value for the `init` flag when creating temporary values.)

dotdash · 2016-01-13T13:58:43Z

@bors r+

bors · 2016-01-13T13:58:44Z

📌 Commit decc286 has been approved by dotdash

…r-issue-30530, r=dotdash Put back alloca zeroing for issues rust-lang#29092, rust-lang#30018, rust-lang#30530; inject zeroing for rust-lang#30822. ---- Background context: `fn alloca_zeroed` was removed in PR rust-lang#22969, so we haven't been "zero'ing" (\*) the alloca's since at least that point, but the logic behind that PR seems sound, so its not entirely obvious how *long* the underlying bug has actually been present. In other words, I have not yet done a survey to see when the new `alloc_ty` and `lvalue_scratch_datum` calls were introduced that should have had "zero'ing" the alloca's. ---- I first fixed rust-lang#30018, then decided to do a survey of `alloc_ty` calls to see if they needed similar treatment, which quickly led to a rediscovery of rust-lang#30530. While making the regression test for the latter, I discovered rust-lang#30822, which is a slightly different bug (in terms of where the "zero'ing" needs to go), but still relevant. I haven't finished the aforementioned survey of `fn alloc_ty` calls, but I decided I wanted to get this up for review in its current state (namely to see if my attempt to force developers to include a justification for passing `Uninit` can possibly fly, or if I should abandon that path of action). ---- (*): I am putting quotation marks around "zero'ing" because we no longer use zero as our "dropped" marker value. Fix rust-lang#29092 Fix rust-lang#30018 Fix rust-lang#30530 Fix rust-lang#30822

- Successful merges: #30626, #30662, #30770, #30801, #30818, #30823, #30828, #30835, #30837, #30839, #30845, #30848, #30850, #30851, #30863 - Failed merges:

rust-highfive assigned brson Jan 11, 2016

rust-highfive assigned nikomatsakis and unassigned brson Jan 11, 2016

pnkfelix mentioned this pull request Jan 11, 2016

Double-drops when control jumps out of borrow of slice literal construction #30822

Closed

pnkfelix force-pushed the put-back-alloca-zeroing-for-issue-30530 branch from ac3e65e to a59b0a3 Compare January 11, 2016 16:48

dotdash reviewed Jan 11, 2016
View reviewed changes

pnkfelix added 2 commits January 12, 2016 15:24

debug instrumentation (can remove)

cec7280

rust-highfive assigned dotdash and unassigned nikomatsakis Jan 12, 2016

pnkfelix force-pushed the put-back-alloca-zeroing-for-issue-30530 branch from a59b0a3 to 65629ef Compare January 12, 2016 15:25

dotdash reviewed Jan 12, 2016
View reviewed changes

dotdash mentioned this pull request Jan 12, 2016

Segfault in a compiler-generated Drop impl #29092

Closed

pnkfelix force-pushed the put-back-alloca-zeroing-for-issue-30530 branch from 65629ef to decc286 Compare January 13, 2016 12:31

pnkfelix changed the title ~~Put back alloca zeroing; issues 30018, 30530, 30822~~ Put back alloca zeroing; issues 29092, 30018, 30530, 30822 Jan 13, 2016

pnkfelix changed the title ~~Put back alloca zeroing; issues 29092, 30018, 30530, 30822~~ Put back zeroing; issues 29092, 30018, 30530, 30822 Jan 13, 2016

pnkfelix added 4 commits January 13, 2016 14:29

Unit/regression tests for issues rust-lang#29092, rust-lang#30018, ru…

df1283c

…st-lang#30530, rust-lang#30822. Note that the test for rust-lang#30822 is folded into the test for rust-lang#30530 (but the file name mentions only 30530).

Factored out private routine for emitting LLVM lifetime intrinsic calls.

8168765

(The reason this is not factored as far as possible because a subsequent commit is going to need to do construction without having access to a `cx`.)

revise lifetime handling for alloca's that are initialized as "dropped."

7706e23

(This can/should be revisited when drop flags are stored out of band.)

add doc for new fn alloc_ty_init.

decc286

(Note that it might be a good idea to replace *all* calls of `alloc_ty` with calls to `alloc_ty_init`, to encourage programmers to consider the appropriate value for the `init` flag when creating temporary values.)

Manishearth mentioned this pull request Jan 13, 2016

Rollup of 15 pull requests #30883

Merged

bors added a commit that referenced this pull request Jan 14, 2016

Auto merge of #30883 - Manishearth:rollup, r=Manishearth

5b3a75f

- Successful merges: #30626, #30662, #30770, #30801, #30818, #30823, #30828, #30835, #30837, #30839, #30845, #30848, #30850, #30851, #30863 - Failed merges:

bors merged commit decc286 into rust-lang:master Jan 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put back zeroing; issues 29092, 30018, 30530, 30822 #30823

Put back zeroing; issues 29092, 30018, 30530, 30822 #30823

pnkfelix commented Jan 11, 2016

rust-highfive commented Jan 11, 2016

pnkfelix commented Jan 11, 2016

dotdash Jan 11, 2016

pnkfelix Jan 11, 2016

dotdash Jan 11, 2016

pnkfelix Jan 12, 2016

dotdash commented Jan 11, 2016

pnkfelix commented Jan 11, 2016

nikomatsakis commented Jan 11, 2016

pnkfelix commented Jan 11, 2016

brson commented Jan 12, 2016

pnkfelix commented Jan 12, 2016

nikomatsakis commented Jan 12, 2016

dotdash Jan 12, 2016

dotdash commented Jan 12, 2016

dotdash commented Jan 12, 2016

dotdash commented Jan 13, 2016

pnkfelix commented Jan 13, 2016

dotdash commented Jan 13, 2016

bors commented Jan 13, 2016

Put back zeroing; issues 29092, 30018, 30530, 30822 #30823

Put back zeroing; issues 29092, 30018, 30530, 30822 #30823

Conversation

pnkfelix commented Jan 11, 2016

rust-highfive commented Jan 11, 2016

pnkfelix commented Jan 11, 2016

dotdash Jan 11, 2016

Choose a reason for hiding this comment

pnkfelix Jan 11, 2016

Choose a reason for hiding this comment

dotdash Jan 11, 2016

Choose a reason for hiding this comment

pnkfelix Jan 12, 2016

Choose a reason for hiding this comment

dotdash commented Jan 11, 2016

pnkfelix commented Jan 11, 2016

nikomatsakis commented Jan 11, 2016

pnkfelix commented Jan 11, 2016

brson commented Jan 12, 2016

pnkfelix commented Jan 12, 2016

nikomatsakis commented Jan 12, 2016

dotdash Jan 12, 2016

Choose a reason for hiding this comment

dotdash commented Jan 12, 2016

dotdash commented Jan 12, 2016

dotdash commented Jan 13, 2016

pnkfelix commented Jan 13, 2016

dotdash commented Jan 13, 2016

bors commented Jan 13, 2016