| Message ID | orwo5mdvn7.fsf@livre.home |
|---|---|
| State | New |
| Headers | show |
| Series | [PR77691] x86-vxworks malloc aligns to 8 bytes like solaris | expand |
On 08/05/20 17:22 -0300, Alexandre Oliva wrote: > >Vxworks 7's malloc, like Solaris', only ensures 8-byte alignment of >returned pointers on 32-bit x86, though GCC's stddef.h defines >max_align_t with 16-byte alignment for __float128. This patch enables >on x86-vxworks the same memory_resource workaround used for x86-solaris. > >The testsuite also had a workaround, defining BAD_MAX_ALIGN_T and >xfailing the test; extend those to x86-vxworks as well, and fix the >one test that could still fail on x86-vxworks, that expected >char-aligned requested allocation to be aligned for max_align_t. With >that change, the test passes on x86-vxworks; I'm guessing that's the >same reason for the test not to pass on x86-solaris (and on >x86_64-solaris -m32), so with the fix, I'm tentatively removing the >xfail. > >Tested on x86_64-linux-gnu-x-i586-vxworks7.2. Ok to install? Yes, but ... >(Couldn't r1->allocate(2, alignof(char)) possibly return a pointer >that's *not* aligned<max_align_t>? Maybe we should drop the test even >if !defined(BAD_MAX_ALIGN_T).) Yes. Different malloc implementations interpret the C standard differently here. One interpretation is that all allocations must be aligned to alignof(max_align_t) but another is that allocations smaller than that don't need to meet that requirement. An object that is two bytes in size cannot require 16-byte alignment (otherwise its sizeof would be 16 too). Some mallocs behave the second way, so it doesn't really matter what the correct interpretation of the C standard is, libstdc++ has to accept reality and work with those mallocs. And so I think that test is buggy and shouldn't assume small allocations will use alignof(max_align_t). Please do remove that line of the test, instead of wrapping it in the #ifdef. OK for master. Also approved for gcc-10 branch if the target maintainers are OK with it. It's an ABI change (allocations aligned to alignof(max_align_t) will store the token in the allocation after this change) but the header is an experimental TS feature so not guaranteed to be stable. The target maintainers can decide if they'd rather have GCC 10.2 be consistent with 10.1 or with master. >for libstdc++-v3/ChangeLog > > PR libstdc++/77691 > * include/experimental/memory_resource > (__resource_adaptor_imp::do_allocate): Handle max_align_t on > x86-vxworks as on x86-solaris. > (__resource_adaptor_imp::do_deallocate): Likewise. > * testsuite/experimental/memory_resource/new_delete_resource.cc: > Drop xfail. > (BAD_MAX_ALIGN_T): Define on x86-vxworks as on x86-solaris. > (test03): Skip align test for char-aligned alloc if > BAD_MAX_ALIGN_T. >--- > libstdc++-v3/include/experimental/memory_resource | 4 ++-- > .../memory_resource/new_delete_resource.cc | 5 +++-- > 2 files changed, 5 insertions(+), 4 deletions(-) > >diff --git a/libstdc++-v3/include/experimental/memory_resource b/libstdc++-v3/include/experimental/memory_resource >index 850a78d..1c4de70 100644 >--- a/libstdc++-v3/include/experimental/memory_resource >+++ b/libstdc++-v3/include/experimental/memory_resource >@@ -413,7 +413,7 @@ namespace pmr { > do_allocate(size_t __bytes, size_t __alignment) override > { > // Cannot use max_align_t on 32-bit Solaris x86, see PR libstdc++/77691 >-#if ! (defined __sun__ && defined __i386__) >+#if ! ((defined __sun__ || defined __VXWORKS__) && defined __i386__) > if (__alignment == alignof(max_align_t)) > return _M_allocate<alignof(max_align_t)>(__bytes); > #endif >@@ -439,7 +439,7 @@ namespace pmr { > do_deallocate(void* __ptr, size_t __bytes, size_t __alignment) noexcept > override > { >-#if ! (defined __sun__ && defined __i386__) >+#if ! ((defined __sun__ || defined __VXWORKS__) && defined __i386__) > if (__alignment == alignof(max_align_t)) > return (void) _M_deallocate<alignof(max_align_t)>(__ptr, __bytes); > #endif >diff --git a/libstdc++-v3/testsuite/experimental/memory_resource/new_delete_resource.cc b/libstdc++-v3/testsuite/experimental/memory_resource/new_delete_resource.cc >index 8a98954..8119349 100644 >--- a/libstdc++-v3/testsuite/experimental/memory_resource/new_delete_resource.cc >+++ b/libstdc++-v3/testsuite/experimental/memory_resource/new_delete_resource.cc >@@ -17,13 +17,12 @@ > > // { dg-do run { target c++14 } } > // { dg-require-cstdint "" } >-// { dg-xfail-run-if "PR libstdc++/77691" { { i?86-*-solaris2.* x86_64-*-solaris2.* } && ilp32 } } > > #include <experimental/memory_resource> > #include <cstdlib> > #include <testsuite_hooks.h> > >-#if defined __sun__ && defined __i386__ >+#if (defined __sun__ || defined __VXWORKS__) && defined __i386__ > // See PR libstdc++/77691 > # define BAD_MAX_ALIGN_T 1 > #endif >@@ -128,7 +127,9 @@ test03() > > p = r1->allocate(2, alignof(char)); > VERIFY( bytes_allocated == 2 ); >+#ifndef BAD_MAX_ALIGN_T > VERIFY( aligned<max_align_t>(p) ); >+#endif > r1->deallocate(p, 2, alignof(char)); > VERIFY( bytes_allocated == 0 ); > > >-- >Alexandre Oliva, freedom fighter he/him https://FSFLA.org/blogs/lxo/ >Free Software Evangelist Stallman was right, but he's left :( >GNU Toolchain Engineer Live long and free, and prosper ethically >
Hello, Jonathan, On May 9, 2020, Jonathan Wakely <jwakely@redhat.com> wrote: > On 08/05/20 17:22 -0300, Alexandre Oliva wrote: >> (Couldn't r1->allocate(2, alignof(char)) possibly return a pointer >> that's *not* aligned<max_align_t>? Maybe we should drop the test even >> if !defined(BAD_MAX_ALIGN_T).) > Yes. > Different malloc implementations interpret the C standard differently > here. One interpretation is that all allocations must be aligned to > alignof(max_align_t) but another is that allocations smaller than that > don't need to meet that requirement. An object that is two bytes in > size cannot require 16-byte alignment (otherwise its sizeof would be > 16 too). I understand you're talking about malloc because that's what our implementation ultimately uses, but my question was on language lawyering, on whether C++ would mandate more alignment than requested by the caller of allocate. If it were to do so, I wonder what the point of specifying the alignment explicitly would be. > Please do remove that line of the test, instead of wrapping it in the > #ifdef. > OK for master. Thanks, here's what I'm installing in master. x86-vxworks malloc aligns to 8 bytes like solaris From: Alexandre Oliva <oliva@adacore.com> Vxworks 7's malloc, like Solaris', only ensures 8-byte alignment of returned pointers on 32-bit x86, though GCC's stddef.h defines max_align_t with 16-byte alignment for __float128. This patch enables on x86-vxworks the same memory_resource workaround used for x86-solaris. The testsuite also had a workaround, defining BAD_MAX_ALIGN_T and xfailing the test; extend those to x86-vxworks as well, and remove the check for char-aligned requested allocation to be aligned like max_align_t. With that change, the test passes on x86-vxworks; I'm guessing that's the same reason for the test not to pass on x86-solaris (and on x86_64-solaris -m32), so with the fix, I'm tentatively removing the xfail. for libstdc++-v3/ChangeLog PR libstdc++/77691 * include/experimental/memory_resource (__resource_adaptor_imp::do_allocate): Handle max_align_t on x86-vxworks as on x86-solaris. (__resource_adaptor_imp::do_deallocate): Likewise. * testsuite/experimental/memory_resource/new_delete_resource.cc: Drop xfail. (BAD_MAX_ALIGN_T): Define on x86-vxworks as on x86-solaris. (test03): Drop max-align test for char-aligned alloc. --- libstdc++-v3/include/experimental/memory_resource | 4 ++-- .../memory_resource/new_delete_resource.cc | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/libstdc++-v3/include/experimental/memory_resource b/libstdc++-v3/include/experimental/memory_resource index 850a78d..1c4de70 100644 --- a/libstdc++-v3/include/experimental/memory_resource +++ b/libstdc++-v3/include/experimental/memory_resource @@ -413,7 +413,7 @@ namespace pmr { do_allocate(size_t __bytes, size_t __alignment) override { // Cannot use max_align_t on 32-bit Solaris x86, see PR libstdc++/77691 -#if ! (defined __sun__ && defined __i386__) +#if ! ((defined __sun__ || defined __VXWORKS__) && defined __i386__) if (__alignment == alignof(max_align_t)) return _M_allocate<alignof(max_align_t)>(__bytes); #endif @@ -439,7 +439,7 @@ namespace pmr { do_deallocate(void* __ptr, size_t __bytes, size_t __alignment) noexcept override { -#if ! (defined __sun__ && defined __i386__) +#if ! ((defined __sun__ || defined __VXWORKS__) && defined __i386__) if (__alignment == alignof(max_align_t)) return (void) _M_deallocate<alignof(max_align_t)>(__ptr, __bytes); #endif diff --git a/libstdc++-v3/testsuite/experimental/memory_resource/new_delete_resource.cc b/libstdc++-v3/testsuite/experimental/memory_resource/new_delete_resource.cc index 8a98954..65a42da 100644 --- a/libstdc++-v3/testsuite/experimental/memory_resource/new_delete_resource.cc +++ b/libstdc++-v3/testsuite/experimental/memory_resource/new_delete_resource.cc @@ -17,13 +17,12 @@ // { dg-do run { target c++14 } } // { dg-require-cstdint "" } -// { dg-xfail-run-if "PR libstdc++/77691" { { i?86-*-solaris2.* x86_64-*-solaris2.* } && ilp32 } } #include <experimental/memory_resource> #include <cstdlib> #include <testsuite_hooks.h> -#if defined __sun__ && defined __i386__ +#if (defined __sun__ || defined __VXWORKS__) && defined __i386__ // See PR libstdc++/77691 # define BAD_MAX_ALIGN_T 1 #endif @@ -128,7 +127,6 @@ test03() p = r1->allocate(2, alignof(char)); VERIFY( bytes_allocated == 2 ); - VERIFY( aligned<max_align_t>(p) ); r1->deallocate(p, 2, alignof(char)); VERIFY( bytes_allocated == 0 );
On 13/05/20 04:49 -0300, Alexandre Oliva wrote: >Hello, Jonathan, > >On May 9, 2020, Jonathan Wakely <jwakely@redhat.com> wrote: > >> On 08/05/20 17:22 -0300, Alexandre Oliva wrote: > >>> (Couldn't r1->allocate(2, alignof(char)) possibly return a pointer >>> that's *not* aligned<max_align_t>? Maybe we should drop the test even >>> if !defined(BAD_MAX_ALIGN_T).) > >> Yes. > >> Different malloc implementations interpret the C standard differently >> here. One interpretation is that all allocations must be aligned to >> alignof(max_align_t) but another is that allocations smaller than that >> don't need to meet that requirement. An object that is two bytes in >> size cannot require 16-byte alignment (otherwise its sizeof would be >> 16 too). > >I understand you're talking about malloc because that's what our >implementation ultimately uses, but my question was on language >lawyering, on whether C++ would mandate more alignment than requested by >the caller of allocate. No it doesn't, but this is a test for our implementation, not the standard, and the new_delete_resource uses new which (in our implementation) uses malloc. That said, I'm not sure if I was really trying to test that property, or if including that line was just a mistake. I suspect it was just a mistake. >If it were to do so, I wonder what the point of >specifying the alignment explicitly would be. > >> Please do remove that line of the test, instead of wrapping it in the >> #ifdef. > >> OK for master. > >Thanks, here's what I'm installing in master. Thanks.
On May 9, 2020, Jonathan Wakely <jwakely@redhat.com> wrote: > Also approved for gcc-10 branch if the target maintainers are OK with > it. It's an ABI change (allocations aligned to alignof(max_align_t) > will store the token in the allocation after this change) but the > header is an experimental TS feature so not guaranteed to be stable. > The target maintainers can decide if they'd rather have GCC 10.2 be > consistent with 10.1 or with master. Olivier Hainque, with his vxworks maintainer hat on, gave me a go ahead to install it the patch in the gcc-10 branch, so I'm putting it in. Thanks,
On 26/05/20 07:11 -0300, Alexandre Oliva wrote: >On May 9, 2020, Jonathan Wakely <jwakely@redhat.com> wrote: > >> Also approved for gcc-10 branch if the target maintainers are OK with >> it. It's an ABI change (allocations aligned to alignof(max_align_t) >> will store the token in the allocation after this change) but the >> header is an experimental TS feature so not guaranteed to be stable. >> The target maintainers can decide if they'd rather have GCC 10.2 be >> consistent with 10.1 or with master. > >Olivier Hainque, with his vxworks maintainer hat on, gave me a go ahead >to install it the patch in the gcc-10 branch, so I'm putting it in. Great. Thanks, Alex.
diff --git a/libstdc++-v3/include/experimental/memory_resource b/libstdc++-v3/include/experimental/memory_resource index 850a78d..1c4de70 100644 --- a/libstdc++-v3/include/experimental/memory_resource +++ b/libstdc++-v3/include/experimental/memory_resource @@ -413,7 +413,7 @@ namespace pmr { do_allocate(size_t __bytes, size_t __alignment) override { // Cannot use max_align_t on 32-bit Solaris x86, see PR libstdc++/77691 -#if ! (defined __sun__ && defined __i386__) +#if ! ((defined __sun__ || defined __VXWORKS__) && defined __i386__) if (__alignment == alignof(max_align_t)) return _M_allocate<alignof(max_align_t)>(__bytes); #endif @@ -439,7 +439,7 @@ namespace pmr { do_deallocate(void* __ptr, size_t __bytes, size_t __alignment) noexcept override { -#if ! (defined __sun__ && defined __i386__) +#if ! ((defined __sun__ || defined __VXWORKS__) && defined __i386__) if (__alignment == alignof(max_align_t)) return (void) _M_deallocate<alignof(max_align_t)>(__ptr, __bytes); #endif diff --git a/libstdc++-v3/testsuite/experimental/memory_resource/new_delete_resource.cc b/libstdc++-v3/testsuite/experimental/memory_resource/new_delete_resource.cc index 8a98954..8119349 100644 --- a/libstdc++-v3/testsuite/experimental/memory_resource/new_delete_resource.cc +++ b/libstdc++-v3/testsuite/experimental/memory_resource/new_delete_resource.cc @@ -17,13 +17,12 @@ // { dg-do run { target c++14 } } // { dg-require-cstdint "" } -// { dg-xfail-run-if "PR libstdc++/77691" { { i?86-*-solaris2.* x86_64-*-solaris2.* } && ilp32 } } #include <experimental/memory_resource> #include <cstdlib> #include <testsuite_hooks.h> -#if defined __sun__ && defined __i386__ +#if (defined __sun__ || defined __VXWORKS__) && defined __i386__ // See PR libstdc++/77691 # define BAD_MAX_ALIGN_T 1 #endif @@ -128,7 +127,9 @@ test03() p = r1->allocate(2, alignof(char)); VERIFY( bytes_allocated == 2 ); +#ifndef BAD_MAX_ALIGN_T VERIFY( aligned<max_align_t>(p) ); +#endif r1->deallocate(p, 2, alignof(char)); VERIFY( bytes_allocated == 0 );