On Tue, 8 Jul 2025 at 13:31, Tomasz Kaminski <tkami...@redhat.com> wrote:
>
>
>
> 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.
>
> I have expressed my preference of doing this check inside the
> memory_resouce::allocate.
We can still do that separately. The point of this patch is to fix the
misaligned allocations from the pool resources, not improve the EB vs
UB position for the memory_resource::allocate API.
> I know that this throws any potential detection of misaligned resources under
> the bus,
> however I think there is more benefit of not exposing custom resources to
> strange alignment values,
> and using the wrong pool.
>
> On the standard level, I think we should make behavior erroneous here, with
> defined behavior being bit_ceil.
Yeah, I think that's a good idea.
>>
>>
>> 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;
>> +
>> + 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
>>
