Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

pnkfelix
Copy link
Member

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
Copy link
Collaborator

r? @brson

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

@pnkfelix
Copy link
Member Author

r? @nikomatsakis

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

dotdash commented Jan 11, 2016

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
Copy link
Member Author

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

@nikomatsakis
Copy link
Contributor

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
Copy link
Member Author

@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
Copy link
Contributor

brson commented Jan 12, 2016

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

@pnkfelix
Copy link
Member Author

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

  1. These three bugs are not currently biting too many clients (AFAWK...)
  2. beta is the least tested (I think) of the three standard channels (nightly beta stable), and
  3. 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
Copy link
Contributor

r? @dotdash

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

@pnkfelix pnkfelix force-pushed the put-back-alloca-zeroing-for-issue-30530 branch from a59b0a3 to 65629ef Compare January 12, 2016 15:25
// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this also needs to be made conditional.

@dotdash
Copy link
Contributor

dotdash commented Jan 12, 2016

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
Copy link
Contributor

dotdash commented Jan 12, 2016

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

@dotdash
Copy link
Contributor

dotdash commented Jan 13, 2016

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
Copy link
Member Author

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

@pnkfelix pnkfelix force-pushed the put-back-alloca-zeroing-for-issue-30530 branch from 65629ef to decc286 Compare January 13, 2016 12:31
@pnkfelix 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 pnkfelix changed the title Put back alloca zeroing; issues 29092, 30018, 30530, 30822 Put back zeroing; issues 29092, 30018, 30530, 30822 Jan 13, 2016
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
Copy link
Contributor

dotdash commented Jan 13, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Jan 13, 2016

📌 Commit decc286 has been approved by dotdash

Manishearth added a commit to Manishearth/rust that referenced this pull request Jan 14, 2016
…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
bors added a commit that referenced this pull request Jan 14, 2016
@bors bors merged commit decc286 into rust-lang:master Jan 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment