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.
>
> 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).
You understand correctly, but there is no ABI break. Firstly, it only
affects programs that pass invalid alignments, which have UB already.
Secondly, there are already objects out there in the world compiled
without a check in memory_resource::allocate and we can't make them
start enforcing valid alignments now.
So it's only a problem for programs with UB, and the problem already
exists even if we change memory_resource::allocate now.
But it's still a separate issue to this bug fix :-)
>>
>>
>> > 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
>> >>
>>
