EricWF added a comment.

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).


https://reviews.llvm.org/D34331



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to