[PATCH] D29908: Disallow returning a __block variable via a move

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D29908#676601, @ahatanak wrote:

> In https://reviews.llvm.org/D29908#676589, @ahatanak wrote:
>
> > Posting John's review comment, which was sent to the review I abandoned:
> >
> > "Hmm.  The behavior I think we actually want here is that we should move out of the __block variable but not perform NRVO, essentially like we would if the variable were a parameter.
>
>
> I think that's what's happening. Because of the changes made in r274291, NRVO isn't performed, but the move constructor instead of the copy constructor is called to return the shared_ptr. The shared_ptr in the heap is moved to returned shared_ptr (%agg.result), so when the block function passed to dispatch_async is executed, it segfaults because the object "ret" used to point to has been destructed.
>
> If that's the case, should we conclude that clang is behaving as expected?
>
> > The block byref copy helper should always be doing a move if it can, regardless of whether the function is returned.  That's independent of this, though."


Oh, I see.  Just to clarify: it doesn't matter that the object "ret" used to point to has been destructed, what matters is that "ret" now holds a null reference because it's been moved out of.

I retract my comment; I agree there's a bug here.  We should not implicitly move out of __block variables during a return because we cannot assume that the variable will no longer be used.  Please update the comment to correctly identify this as the reasoning; it has nothing to do with whether __block variables technically *can* support NRVO.


https://reviews.llvm.org/D29908