Bug 90569 - __STDCPP_DEFAULT_NEW_ALIGNMENT__ is wrong for i386-pc-solaris2.11
Summary: __STDCPP_DEFAULT_NEW_ALIGNMENT__ is wrong for i386-pc-solaris2.11
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 10.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks: 77691
  Show dependency treegraph
 
Reported: 2019-05-22 11:34 UTC by Jonathan Wakely
Modified: 2024-06-15 02:27 UTC (History)
4 users (show)

See Also:
Host:
Target: i386-pc-solaris2.11
Build:
Known to work:
Known to fail:
Last reconfirmed: 2019-05-22 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Wakely 2019-05-22 11:34:08 UTC
The default value for __STDCPP_DEFAULT_NEW_ALIGNMENT__ is alignof(max_align_t), but GCC's max_align_t does not agree with the OS on i386-pc-solaris2.*, see PR 77691. This means that operator new(size_t) does not necessarily meet the guarantee that __STDCPP_DEFAULT_NEW_ALIGNMENT__ is supposed to provide.

It looks like changing MALLOC_ABI_ALIGNMENT for the target should work, except for this in gcc/cp/init.c:

/* Return the alignment we expect malloc to guarantee.  This should just be
   MALLOC_ABI_ALIGNMENT, but that macro defaults to only BITS_PER_WORD for some
   reason, so don't let the threshold be smaller than max_align_t_align.  */

unsigned
malloc_alignment ()
{
  return MAX (max_align_t_align(), MALLOC_ABI_ALIGNMENT);
}

Shouldn't we trust the target if it has overridden the default? e.g.

/* Return the alignment we expect malloc to guarantee.  This should just be
   MALLOC_ABI_ALIGNMENT, but that macro defaults to only BITS_PER_WORD for some
   reason.  If MALLOC_ABI_ALIGNMENT has been changed from the default, use it.
   Otherwise don't let the threshold be smaller than max_align_t_align.  */

unsigned
malloc_alignment ()
{
  if (MALLOC_ABI_ALIGNMENT != BITS_PER_WORD)
    return MALLOC_ABI_ALIGNMENT;
  return MAX (max_align_t_align(), MALLOC_ABI_ALIGNMENT);
}
Comment 1 Jonathan Wakely 2019-05-22 11:37:38 UTC
(I'm starting to think that __float128 support should have been disabled on targets where it requires greater alignment than malloc guarantees, instead of making GCC's max_align_t lie).
Comment 2 Jonathan Wakely 2019-05-22 14:18:16 UTC
I thought we could workaround this in libstdc++ like so:

diff --git a/libstdc++-v3/libsupc++/Makefile.am b/libstdc++-v3/libsupc++/Makefile.am
index eec7b953514..a50a9848461 100644
--- a/libstdc++-v3/libsupc++/Makefile.am
+++ b/libstdc++-v3/libsupc++/Makefile.am
@@ -129,6 +129,8 @@ cp-demangle.o: cp-demangle.c
 
 
 # Use special rules for the C++17 sources so that the proper flags are passed.
+new_op.lo: new_op.cc
+       $(LTCXXCOMPILE) -std=gnu++1z -c $<
 new_opa.lo: new_opa.cc
        $(LTCXXCOMPILE) -std=gnu++1z -c $<
 new_opant.lo: new_opant.cc
diff --git a/libstdc++-v3/libsupc++/Makefile.in b/libstdc++-v3/libsupc++/Makefile.in
index 5d8ac5ca0ba..0e3cbff0055 100644
--- a/libstdc++-v3/libsupc++/Makefile.in
+++ b/libstdc++-v3/libsupc++/Makefile.in
@@ -956,6 +956,8 @@ cp-demangle.o: cp-demangle.c
        $(C_COMPILE) -DIN_GLIBCPP_V3 -Wno-error -c $<
 
 # Use special rules for the C++17 sources so that the proper flags are passed.
+new_op.lo: new_op.cc
+       $(LTCXXCOMPILE) -std=gnu++1z -c $<
 new_opa.lo: new_opa.cc
        $(LTCXXCOMPILE) -std=gnu++1z -c $<
 new_opant.lo: new_opant.cc
diff --git a/libstdc++-v3/libsupc++/new_op.cc b/libstdc++-v3/libsupc++/new_op.cc
index 863530b7564..203c57d9171 100644
--- a/libstdc++-v3/libsupc++/new_op.cc
+++ b/libstdc++-v3/libsupc++/new_op.cc
@@ -27,6 +27,9 @@
 #include <cstdlib>
 #include <bits/exception_defines.h>
 #include "new"
+#if defined __sun__ || defined __i386__
+# include <cstddef>
+#endif
 
 using std::new_handler;
 using std::bad_alloc;
