On Tue, 8 Jul 2025 at 14:12, Tomasz Kaminski <tkami...@redhat.com> wrote:
>
>
>
> On Tue, Jul 8, 2025 at 2:48 PM Jonathan Wakely <jwak...@redhat.com> wrote:
>>
>> 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.
>
> Because do_allocate is not publicly accessible, and can be called only via
> allocate, changing
> memory_resource::allocate API will also fix the misaligned allocations from
> pool resources.
No it won't, the bug happens with res.allocate(24,16) and 16 is a
valid alignment.
The bug is in the logic that finds the appropriate object pool to
allocate from, and happens for valid alignments.
The reason I added a call to bit_ceil in the patch is because the fix
to round up to a multiple of the alignment assumes that the alignment
is valid (previously we didn't really care if it was valid, we just
used it).
>
> And I believe, moving the check between specific pool do_allocate to
> memory_resource::allocate,
> would be ABI break, as we may end up with binary that contains:
> * memory_resource::allocate without check (check is in Derived::do_allocate)
> * Derived::do_allocate without check (check is memory_resource::allocate)
> (Let me know if I misunderstood how this kind of ABI breaks work).
>>
>>
>> > 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
>> >>
>>
