From 7fb9374e2139fa39d60ce8bbe19de429016a16cb Mon Sep 17 00:00:00 2001 From: Ian Petersen Date: Tue, 27 Oct 2026 18:36:40 -0700 Subject: [PATCH] Make frame allocators provide only storage An operation state too big for `function`'s inline buffer is allocated from the frame allocator by the `__any` machinery that `function` shares with `exec::any_sender`, which constructs and destroys it with `allocator_traits<...>::construct` and `destroy`. `function` adapts the frame allocator with `__frame_allocator_t`, which for an allocator is the allocator itself, rebound to `std::byte`. So when a `function` declares `get_frame_allocator`, the caller's allocator's own `construct` and `destroy` members, if it has any, are called on `__any`'s wrapper around the operation state. For a memory resource, `__frame_allocator_t` is `polymorphic_allocator` or an equivalent for the resource's concrete type, whose `construct` does uses-allocator construction. A frame allocator should only provide storage for a frame; what's constructed in it, and how, is up to whoever allocated the frame. This diff makes `__frame_allocator_t` declare no `construct` or `destroy` in every case, so `allocator_traits` constructs with `construct_at` and destroys with `destroy_at`: - for an allocator, it's a new `__allocator_adaptor` around the rebound allocator, which allocates and deallocates through it and is `noexcept` where it is; and - for a pointer to `memory_resource` or to a type derived from it, it's the allocator that already handled derived resources, without its `construct` and `destroy`, so a plain `memory_resource *` no longer gives a `polymorphic_allocator`. `any_sender` doesn't use `__frame_allocator_t`, so it's unchanged. The `__frame_allocator_t` tests now check that it never does uses-allocator construction and never calls a delegate allocator's `construct` or `destroy`, and a new `function` test checks that a large operation state is allocated from a declared frame allocator but neither constructed nor destroyed through it. Co-Authored-By: Claude Opus 5.5 --- include/exec/__frame_allocator.hpp | 244 ++++++++++++++++------------- include/exec/function.hpp | 24 +-- test/exec/test_frame_allocator.cpp | 117 ++++++++++---- test/exec/test_function.cpp | 79 ++++++++++ 4 files changed, 304 insertions(+), 160 deletions(-) diff --git a/include/exec/__frame_allocator.hpp b/include/exec/__frame_allocator.hpp index 4ac0327a5..ec7cd8c12 100644 --- a/include/exec/__frame_allocator.hpp +++ b/include/exec/__frame_allocator.hpp @@ -27,138 +27,162 @@ //! Defines template exec::__frame_allocator_t //! //! The intended use for __frame_allocator_t is the dynamic allocation of -//! operation states and/or coroutine frames. __frame_allocator_t models the -//! Allocator named requirement in terms of its type parameter, _Delegate. +//! operation states and/or coroutine frames. __frame_allocator_t<_Delegate> is +//! an allocator of std::byte that allocates and deallocates through _Delegate, +//! which may be one of: +//! - another allocator, which allocates and deallocates as itself rebound to +//! the requested type; +//! - std::pmr::memory_resource*; or +//! - T* for some T that inherits from std::pmr::memory_resource, in which case +//! __frame_allocator_t knows the concrete type of its memory_resource and so +//! may be able to avoid virtual dispatch. //! -//! _Delegate may be one of: -//! - Another allocator, in which case __frame_allocator_t<_Delegate> is just -//! _Delegate rebound to std::byte; -//! - std::pmr::memory_resource*, in which case __frame_allocator_t<_Delegate> -//! is just std::pmr::polymorphic_allocator; or -//! - T* for some T that inherits from std::pmr::memor_resource, in which case -//! __frame_allocator_t is an allocator that behaves like -//! std::pmr::polymorphic_allocator, but knows the concrete type of -//! its memory_resource and so therefore may be able to avoid virtual -//! dispatch. -//! -//! Given that __frame_allocator_t<_Delegate> is just an alias to _Delegate when -//! _Delegate is an allocator type, it's up to that type whether its construct -//! member function does "uses-allocator construction". When _Delegate is a -//! pointer to a memory_resource, __frame_allocator_t<_Delegate>::construct does -//! "uses-allocator construction". +//! A frame allocator only provides storage: __frame_allocator_t declares no +//! construct or destroy members, so allocator_traits constructs objects with +//! construct_at and destroys them with destroy_at. In particular, it never does +//! uses-allocator construction, and a delegate allocator's own construct and +//! destroy members, if it has any, are never called. namespace experimental::execution { namespace __fa { using namespace STDEXEC; + //! An allocator that allocates and deallocates through _Alloc but declares + //! no construct or destroy members + template + class __allocator_adaptor + { + using __traits_t = std::allocator_traits<_Alloc>; + + template + friend class __allocator_adaptor; + + STDEXEC_ATTRIBUTE(no_unique_address) _Alloc __alloc_; + + public: + using value_type = __traits_t::value_type; + using pointer = __traits_t::pointer; + using size_type = __traits_t::size_type; + using is_always_equal = __traits_t::is_always_equal; + + template + struct rebind + { + using other = __allocator_adaptor>; + }; + + /*implicit*/ constexpr __allocator_adaptor(_Alloc const &__alloc) + noexcept(std::is_nothrow_copy_constructible_v<_Alloc>) + : __alloc_(__alloc) + {} + + template + /*implicit*/ constexpr __allocator_adaptor(__allocator_adaptor<_OtherAlloc> const &__other) + noexcept(std::is_nothrow_constructible_v<_Alloc, _OtherAlloc const &>) + : __alloc_(__other.__alloc_) + {} + + constexpr auto + allocate(size_type __n) noexcept(noexcept(std::declval<_Alloc &>().allocate(__n))) -> pointer + { + return __alloc_.allocate(__n); + } + + constexpr void deallocate(pointer __p, size_type __n) noexcept + { + __alloc_.deallocate(__p, __n); + } + + template + constexpr auto + operator==(__allocator_adaptor<_OtherAlloc> const &__other) const noexcept -> bool + { + return __alloc_ == __other.__alloc_; + } + }; + + //! An allocator that allocates from and deallocates to a memory resource of + //! type _Resource, which is std::pmr::memory_resource or derives from it + template + class __resource_allocator + { + template + friend class __resource_allocator; + + _Resource *__resource_; + + public: + using value_type = _Ty; + using pointer = value_type *; + + template + struct rebind + { + using other = __resource_allocator<_Resource, _Uy>; + }; + + // unlike polymorphic_allocator, there's no default constructor, because + // there's no default resource of type _Resource + + /*implicit*/ constexpr __resource_allocator(_Resource *__resource) noexcept + : __resource_(__resource) + {} + + template + /*implicit*/ constexpr __resource_allocator( + __resource_allocator<_Resource, _Uy> const &__other) noexcept + : __resource_(__other.__resource_) + {} + + constexpr auto allocate(std::size_t __n) -> pointer + { + return static_cast( + __resource_->allocate(__n * sizeof(value_type), alignof(value_type))); + } + + constexpr void deallocate(pointer __p, std::size_t __n) noexcept + { + __resource_->deallocate(__p, __n * sizeof(value_type), alignof(value_type)); + } + + constexpr auto resource() const noexcept -> _Resource * + { + return __resource_; + } + + template + constexpr auto + operator==(__resource_allocator<_Resource, _Uy> const &__other) const noexcept -> bool + { + return *__resource_ == *__other.__resource_; + } + }; + template struct __frame_allocator; //! Handle the case that _Delegate is an allocator already - //! - //! In this case, __frame_allocator_t is just an alias to - //! _Delegate but rebound to std::byte. template requires __simple_allocator<_Delegate> struct __frame_allocator<_Delegate> { - template - using type = std::allocator_traits<_Delegate>::template rebind_alloc<_Ty>; + using type = __allocator_adaptor< + typename std::allocator_traits<_Delegate>::template rebind_alloc>; }; - //! Handle the case that _Delegate is exactly std::pmr::memory_resource * - //! - //! In this case, __frame_allocator_t<_Delegate> is exactly - //! std::pmr::polymorphic_allocator. - template <> - struct __frame_allocator + //! Handle the case that _Delegate is a pointer to std::pmr::memory_resource + //! or to a type that derives from it + template + requires __std::derived_from<_Resource, std::pmr::memory_resource> + struct __frame_allocator<_Resource *> { - template - using type = std::pmr::polymorphic_allocator<_Ty>; - }; - - //! Handle the case that _Delegate is a pointer to a type that derives from - //! std::pmr::memory_resource - //! - //! In this case, __frame_allocator_t<_Delegate> is an allocator that - //! behaves like std::pmr::polymorphic_allocator except that it - //! knows the concrete type of its memory resource. In other words, - //! allocation and deallocation are delegated to the given resource, - //! construct does uses-allocator construction, etc. - template - requires __std::derived_from<_Delegate, std::pmr::memory_resource> - struct __frame_allocator<_Delegate *> - { - template - struct type - { - using value_type = _Ty; - using pointer = value_type *; - - // polymorphic_allocator's default constructor grabs the default memory - // resource, which we can't do because there's no default for _Delegate - - /*implicit*/ constexpr type(_Delegate *__resource) noexcept - : __resource_(__resource) - {} - - constexpr type(type const &) noexcept = default; - - template - /*implicit*/ constexpr type(type<_Uy> const &other) noexcept - : __resource_(other.resource()) - {} - - constexpr ~type() = default; - - constexpr type &operator=(type const &) noexcept = default; - - constexpr pointer allocate(std::size_t __n) - { - return static_cast( - __resource_->allocate(__n * sizeof(value_type), alignof(value_type))); - } - - constexpr void deallocate(void *__p, std::size_t __n) noexcept - { - __resource_->deallocate(__p, __n * sizeof(value_type), alignof(value_type)); - } - - template - constexpr void construct(_Uy *__p, _Args &&...__args) - { - (void) std::uninitialized_construct_using_allocator(__p, - *this, - static_cast<_Args &&>(__args)...); - } - - template - constexpr void destroy(_Uy *__p) noexcept - { - __p->~_Uy(); - } - - constexpr _Delegate *resource() const noexcept - { - return __resource_; - } - - template - constexpr bool operator==(type<_Uy> const &__other) const noexcept - { - return *__resource_ == *__other.resource(); - } - - private: - _Delegate *__resource_; - }; + using type = __resource_allocator<_Resource, std::byte>; }; } // namespace __fa template - using __frame_allocator_t = - __fa::__frame_allocator>::template type; + using __frame_allocator_t = __fa::__frame_allocator>::type; } // namespace experimental::execution namespace exec = experimental::execution; diff --git a/include/exec/function.hpp b/include/exec/function.hpp index 3a9d08e67..5aa5a682f 100644 --- a/include/exec/function.hpp +++ b/include/exec/function.hpp @@ -731,24 +731,12 @@ namespace experimental::execution using __alloc_t = decltype(__choose_frame_allocator(STDEXEC::get_env(__rcvr))); auto __alloc = __frame_allocator_t<__alloc_t>( __choose_frame_allocator(STDEXEC::get_env(__rcvr))); - // Ideally, function would allocate raw storage for the operation state - // from __alloc and construct it with construct_at. Instead, an - // operation state too big for the inline buffer is wrapped by the - // __any machinery that function shares with any_sender_of, and the - // wrapper is constructed and destroyed with - // allocator_traits<...>::construct and destroy. - // Uses-allocator construction never reaches the operation state: the - // wrapper doesn't declare allocator_type, and the operation state is - // initialized inside it directly from connect's result. What does - // leak is that a frame allocator's own construct and destroy members, - // if it has them, are called with the wrapper's type. That happens - // only when the function declares the get_frame_allocator query; - // otherwise __alloc is the injected polymorphic_allocator, which - // forwards only allocate and deallocate to the caller's allocator. - // Tolerated because frame allocators are unlikely to customize - // construct or destroy. A fix would give function its own path through - // __any (raw storage plus construct_at and destroy_at), leaving - // any_sender_of unchanged. + // __alloc declares no construct or destroy members, so the __any + // machinery that function shares with any_sender_of constructs an + // operation state too big for the inline buffer with construct_at in + // storage allocated from __alloc, and destroys it with destroy_at. + static_assert(!requires(decltype(__alloc) &__a, std::byte *__p) { __a.construct(__p); }); + static_assert(!requires(decltype(__alloc) &__a, std::byte *__p) { __a.destroy(__p); }); return _any::_any_opstate_base(__in_place_from, std::allocator_arg, __alloc, diff --git a/test/exec/test_frame_allocator.cpp b/test/exec/test_frame_allocator.cpp index dde21550c..42abe3b16 100644 --- a/test/exec/test_frame_allocator.cpp +++ b/test/exec/test_frame_allocator.cpp @@ -28,24 +28,33 @@ namespace ex = STDEXEC; namespace { + template + concept declares_construct_or_destroy = requires(Alloc &alloc, int *p) { alloc.construct(p, 42); } + || requires(Alloc &alloc, int *p) { alloc.destroy(p); }; + TEST_CASE("exec::__frame_allocator is constructible", "[types][frame_allocator]") { using namespace exec; SECTION("with a memory_resource*") { - std::pmr::memory_resource* rsc = std::pmr::new_delete_resource(); + std::pmr::memory_resource *rsc = std::pmr::new_delete_resource(); __frame_allocator_t fa(rsc); - STATIC_REQUIRE(std::same_as>); + STATIC_REQUIRE(std::same_as); + STATIC_REQUIRE(!declares_construct_or_destroy); + REQUIRE(fa.resource() == rsc); } SECTION("with a specific resource") { std::pmr::unsynchronized_pool_resource rsc; - __frame_allocator_t fa(&rsc); + __frame_allocator_t fa(&rsc); - STATIC_REQUIRE(!std::same_as>); + STATIC_REQUIRE(std::same_as); + STATIC_REQUIRE(!declares_construct_or_destroy); + STATIC_REQUIRE( + std::same_as); } SECTION("with an allocator") @@ -53,7 +62,8 @@ namespace std::allocator rsc; __frame_allocator_t fa(rsc); - STATIC_REQUIRE(std::same_as>); + STATIC_REQUIRE(std::same_as); + STATIC_REQUIRE(!declares_construct_or_destroy); } } @@ -74,7 +84,7 @@ namespace using resource_for_t = resource_for::type; template - constexpr decltype(auto) frame_allocator_arg_from(Resource& resource) noexcept + constexpr decltype(auto) frame_allocator_arg_from(Resource &resource) noexcept { if constexpr (ex::__std::derived_from) { @@ -102,12 +112,12 @@ namespace bool usedAllocator; }; - TEMPLATE_TEST_CASE("__frame_allocator correctly does uses-allocator construction with " + TEMPLATE_TEST_CASE("__frame_allocator doesn't do uses-allocator construction of " "leading-allocator types", "[types][frame_allocator]", std::allocator, - std::pmr::memory_resource*, - std::pmr::unsynchronized_pool_resource*) + std::pmr::memory_resource *, + std::pmr::unsynchronized_pool_resource *) { using raw_alloc = exec::__frame_allocator_t; using traits_t = @@ -118,20 +128,11 @@ namespace resource_for_t rsc; alloc_t alloc(frame_allocator_arg_from(rsc)); - uses_leading_allocator* p = traits_t::allocate(alloc, 1); + uses_leading_allocator *p = traits_t::allocate(alloc, 1); traits_t::construct(alloc, p, 42); - if (std::is_pointer_v) - { - // std::pmr::polymorphic_allocator and the static equivalent do - // uses-allocator construction - REQUIRE(p->usedAllocator); - } - else - { - // std::allocator does not do uses-allocator construction - REQUIRE(!p->usedAllocator); - } + // a frame allocator only provides storage + REQUIRE(!p->usedAllocator); traits_t::destroy(alloc, p); traits_t::deallocate(alloc, p, 1); @@ -153,12 +154,12 @@ namespace bool usedAllocator; }; - TEMPLATE_TEST_CASE("__frame_allocator correctly does uses-allocator construction with " + TEMPLATE_TEST_CASE("__frame_allocator doesn't do uses-allocator construction of " "trailing-allocator types", "[types][frame_allocator]", std::allocator, - std::pmr::memory_resource*, - std::pmr::unsynchronized_pool_resource*) + std::pmr::memory_resource *, + std::pmr::unsynchronized_pool_resource *) { using raw_alloc = exec::__frame_allocator_t; using traits_t = @@ -169,22 +170,74 @@ namespace resource_for_t rsc; alloc_t alloc(frame_allocator_arg_from(rsc)); - uses_trailing_allocator* p = traits_t::allocate(alloc, 1); + uses_trailing_allocator *p = traits_t::allocate(alloc, 1); traits_t::construct(alloc, p, 42); - if (std::is_pointer_v) + // a frame allocator only provides storage + REQUIRE(!p->usedAllocator); + + traits_t::destroy(alloc, p); + traits_t::deallocate(alloc, p, 1); + } + + struct construct_counts + { + int construct = 0; + int destroy = 0; + }; + + //! Counts calls to its own construct and destroy + template + struct constructing_allocator : std::allocator + { + template + struct rebind { - // std::pmr::polymorphic_allocator and the static equivalent do - // uses-allocator construction - REQUIRE(p->usedAllocator); + using other = constructing_allocator; + }; + + construct_counts *counts_; + + explicit constructing_allocator(construct_counts *counts) noexcept + : counts_(counts) + {} + + template + constructing_allocator(constructing_allocator const &other) noexcept + : counts_(other.counts_) + {} + + template + void construct(U *p, Args &&...args) + { + ++counts_->construct; + std::construct_at(p, static_cast(args)...); } - else + + template + void destroy(U *p) noexcept { - // std::allocator does not do uses-allocator construction - REQUIRE(!p->usedAllocator); + ++counts_->destroy; + std::destroy_at(p); } + }; + + TEST_CASE("__frame_allocator doesn't call a delegate allocator's construct or destroy", + "[types][frame_allocator]") + { + using alloc_t = exec::__frame_allocator_t>; + using traits_t = std::allocator_traits::template rebind_traits; + construct_counts counts; + traits_t::allocator_type alloc{alloc_t(constructing_allocator(&counts))}; + + int *p = traits_t::allocate(alloc, 1); + traits_t::construct(alloc, p, 42); + REQUIRE(*p == 42); traits_t::destroy(alloc, p); traits_t::deallocate(alloc, p, 1); + + REQUIRE(counts.construct == 0); + REQUIRE(counts.destroy == 0); } } // namespace diff --git a/test/exec/test_function.cpp b/test/exec/test_function.cpp index ec380eb92..f057a0198 100644 --- a/test/exec/test_function.cpp +++ b/test/exec/test_function.cpp @@ -335,6 +335,85 @@ namespace REQUIRE(res.count == 1); } + //! Counts calls to its own construct and destroy, which a frame allocator + //! may declare but which function must not call + struct allocator_counts + { + int allocate = 0; + int construct = 0; + int destroy = 0; + }; + + template + struct constructing_allocator + { + using value_type = T; + + allocator_counts *counts_; + + explicit constructing_allocator(allocator_counts *c) noexcept + : counts_(c) + {} + + template + constructing_allocator(constructing_allocator const &other) noexcept + : counts_(other.counts_) + {} + + T *allocate(std::size_t n) + { + ++counts_->allocate; + return std::allocator().allocate(n); + } + + void deallocate(T *p, std::size_t n) noexcept + { + std::allocator().deallocate(p, n); + } + + template + void construct(U *p, Args &&...args) + { + ++counts_->construct; + std::construct_at(p, static_cast(args)...); + } + + template + void destroy(U *p) noexcept + { + ++counts_->destroy; + std::destroy_at(p); + } + + template + bool operator==(constructing_allocator const &other) const noexcept + { + return counts_ == other.counts_; + } + }; + + TEST_CASE("function allocates from a declared frame allocator but constructs the operation " + "state itself", + "[types][function]") + { + // big enough that the erased operation state can't be stored inline + using big = std::array; + using alloc = constructing_allocator; + + allocator_counts counts; + exec::function> sndr( + [] { return ex::just(big{}); }); + + auto [ret] = ex::sync_wait(std::move(sndr) + | ex::write_env(ex::prop(exec::get_frame_allocator, alloc(&counts)))) + .value(); + + REQUIRE(ret == big{}); + REQUIRE(counts.allocate == 1); + REQUIRE(counts.construct == 0); + REQUIRE(counts.destroy == 0); + } + //! An allocator whose allocate is declared noexcept, counting allocations template struct nothrow_allocator