@@ -41,6 +44,14 @@ extern "C" void *malloc (std::size_t);
 _GLIBCXX_WEAK_DEFINITION void *
 operator new (std::size_t sz) _GLIBCXX_THROW (std::bad_alloc)
 {
+#if defined __sun__ || defined __i386__
+  if (sz >= alignof(std::max_align_t))
+    {
+      std::align_val_t al{alignof(std::max_align_t)};
+      return ::operator new(sz, al);
+    }
+#endif
+
   void *p;
 
   /* malloc (0) is unpredictable; avoid it.  */



This would force operator new to use aligned_alloc instead of malloc for allocations that might be for objects large enough to require greater alignment than malloc guarantees. But since Solaris 11 doesn't appear to define aligned_alloc, this would use the fallback implementation in libsupc++/new_opa.cc which is much less efficient than plain malloc.
Comment 3 Jason Merrill 2019-05-22 14:52:08 UTC
(In reply to Jonathan Wakely from comment #0)
> unsigned
> malloc_alignment ()
> {
>   if (MALLOC_ABI_ALIGNMENT != BITS_PER_WORD)
>     return MALLOC_ABI_ALIGNMENT;
>   return MAX (max_align_t_align(), MALLOC_ABI_ALIGNMENT);
> }

The last line can just be 

  return max_align_t_align();

Otherwise looks good to me.
Comment 4 Jonathan Wakely 2019-05-22 15:02:03 UTC
Rainer, the change to gcc/cp/init.c would allow you to do:

#define MALLOC_ABI_ALIGNMENT 8

in gcc/config/i386/sol2.h and that would cause std::allocator to know that it can't rely on malloc for 16-byte alignment.

Although that would only help for C++17, because otherwise __cpp_aligned_new isn't defined ... drat.

It's better than nothing though.

Does that seem acceptable for your target?
Comment 5 Jonathan Wakely 2019-05-22 15:45:15 UTC
(In reply to Jonathan Wakely from comment #4)
> Rainer, the change to gcc/cp/init.c would allow you to do:
> 
> #define MALLOC_ABI_ALIGNMENT 8

Oops, it's in bits not bytes, so that should be

#define MALLOC_ABI_ALIGNMENT 64
Comment 6 Jonathan Wakely 2019-05-22 22:39:57 UTC
This bug also affects 32-bit GNU/Linux with older versions of glibc.
Comment 7 Jonathan Wakely 2019-05-23 08:08:08 UTC
The glibc change was https://sourceware.org/bugzilla/show_bug.cgi?id=21120 and is present from version 2.26
Comment 8 Florian Weimer 2019-05-23 14:25:21 UTC
(In reply to Jonathan Wakely from comment #1)
> (I'm starting to think that __float128 support should have been disabled on
> targets where it requires greater alignment than malloc guarantees, instead
> of making GCC's max_align_t lie).

The problem is that popular mallocs do not care about ABI and return unaligned pointers for allocations smaller than the max_align_t alignment.  As a result, __float128 support with 16-byte alignment would end up with a potentially rather short list of supported targets (including targets which have a 16-byte-aligned long double already).

We spent a lot energy fixing this in glibc without breaking backwards compatibility with existing Emacs binaries:

  <https://sourceware.org/bugzilla/show_bug.cgi?id=6527>

Basically, we had to include a heap rewriter that converts the heap from the old to the new format.  But other mallocs wouldn't even have to do that, and they still don't want to fix their default alignment choices unfortunately.

I also found this for the first time:

  <http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2293.htm>

So if the standard moves to the weak guarantee, GCC probably shouldn't make assumptions in the other direction.
Comment 9 Jonathan Wakely 2019-05-23 14:44:19 UTC
(In reply to Florian Weimer from comment #8)
> The problem is that popular mallocs do not care about ABI and return
> unaligned pointers for allocations smaller than the max_align_t alignment. 
> As a result, __float128 support with 16-byte alignment would end up with a
> potentially rather short list of supported targets (including targets which
> have a 16-byte-aligned long double already).

Understood.

> I also found this for the first time:
> 
>   <http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2293.htm>
> 
> So if the standard moves to the weak guarantee, GCC probably shouldn't make
> assumptions in the other direction.

Yes, I already fixed libstdc++ to not assume the strong-alignment reading, see r265068.

But this PR isn't about the alignment of allocations where the size is smaller than alignof(max_align_t). What makes my life difficult is that some mallocs (including glibc up to 2.25) do not respect GCC's new alignof(max_align_t) even for large allocations. I understand why, but it's just annoying that GCC changed max_align_t to be misleading. If the underlying malloc doesn't agree, making max_align_t lie isn't very helpful.
Comment 10 Jonathan Wakely 2020-11-17 00:13:45 UTC
This isn't solaris-specific, it affects vxworks and mingw too.