Be defensive against a reentrant std::function::operator=(), in case the held function object has a non-trivial destructor. Destroying the function object in-place can lead to the destructor being called twice.
rdar://problem/32836603
Differential D34331
func.wrap.func.con: Unset function before destroying anything vsapsai on Jun 18 2017, 11:52 AM. Authored by
Details Be defensive against a reentrant std::function::operator=(), in case the held function object has a non-trivial destructor. Destroying the function object in-place can lead to the destructor being called twice. rdar://problem/32836603
Diff Detail
Event TimelineComment Actions @dexonsmith Mind if I hijack this and check in your changes to <functional> with my tests?
Comment Actions @dexonsmith I'm not sure it's sane to allow reentrant behavior. Could you explain why you think it is? Should the copy assignment operator allow reentrancy as well? However there is another bug here. operator=(function&&) doesn't correctly call the destructor of the functor. I'll fix that as a separate commit. Comment Actions
Woops, I misread the diff. There is no existing bug W.R.T. missing destructor calls. Comment Actions Not at all. 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:
I'm not sure; what do you think?
Comment Actions 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. 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. 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). 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). Comment Actions 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. Comment Actions Don't you need // UNSUPPORTED: c++98, c++03 since std::function is supported in C++11 only? Comment Actions The tests in libcxx/test/std/utilities/function.objects/ don't have UNSUPPORTED lines, and I don't see a lit.local.cfg that sets config.unsupported. What would be different here? Comment Actions The tests actually do compile in C++03, but they still fail because of infinite recursion: frame #196562: 0x000000010000155c nullptr_t_assign_reentrant.pass.cpp.exe`A::~A() + 92 frame #196563: 0x0000000100001405 nullptr_t_assign_reentrant.pass.cpp.exe`std::__1::__compressed_pair_elem<A, 0, true>::~__compressed_pair_elem() + 21 frame #196564: 0x0000000100002685 nullptr_t_assign_reentrant.pass.cpp.exe`std::__1::__compressed_pair<A, std::__1::allocator<A> >::~__compressed_pair() + 21 frame #196565: 0x0000000100002665 nullptr_t_assign_reentrant.pass.cpp.exe`std::__1::__compressed_pair<A, std::__1::allocator<A> >::~__compressed_pair() + 21 frame #196566: 0x0000000100002389 nullptr_t_assign_reentrant.pass.cpp.exe`std::__1::__function::__func<A, std::__1::allocator<A>, void ()>::destroy() + 25 frame #196567: 0x00000001000014b9 nullptr_t_assign_reentrant.pass.cpp.exe`std::__1::function<void ()>::operator=(std::__1::nullptr_t) + 57 frame #196568: 0x000000010000155c nullptr_t_assign_reentrant.pass.cpp.exe`A::~A() + 92 Looks like prior to C++11 some different destruction behaviour is triggered which isn't fixed by this patch. So the tests should be either guarded by UNSUPPORTED/XFAIL or the patch should support C++03. Comment Actions If you look at the AST diff between C++11 and C++03 this error still happens because of how nullptr is processed: C++11: | |-CXXDestructorDecl 0x7fdab27a2498 <line:24:3, line:28:3> line:24:3 used ~A 'void (void) noexcept' | | `-CompoundStmt 0x7fdab27bcfb8 <col:8, line:28:3> | | |-GCCAsmStmt 0x7fdab27a2670 <line:25:5, col:11> | | `-IfStmt 0x7fdab27bcf88 <line:26:5, line:27:16> | | |-<<<NULL>>> | | |-ImplicitCastExpr 0x7fdab27a26e0 <line:26:9> '_Bool' <LValueToRValue> | | | `-DeclRefExpr 0x7fdab27a26b8 <col:9> '_Bool' lvalue Var 0x7fdab27a23b8 'cancel' '_Bool' | | |-CXXOperatorCallExpr 0x7fdab27bcf40 <line:27:7, col:16> 'class std::__1::function<void (void)>' lvalue | | | |-ImplicitCastExpr 0x7fdab27bcf28 <col:14> 'class std::__1::function<void (void)> &(*)(nullptr_t) noexcept' <FunctionToPointerDecay> | | | | `-DeclRefExpr 0x7fdab27bcea0 <col:14> 'class std::__1::function<void (void)> &(nullptr_t) noexcept' lvalue CXXMethod 0x7fdab27ab010 'operator=' 'class std::__1::function<void (void)> &(nullptr_t) noexcept' | | | |-DeclRefExpr 0x7fdab27a26f8 <col:7> 'std::function<void (void)>':'class std::__1::function<void (void)>' lvalue Var 0x7fdab27a2348 'global' 'std::function<void (void)>':'class std::__1::function<void (void)>' | | | `-CXXNullPtrLiteralExpr 0x7fdab27a2720 <col:16> 'nullptr_t' | | `-<<<NULL>>> vs C++03: | |-CXXDestructorDecl 0x7fc72b6d80d8 <line:24:3, line:28:3> line:24:3 used ~A 'void (void)' | | `-CompoundStmt 0x7fc72b6dfd88 <col:8, line:28:3> | | |-GCCAsmStmt 0x7fc72b6d82a0 <line:25:5, col:11> | | `-IfStmt 0x7fc72b6dfd58 <line:26:5, /Users/buildslave/jenkins/libcxx-build/libcxx.src/include/__nullptr:49:40> | | |-<<<NULL>>> | | |-ImplicitCastExpr 0x7fc72b6d8310 </Users/buildslave/jenkins/libcxx-build/libcxx.src/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp:26:9> '_Bool' <LValueToRValue> | | | `-DeclRefExpr 0x7fc72b6d82e8 <col:9> '_Bool' lvalue Var 0x7fc72b6d8028 'cancel' '_Bool' | | |-CXXOperatorCallExpr 0x7fc72b6dfd10 <line:27:7, /Users/buildslave/jenkins/libcxx-build/libcxx.src/include/__nullptr:49:40> 'class std::__1::function<void (void)>' lvalue | | | |-ImplicitCastExpr 0x7fc72b6dfcf8 </Users/buildslave/jenkins/libcxx-build/libcxx.src/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp:27:14> 'class std::__1::function<void (void)> &(*)(struct std::__1::nullptr_t)' <FunctionToPointerDecay> | | | | `-DeclRefExpr 0x7fc72b6dfca0 <col:14> 'class std::__1::function<void (void)> &(struct std::__1::nullptr_t)' lvalue CXXMethod 0x7fc72b6daea0 'operator=' 'class std::__1::function<void (void)> &(struct std::__1::nullptr_t)' | | | |-DeclRefExpr 0x7fc72b6d8328 <col:7> 'std::function<void (void)>':'class std::__1::function<void (void)>' lvalue Var 0x7fc72b6d7fb8 'global' 'std::function<void (void)>':'class std::__1::function<void (void)>' | | | `-CXXConstructExpr 0x7fc72b6dfc68 </Users/buildslave/jenkins/libcxx-build/libcxx.src/include/__config:441:15, /Users/buildslave/jenkins/libcxx-build/libcxx.src/include/__nullptr:49:40> 'struct std::__1::nullptr_t' 'void (const struct std::__1::nullptr_t &) throw()' elidable | | | `-MaterializeTemporaryExpr 0x7fc72b6dfc50 </Users/buildslave/jenkins/libcxx-build/libcxx.src/include/__config:441:15, /Users/buildslave/jenkins/libcxx-build/libcxx.src/include/__nullptr:49:40> 'const struct std::__1::nullptr_t' lvalue | | | `-ImplicitCastExpr 0x7fc72b6dfc38 </Users/buildslave/jenkins/libcxx-build/libcxx.src/include/__config:441:15, /Users/buildslave/jenkins/libcxx-build/libcxx.src/include/__nullptr:49:40> 'const struct std::__1::nullptr_t' <NoOp> | | | `-CallExpr 0x7fc72b6d83d0 </Users/buildslave/jenkins/libcxx-build/libcxx.src/include/__config:441:15, /Users/buildslave/jenkins/libcxx-build/libcxx.src/include/__nullptr:49:40> 'struct std::__1::nullptr_t' | | | `-ImplicitCastExpr 0x7fc72b6d83b8 </Users/buildslave/jenkins/libcxx-build/libcxx.src/include/__config:441:15, /Users/buildslave/jenkins/libcxx-build/libcxx.src/include/__nullptr:49:24> 'struct std::__1::nullptr_t (*)(void)' <FunctionToPointerDecay> | | | `-DeclRefExpr 0x7fc72b6d8380 </Users/buildslave/jenkins/libcxx-build/libcxx.src/include/__config:441:15, /Users/buildslave/jenkins/libcxx-build/libcxx.src/include/__nullptr:49:24> 'struct std::__1::nullptr_t (void)' lvalue Function 0x7fc72b08d340 '__get_nullptr_t' 'struct std::__1::nullptr_t (void)' | | `-<<<NULL>>> Comment Actions Ah, I got it. There's a __functional_03 header that seems to implement function for C++03 because of manual variadic template expansions. Comment Actions
Didn't move the tests as the standard doesn't require assignment operator to be Comment Actions A few small comments...
Comment Actions Why did tests for this this go into std? [reentrancy]/1 says this isn't required to work. Moreover, assignments in the dtor like this *can't* work in the general case because they would try to overwrite the SSO space. e.g. what do you expect this to do? std::function<void()> global; struct B { int data = 1729; void operator() {} }; struct A { int data = 42; ~A() { global = std::function<void()>(B{}); // whoops, constructs a B on top of A if Small Functor Optimization engages assert(data == 42); } void operator() {} }; int main() { global = std::function<void ()>(A{}); global = nullptr; } Comment Actions It is agreed that it is untenable to support reentrancy for std::function assignment operators and I'm not trying to achieve that. Here we restrict reentrancy only to std::function::operator=(nullptr_t). @mclow.lists where should the tests go if the standard specifies the functionality as implementation-defined?
[reentrancy] p17.6.5.8 |