On Tue, Jul 8, 2025 at 3:24 PM Jonathan Wakely <jwak...@redhat.com> wrote:
> 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).
>
Thanks for the explanation, I missed the most important part of the patch,
and focused
on the bit_ceil call instead.
>
> >
> > 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
> >> >>
> >>
>
>
