On Sat, Jul 5, 2025 at 1:12 AM Jonathan Wakely <jwak...@redhat.com> wrote:
> For allocations with size > alignment and size % alignment != 0 we were
> sometimes returning pointers that did not meet the requested aligment.
> For example, allocate(24, 16) would select the pool for 24-byte objects
> and the second allocation from that pool (at offset 24 bytes into the
> pool) is only 8-byte aligned not 16-byte aligned.
>
> The pool resources need to round up the requested allocation size to a
> multiple of the alignment, so that the selected pool will always return
> allocations that meet the alignment requirement.
>
> libstdc++-v3/ChangeLog:
>
> PR libstdc++/118681
> * src/c++17/memory_resource.cc (choose_block_size): New
> function.
> (synchronized_pool_resource::do_allocate): Use choose_block_size
> to determine appropriate block size.
> (synchronized_pool_resource::do_deallocate): Likewise
> (unsynchronized_pool_resource::do_allocate): Likewise.
> (unsynchronized_pool_resource::do_deallocate): Likewise
> * testsuite/20_util/synchronized_pool_resource/118681.cc: New
> test.
> * testsuite/20_util/unsynchronized_pool_resource/118681.cc: New
> test.
> ---
>
> Tested x86_64-linux.
>
LGTM, now after I understood what this is fixing.
>
> libstdc++-v3/src/c++17/memory_resource.cc | 26 +++++++--
> .../synchronized_pool_resource/118681.cc | 5 ++
> .../unsynchronized_pool_resource/118681.cc | 58 +++++++++++++++++++
> 3 files changed, 85 insertions(+), 4 deletions(-)
> create mode 100644
> libstdc++-v3/testsuite/20_util/synchronized_pool_resource/118681.cc
> create mode 100644
> libstdc++-v3/testsuite/20_util/unsynchronized_pool_resource/118681.cc
>
> diff --git a/libstdc++-v3/src/c++17/memory_resource.cc
> b/libstdc++-v3/src/c++17/memory_resource.cc
> index fac4c782c5f7..fddfe2c7dd98 100644
> --- a/libstdc++-v3/src/c++17/memory_resource.cc
> +++ b/libstdc++-v3/src/c++17/memory_resource.cc
> @@ -1242,12 +1242,30 @@ namespace pmr
> return pools;
> }
>
> + static inline size_t
> + choose_block_size(size_t bytes, size_t alignment)
> + {
> + if (bytes == 0) [[unlikely]]
> + return alignment;
> +
> + // Use bit_ceil in case alignment is invalid (i.e. not a power of
> two).
> + size_t mask = std::__bit_ceil(alignment) - 1;
> + // Round up to a multiple of alignment.
> + size_t block_size = (bytes + mask) & ~mask;
>
Took me some time to convince myself that this is doing correct rounding of
alignment,
assuming it is a power of two.
> +
> + if (block_size >= bytes) [[likely]]
> + return block_size;
> +
> + // Wrapped around to zero, bytes must have been impossibly large.
> + return numeric_limits<size_t>::max();
> + }
> +
> // Override for memory_resource::do_allocate
> void*
> synchronized_pool_resource::
> do_allocate(size_t bytes, size_t alignment)
> {
> - const auto block_size = std::max(bytes, alignment);
> + const auto block_size = choose_block_size(bytes, alignment);
> const pool_options opts = _M_impl._M_opts;
> if (block_size <= opts.largest_required_pool_block)
> {
> @@ -1294,7 +1312,7 @@ namespace pmr
> synchronized_pool_resource::
> do_deallocate(void* p, size_t bytes, size_t alignment)
> {
> - size_t block_size = std::max(bytes, alignment);
> + size_t block_size = choose_block_size(bytes, alignment);
> if (block_size <= _M_impl._M_opts.largest_required_pool_block)
> {
> const ptrdiff_t index = pool_index(block_size, _M_impl._M_npools);
> @@ -1453,7 +1471,7 @@ namespace pmr
> void*
> unsynchronized_pool_resource::do_allocate(size_t bytes, size_t
> alignment)
> {
> - const auto block_size = std::max(bytes, alignment);
> + const auto block_size = choose_block_size(bytes, alignment);
> if (block_size <= _M_impl._M_opts.largest_required_pool_block)
> {
> // Recreate pools if release() has been called:
> @@ -1470,7 +1488,7 @@ namespace pmr
> unsynchronized_pool_resource::
> do_deallocate(void* p, size_t bytes, size_t alignment)
> {
> - size_t block_size = std::max(bytes, alignment);
> + size_t block_size = choose_block_size(bytes, alignment);
> if (block_size <= _M_impl._M_opts.largest_required_pool_block)
> {
> if (auto pool = _M_find_pool(block_size))
> diff --git
> a/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/118681.cc
> b/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/118681.cc
> new file mode 100644
> index 000000000000..6d7434ff9106
> --- /dev/null
> +++ b/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/118681.cc
> @@ -0,0 +1,5 @@
> +// { dg-do run { target c++17 } }
> +// Bug 118681 - unsynchronized_pool_resource may fail to respect alignment
> +
> +#define RESOURCE std::pmr::synchronized_pool_resource
> +#include "../unsynchronized_pool_resource/118681.cc"
> diff --git
> a/libstdc++-v3/testsuite/20_util/unsynchronized_pool_resource/118681.cc
> b/libstdc++-v3/testsuite/20_util/unsynchronized_pool_resource/118681.cc
> new file mode 100644
> index 000000000000..87e1b1d94043
> --- /dev/null
> +++ b/libstdc++-v3/testsuite/20_util/unsynchronized_pool_resource/118681.cc
> @@ -0,0 +1,58 @@
> +// { dg-do run { target c++17 } }
> +// Bug 118681 - unsynchronized_pool_resource may fail to respect alignment
> +
> +#include <memory_resource>
> +#include <cstdio>
> +#include <testsuite_hooks.h>
> +
> +#ifndef RESOURCE
> +# define RESOURCE std::pmr::unsynchronized_pool_resource
> +#endif
> +
> +bool any_misaligned = false;
> +
> +bool
> +is_aligned(void* p, [[maybe_unused]] std::size_t size, std::size_t
> alignment)
> +{
> + const bool misaligned = reinterpret_cast<std::uintptr_t>(p) % alignment;
> +#ifdef DEBUG
> + std::printf("allocate(%2zu, %2zu): %p is aligned %scorrectly\n",
> + size, alignment, p, misaligned ? "in" : "");
> + any_misaligned |= misaligned;
> + return true;
> +#endif
> + return ! misaligned;
> +}
> +
> +void
> +test_alignment(std::pmr::memory_resource& res, bool dealloc)
> +{
> + for (std::size_t alignment : { 8, 16, 32, 64 })
> + {
> + for (std::size_t size : { 9, 12, 24, 40, 48, 56, 72 })
> + {
> + void* p1 = res.allocate(size, alignment);
> + void* p2 = res.allocate(size, alignment);
> +
> + VERIFY( is_aligned(p1, size, alignment) );
> + VERIFY( is_aligned(p2, size, alignment) );
> +
> + if (dealloc)
> + {
> + res.deallocate(p2, size, alignment);
> + res.deallocate(p2, size, alignment);
> + }
> + }
> + }
> +}
> +
> +int main()
> +{
> + RESOURCE res;
> + test_alignment(res, true);
> + res.release();
> + test_alignment(res, false);
> + res.release();
> +
> + VERIFY( ! any_misaligned );
> +}
> --
> 2.50.0
>
>
