[PR77691] x86-vxworks malloc aligns to 8 bytes like solaris

[PR77691] x86-vxworks malloc aligns to 8 bytes like solaris

[Alexandre Oliva]

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?

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


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

Re: [PR77691] x86-vxworks malloc aligns to 8 bytes like solaris

[Jonathan Wakely]

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
>

Re: [PR77691] x86-vxworks malloc aligns to 8 bytes like solaris

[Alexandre Oliva]

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


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

Re: [PR77691] x86-vxworks malloc aligns to 8 bytes like solaris

[Jonathan Wakely]

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.

Re: [PR77691] x86-vxworks malloc aligns to 8 bytes like solaris

[Alexandre Oliva]

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,

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

Re: [PR77691] x86-vxworks malloc aligns to 8 bytes like solaris

[Jonathan Wakely]

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.