From ac0a703d5bc0724f5c70d626eb77a9d7b4695432 Mon Sep 17 00:00:00 2001 From: Alex Guteniev Date: Sun, 30 Nov 2025 16:51:39 +0200 Subject: [PATCH 1/5] Fix call unwrapping optimization --- stl/inc/functional | 2 +- .../GH_005504_avoid_function_call_wrapping/test.cpp | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/stl/inc/functional b/stl/inc/functional index 65ad553b2da..0f97cda4636 100644 --- a/stl/inc/functional +++ b/stl/inc/functional @@ -2051,7 +2051,7 @@ public: using _Vt = decay_t<_Fn>; static_assert(is_constructible_v<_Vt, _Fn>, "_Vt should be constructible from _Fn. " "(N4950 [func.wrap.move.ctor]/6)"); - if constexpr (is_same_v<_Vt, function<_Signature...>>) { + if constexpr (!is_lvalue_reference_v<_Fn> && is_same_v<_Vt, function<_Signature...>>) { this->template _Construct_with_old_fn<_Vt>(_STD forward<_Fn>(_Callable)); } else { if constexpr (is_member_pointer_v<_Vt> || is_pointer_v<_Vt> diff --git a/tests/std/tests/GH_005504_avoid_function_call_wrapping/test.cpp b/tests/std/tests/GH_005504_avoid_function_call_wrapping/test.cpp index 44ec403e220..2a556d5dc35 100644 --- a/tests/std/tests/GH_005504_avoid_function_call_wrapping/test.cpp +++ b/tests/std/tests/GH_005504_avoid_function_call_wrapping/test.cpp @@ -163,4 +163,16 @@ int main() { alloc_checker{0}, test_wrapped_null, function>(true, true); alloc_checker{0}, test_wrapped_null, move_only_function>(true, false); alloc_checker{0}, test_wrapped_null, function>(false, true); + + { + // make sure we only move from function when we can + function f1{small_callable{}}; + assert(move_only_function(f1)(copy_counter{}) == 1); + assert(f1); + assert(f1(copy_counter{}) == 0); + + function f2{small_callable{}}; + assert(move_only_function(move(f2))(copy_counter{}) == 0); + assert(!f2); + } } From f749382024d1921207f7eca5921d26410f84af81 Mon Sep 17 00:00:00 2001 From: Alex Guteniev Date: Sun, 30 Nov 2025 19:45:26 +0200 Subject: [PATCH 2/5] better coverage --- .../test.cpp | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/std/tests/GH_005504_avoid_function_call_wrapping/test.cpp b/tests/std/tests/GH_005504_avoid_function_call_wrapping/test.cpp index 2a556d5dc35..cd34f52f6b8 100644 --- a/tests/std/tests/GH_005504_avoid_function_call_wrapping/test.cpp +++ b/tests/std/tests/GH_005504_avoid_function_call_wrapping/test.cpp @@ -104,6 +104,14 @@ void test_wrapped_call(const int expected_copies) { assert(outer(copy_counter{}) == expected_copies); } +template +void test_wrapped_copy_call(const int expected_copies) { + InnerWrapper inner{Callable{}}; + OuterWrapper outer{inner}; + assert(inner); + assert(outer(copy_counter{}) == expected_copies); +} + template void check_call_null(Wrapper& wrapper, const bool throws) { if (throws) { @@ -143,6 +151,8 @@ int main() { // Moves to the same alloc_checker{0}, test_wrapped_call, function, small_callable>(0); alloc_checker{1}, test_wrapped_call, function, large_callable>(0); + alloc_checker{0}, test_wrapped_copy_call, function, small_callable>(0); + alloc_checker{2}, test_wrapped_copy_call, function, large_callable>(0); alloc_checker{0}, test_wrapped_call, move_only_function, small_callable>(0); alloc_checker{1}, test_wrapped_call, move_only_function, large_callable>(0); @@ -154,6 +164,8 @@ int main() { #endif test_wrapped_call, function, small_callable>(0); alloc_checker{1}, test_wrapped_call, function, large_callable>(0); + alloc_checker{1}, test_wrapped_copy_call, function, small_callable>(1); + alloc_checker{3}, test_wrapped_copy_call, function, large_callable>(1); // nulls alloc_checker{0}, test_plain_null>(true); @@ -163,16 +175,4 @@ int main() { alloc_checker{0}, test_wrapped_null, function>(true, true); alloc_checker{0}, test_wrapped_null, move_only_function>(true, false); alloc_checker{0}, test_wrapped_null, function>(false, true); - - { - // make sure we only move from function when we can - function f1{small_callable{}}; - assert(move_only_function(f1)(copy_counter{}) == 1); - assert(f1); - assert(f1(copy_counter{}) == 0); - - function f2{small_callable{}}; - assert(move_only_function(move(f2))(copy_counter{}) == 0); - assert(!f2); - } } From 2486130eab0182044d7b4a59a9fc2a2be0c8fa13 Mon Sep 17 00:00:00 2001 From: Alex Guteniev Date: Sun, 30 Nov 2025 20:06:02 +0200 Subject: [PATCH 3/5] Proper fix --- stl/inc/functional | 41 +++++++++++++++---- .../test.cpp | 14 +++---- 2 files changed, 41 insertions(+), 14 deletions(-) diff --git a/stl/inc/functional b/stl/inc/functional index 0f97cda4636..072caba7d4d 100644 --- a/stl/inc/functional +++ b/stl/inc/functional @@ -1629,15 +1629,38 @@ public: _STL_INTERNAL_STATIC_ASSERT(alignof(max_align_t) == alignof(void*)); // 64-bit target, can put small function into small move_only_function directly _Data._Impl = _Create_impl_ptr<_Impl_kind::_Old_fn_small, _Vt, void>(); - _Old_fn_impl->_Move(_Data._Buf_ptr()); - _Func._Tidy(); + if constexpr (is_lvalue_reference_v<_Fn>) { + _Old_fn_impl->_Copy(_Data._Buf_ptr()); + } else { + _Old_fn_impl->_Move(_Data._Buf_ptr()); + _Func._Tidy(); + } #else // ^^^ 64-bit / 32-bit vvv _STL_INTERNAL_STATIC_ASSERT(alignof(max_align_t) > alignof(void*)); // 32-bit target, cannot put small function into small move_only_function directly // due to potentially not enough alignment. Allocate large function void* _Where = ::operator new((_Small_object_num_ptrs - 1) * sizeof(void*)); - _Old_fn_impl->_Move(_Where); - _Func._Tidy(); + if constexpr (is_lvalue_reference_v<_Fn>) { + struct _NODISCARD _Guard_type { + void* _Ptr; + + ~_Guard_type() { + // _Ptr is not nullptr only if an exception is thrown as a result of _Vt construction. + // Check _Ptr before calling operator delete to save the call in the common case. + if (_Ptr) { + ::operator delete(_Ptr); + } + } + }; + + _Guard_type _Guard; + _Guard._Ptr = _Where; + _Old_fn_impl->_Copy(_Where); + _Guard._Ptr = nullptr; + } else { + _Old_fn_impl->_Move(_Where); + _Func._Tidy(); + } _Data._Impl = _Create_impl_ptr<_Impl_kind::_Old_fn_small_as_large, _Vt, void>(); _Data._Set_large_fn_ptr(_Where); @@ -1645,8 +1668,12 @@ public: } else { // Just take ownership of the inner impl pointer _Data._Impl = _Create_impl_ptr<_Impl_kind::_Old_fn_large, _Vt, void>(); - _Data._Set_large_fn_ptr(_Old_fn_impl); - _Func._Set(nullptr); + if constexpr (is_lvalue_reference_v<_Fn>) { + _Data._Set_large_fn_ptr(_Old_fn_impl->_Copy(nullptr)); + } else { + _Data._Set_large_fn_ptr(_Old_fn_impl); + _Func._Set(nullptr); + } } } @@ -2051,7 +2078,7 @@ public: using _Vt = decay_t<_Fn>; static_assert(is_constructible_v<_Vt, _Fn>, "_Vt should be constructible from _Fn. " "(N4950 [func.wrap.move.ctor]/6)"); - if constexpr (!is_lvalue_reference_v<_Fn> && is_same_v<_Vt, function<_Signature...>>) { + if constexpr (is_same_v<_Vt, function<_Signature...>>) { this->template _Construct_with_old_fn<_Vt>(_STD forward<_Fn>(_Callable)); } else { if constexpr (is_member_pointer_v<_Vt> || is_pointer_v<_Vt> diff --git a/tests/std/tests/GH_005504_avoid_function_call_wrapping/test.cpp b/tests/std/tests/GH_005504_avoid_function_call_wrapping/test.cpp index cd34f52f6b8..1102e74009b 100644 --- a/tests/std/tests/GH_005504_avoid_function_call_wrapping/test.cpp +++ b/tests/std/tests/GH_005504_avoid_function_call_wrapping/test.cpp @@ -156,16 +156,16 @@ int main() { alloc_checker{0}, test_wrapped_call, move_only_function, small_callable>(0); alloc_checker{1}, test_wrapped_call, move_only_function, large_callable>(0); + constexpr bool is_64_bit = sizeof(void*) > 4; + // Moves from function to move_only_function -#ifdef _WIN64 - alloc_checker{0}, -#else - alloc_checker{1}, -#endif + alloc_checker{is_64_bit ? 0 : 1}, test_wrapped_call, function, small_callable>(0); alloc_checker{1}, test_wrapped_call, function, large_callable>(0); - alloc_checker{1}, test_wrapped_copy_call, function, small_callable>(1); - alloc_checker{3}, test_wrapped_copy_call, function, large_callable>(1); + + alloc_checker{is_64_bit ? 0 : 1}, + test_wrapped_copy_call, function, small_callable>(0); + alloc_checker{2}, test_wrapped_copy_call, function, large_callable>(0); // nulls alloc_checker{0}, test_plain_null>(true); From 2f17e3240c0c3fc870b6dd0685edc2bbf889cf3a Mon Sep 17 00:00:00 2001 From: Alex Guteniev Date: Sun, 30 Nov 2025 20:55:58 +0200 Subject: [PATCH 4/5] fix comment --- stl/inc/functional | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/functional b/stl/inc/functional index 072caba7d4d..1908ea5c53e 100644 --- a/stl/inc/functional +++ b/stl/inc/functional @@ -1645,7 +1645,7 @@ public: void* _Ptr; ~_Guard_type() { - // _Ptr is not nullptr only if an exception is thrown as a result of _Vt construction. + // _Ptr is not nullptr only if an exception is thrown as a result of _Copy. // Check _Ptr before calling operator delete to save the call in the common case. if (_Ptr) { ::operator delete(_Ptr); From 2729c744036264cc8d2bd520c0274fec75ff5ee8 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Mon, 1 Dec 2025 04:13:01 -0800 Subject: [PATCH 5/5] Use aggregate init for the guard. --- stl/inc/functional | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/stl/inc/functional b/stl/inc/functional index 1908ea5c53e..2a71748f412 100644 --- a/stl/inc/functional +++ b/stl/inc/functional @@ -1653,8 +1653,7 @@ public: } }; - _Guard_type _Guard; - _Guard._Ptr = _Where; + _Guard_type _Guard{_Where}; _Old_fn_impl->_Copy(_Where); _Guard._Ptr = nullptr; } else {