std::function copies movable objects when is SOO is used

[llvmbot]

Bugzilla Link	33125
Version	unspecified
OS	All
Attachments	Test case
Reporter	LLVM Bugzilla Contributor
CC	@mclow

Extended Description

Hi. Libc++ that comes with Xcode 8.2.1 has small object optimization implemented for std::function. But move constructor of std::function always uses copy constructor of the object. This results in unnecessary copies being made and additional destructor calls.

In our use case this even leads to crashes because the object is a handle and we rely that it will be destroyed on specific thread. But that extra copies, created while the std::function is dispatched to the worker thread, result in destructor being deleted at another time and from another thread.

It would be good if moving std::function object resulted in moving the underlying functor if it is movable.

I've attached a test program, that demonstrates the problem.

[llvmbot]

I've tested with libc++=3.9.1-2 available in ubuntu zesty and it has the same problem.

[llvmbot]

The SSO is only applied in this case because the copy constructor is noexcept.
You can work around this on your end by making the copy constructor not noexcept, but this will remove the SSO.

I need to do some further investigation to discover if libc++'s current behavior is legal, but I suspect it is.

Even if the behavior is non-conforming, fixing this to behave as requested would take an ABI break.

[llvmbot]

The SSO is only applied in this case because the copy constructor is noexcept.

Yes, that's totally true. But my move constructor is noexcept too. So there is no reason to choose copy constructor over move constructor.

You can work around this on your end by making the copy constructor not noexcept, but this will remove the SSO.

This is not always possible. In the part of the system where we've found the problem we use std::shared_ptr's. We can't change declaration of shared_ptr's copy constructor.
Of course we will add some workaround in our code because even if this behavior will be fixed, it will take quite a while for changes to propagate to our build environment. This is more a question for eliminating pitfalls in which other developers may fall.

I need to do some further investigation to discover if libc++'s current behavior is legal, but I suspect it is.

I think that this is covered by section 20.9.12.2.1
function construct/copy/destroy (the section number may vary on standard version). For move constructor it says:

Effects: If !f, *this has no target; otherwise, move-constructs the target of f into the target of *this, leaving f in a valid state with an unspecified value.

I'm not very good at reading standard and I may be mistaken. But even if current behavior is legal I still do not think that it is good and that it is not a flaw in standard. Though I agree, that such change will complicate std::function's code.

Even if the behavior is non-conforming, fixing this to behave as requested would take an ABI break.

Why would it break ABI? I do not suggest to disable SOO, I suggest to use move constructor if it is available and marked as noexcept. Memory layout of std::function will not change. The only thing that will change is the constructor to be called. And that constructors will present in both old and new code, so no break here too.

[llvmbot]

You can work around this on your end by making the copy constructor not noexcept, but this will remove the SSO.

This is not always possible. In the part of the system where we've found the
problem we use std::shared_ptr's. We can't change declaration of
shared_ptr's copy constructor.

Good point, I hadn't considered that. It seems like you're relying on non-standard
behavior if your depending on the move operation of shared_ptr not to segfault but
not the copy op.

I need to do some further investigation to discover if libc++'s current behavior is legal, but I suspect it is.

I think that this is covered by section [func.wrap.func.con] [...]

[function(function&&)

Effects: If !f, *this has no target; otherwise, move-constructs the target
of f into the target of *this, leaving f in a valid state with an
unspecified value.

I'm not very good at reading standard and I may be mistaken.

You're not necessarily mistaken, but that language has since changed, and the change
was made retroactively as a defect fix. The new requirements state:

Postconditions: If !f, *this has no target; otherwise, the target of *this is equivalent to the target of f before the construction, and f is in a valid state with an unspecified value.

This language makes it much clearer that Libc++'s behavior is legal.

[...] I still do not think that it is good and that it
is not a flaw in standard. Though I agree, that such change will complicate
std::function's code.

I agree, it would be much better to perform a noexcept move operation in this case.

The coset to make that change in Libc++ is very gerat because it means changing a vtable
and breaking ABI compatibility. However if/when we have to eventually break ABI compatibility
in std::function, I'll be sure this issues gets fixed.

Even if the behavior is non-conforming, fixing this to behave as requested would take an ABI break.

Why would it break ABI?

std::function does type-erasure; Which is implemented using virtual functions.
Fixing this bug would require adding a new "__clone_move" function to the vtable
to complement the current "__clone" function (which is getting called and causing your bug).
Adding a new virtual function requires changing the vtable in an ABI incompatible way.

[llvmbot]

Thank you for clarifications. ABI breakage is a real stopper.