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).
>
> > 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
> >>
>
>
