[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D34331#803452, @EricWF wrote:

> In https://reviews.llvm.org/D34331#803105, @dexonsmith wrote:
>
> > In https://reviews.llvm.org/D34331#802747, @EricWF wrote:
> >
> > > @dexonsmith Mind if I hijack this and check in your changes to `<functional>` with my tests?
> >
> >
> > Not at all.
> >
> > In https://reviews.llvm.org/D34331#802821, @EricWF wrote:
> >
> > > @dexonsmith I'm not sure it's sane to allow reentrant behavior. Could you explain why you think it is?
> >
> >
> > It didn't seem sane to me at first either, despite this being supported by `std::unique_ptr` and `std::shared_ptr`.  I found our user fairly convincing, though:
> >
> > - They had an underlying reference counted object, so only the destruction of the last copy of A nulled out the pointer (mimicked that with the `bool cancel`).
> > - They had a callback function intended to be called once, and dropping that function on cancellation (mimicked that with a global variable). The cancel operation nulled out a `std::function`, but destroying the objects captured in the lambda in that std::function also separately decided to perform a cancel, which should have been OK. The cancel function just contained `callback = nullptr`.
>
>
> According to [reentrancy] it is implementation defined what STL functions are allowed to be recursively reentered. I'm not opposed to providing reentrancy where it's useful, but we would have to provide it well.
>  This is where I was running into problems while I was writing tests. There are so many different ways you can reenter std::function's special members that it makes it impossible for an implementation
>  to truly support reentrancy as the standard would require.
>
> If you consider that every possible copy construction or destruction of a user type is a potential reentrancy point, the complexity of having well-defined reentrant behavior starts to become clear.
>  Any time a copy constructor or destructor is invoked you have a potential reentrancy point which, in order to provide well defined behavior, must also act as a sort of _sequence point_ where the side effects of the current call are visible to the reentrant callers (For example your patches use of `operator=(nullptr_t)`).
>
> The methods fixed in this patch are seemingly improvements; It's clear that `operator=(nullptr)` can safely be made reentrant. On the other hand consider `operator=(function const& rhs)`, which is specified as equivalent to `function(rhs).swap(*this)`.
>  I posit `swap` is not the kind of thing that can easily be made reentrant, but that making `std::function` assignment reentrant in general clearly requires it.
>
> If fixing this bug is sufficiently important I'm willing to accept that, but I don't think it improves the status quo; We may have fixed a specific users bug, but we haven't made these functions safely reentrant (in fact most of the special members are likely still full of reentrancy bugs).


IMO, `function::operator=(nullptr_t)` is a clear, restricted subset of reentrancy: it's the cutely-named `std::function` equivalent of `unique_ptr::reset()`.  I think it's worth supporting reentrancy here.

After my own experimentation creating a testcase (and iterating with our internal users on motivation), I agree that supporting reentrancy for anything else would be untenable.


https://reviews.llvm.org/D34331