Bug 86861 - 18_support/new_aligned.cc FAILs
Summary: 18_support/new_aligned.cc FAILs
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 9.0
: P3 normal
Target Milestone: 7.4
Assignee: Jonathan Wakely
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-05 14:26 UTC by Rainer Orth
Modified: 2018-08-07 22:53 UTC (History)
2 users (show)

See Also:
Host: *-*-solaris2.10
Target: *-*-solaris2.10
Build: *-*-solaris2.10
Known to work:
Known to fail:
Last reconfirmed: 2018-08-06 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Rainer Orth 2018-08-05 14:26:56 UTC
Thew new 18_support/new_aligned.cc test FAILs on Solaris 10 (sparc and x86),
which lacks aligned_alloc in libc:

+FAIL: 18_support/new_aligned.cc execution test

terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc
[New Thread 1 (LWP 1)]

Thread 2 received signal SIGABRT, Aborted.
[Switching to Thread 1 (LWP 1)]
0xfec5c9b5 in _lwp_kill () from /lib/libc.so.1
(gdb) where
#0  0xfec5c9b5 in _lwp_kill () from /lib/libc.so.1
#1  0xfec5782c in thr_kill () from /lib/libc.so.1
#2  0xfec037db in raise () from /lib/libc.so.1
#3  0xfebe29f5 in abort () from /lib/libc.so.1
#4  0xfeedddad in __gnu_cxx::__verbose_terminate_handler ()
    at /vol/gcc/src/hg/trunk/local/libstdc++-v3/libsupc++/vterminate.cc:95
#5  0xfeeda727 in __cxxabiv1::__terminate(void (*)()) ()
    at /vol/gcc/src/hg/trunk/local/libstdc++-v3/libsupc++/eh_terminate.cc:47
#6  0xfeeda7a0 in std::terminate ()
    at /vol/gcc/src/hg/trunk/local/libstdc++-v3/libsupc++/eh_terminate.cc:57
#7  0xfeedaaa8 in __cxxabiv1::__cxa_throw (obj=0x8066ab8, 
    tinfo=0xfef763ac <typeinfo for std::bad_alloc>, 
    dest=0xfeed8380 <std::bad_alloc::~bad_alloc()>)
    at /vol/gcc/src/hg/trunk/local/libstdc++-v3/libsupc++/eh_throw.cc:95
#8  0xfeedc257 in operator new (sz=1, al=(unknown: 1))
    at /vol/gcc/src/hg/trunk/local/libstdc++-v3/libsupc++/new_opa.cc:113
#9  0x0805104d in Test::Test (this=0x8047494, size=1, a=1)
    at /vol/gcc/src/hg/trunk/local/libstdc++-v3/testsuite/18_support/new_aligned.cc:29
#10 0x080511ad in test01 ()
    at /vol/gcc/src/hg/trunk/local/libstdc++-v3/testsuite/18_support/new_aligned.cc:64
#11 0x080519fe in main ()
    at /vol/gcc/src/hg/trunk/local/libstdc++-v3/testsuite/18_support/new_aligned.cc:118
Comment 1 Jonathan Wakely 2018-08-06 12:51:07 UTC
(In reply to Rainer Orth from comment #0)
> Thew new 18_support/new_aligned.cc test FAILs on Solaris 10 (sparc and x86),
> which lacks aligned_alloc in libc:

Ah good, I thought that testcase might shake out some more Solaris bugs :-)

Which implementation in libsupc++/new_opa.cc gets used? Is posix_memalign available? Or memalign?

I'm guessing it uses memalign, and Solaris memalign has an additional requirement that posix_memalign has, but GNU memalign doesn't:

  The value of alignment must be a power of two and must be greater than or
  equal to the size of a word.

So maybe this will fix it:

--- a/libstdc++-v3/libsupc++/new_opa.cc
+++ b/libstdc++-v3/libsupc++/new_opa.cc
@@ -53,7 +53,14 @@ aligned_alloc (std::size_t al, std::size_t sz)
 #else
 extern "C" void *memalign(std::size_t boundary, std::size_t size);
 #endif
-#define aligned_alloc memalign
+static inline void*
+aligned_alloc (std::size_t al, std::size_t sz)
+{
+  // Solaris requires that sz is greater than or equal to sizeof(int)
+  if (al < sizeof(int))
+    al = sizeof(int);
+  return memalign (al, sz);
+}
 #else
 #include <stdint.h>
 // The C library doesn't provide any aligned allocation functions, define one.
Comment 2 ro@CeBiTec.Uni-Bielefeld.DE 2018-08-06 21:46:38 UTC
> --- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
> (In reply to Rainer Orth from comment #0)
>> Thew new 18_support/new_aligned.cc test FAILs on Solaris 10 (sparc and x86),
>> which lacks aligned_alloc in libc:
>
> Ah good, I thought that testcase might shake out some more Solaris bugs :-)
>
> Which implementation in libsupc++/new_opa.cc gets used? Is posix_memalign
> available? Or memalign?

It's memalign indeed.  posix_memalign and aligned_alloc were introduced
at different points in the Solaris 11 release series.

> I'm guessing it uses memalign, and Solaris memalign has an additional
> requirement that posix_memalign has, but GNU memalign doesn't:
>
>   The value of alignment must be a power of two and must be greater than or
>   equal to the size of a word.

Indeed: this goes back as far as Solaris 2.5.1 and still exists in 11.5.

> So maybe this will fix it:
[...]
It did indeed: bootstrapped on i386-pc-solaris2.10 where

-FAIL: 18_support/new_aligned.cc execution test
-FAIL: 20_util/memory_resource/2.cc execution test

are now gone and i386-pc-solaris2.11 (unchanged).

Thanks.
	Rainer
Comment 3 Jonathan Wakely 2018-08-06 21:49:26 UTC
Great, thanks for testing it. Is it fixed for 64-bit as well as 32-bit? I was concerned that "the size of a word" might actually be imprecise and should be sizeof(void*) not sizeof(int).

I'll commit a slightly improved patch tomorrow.
Comment 4 ro@CeBiTec.Uni-Bielefeld.DE 2018-08-06 22:10:25 UTC
> --- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
> Great, thanks for testing it. Is it fixed for 64-bit as well as 32-bit? I was
> concerned that "the size of a word" might actually be imprecise and should be
> sizeof(void*) not sizeof(int).

It is.  In fact, I found the following comment in the OpenSolaris
sources of memalign.c, still in Illumos:

https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/libc/port/gen/memalign.c#L35

#define	_misaligned(p)		((unsigned)(p) & 3)
		/* 4-byte "word" alignment is considered ok in LP64 */
Comment 5 Jonathan Wakely 2018-08-06 22:48:42 UTC
Perfect, thanks.
Comment 6 Jonathan Wakely 2018-08-07 16:11:13 UTC
Author: redi
Date: Tue Aug  7 16:10:29 2018
New Revision: 263360

URL: https://gcc.gnu.org/viewcvs?rev=263360&root=gcc&view=rev
Log:
PR libstdc++/86861 Meet precondition for Solaris memalign

Solaris memalign requires alignment to be at least sizeof(int), so
increase it as needed.

Also move the check for valid alignments from the fallback
implementation of aligned_alloc into operator new, as it's required for
all of aligned_alloc, memalign, posix_memalign and __aligned_malloc.
This adds a branch to check for undefined behaviour which we could just
ignore, so the check could just be removed. It should certainly be
removed if PR 86878 is implemented to issue a warning about calls with
invalid alignments.

	PR libstdc++/86861
	* libsupc++/new_opa.cc [_GLIBCXX_HAVE_MEMALIGN] (aligned_alloc):
	Replace macro with inline function.
	[__sun]: Increase alignment to meet memalign precondition.
	[!HAVE__ALIGNED_MALLOC && !HAVE_POSIX_MEMALIGN && !HAVE_MEMALIGN]
	(aligned_alloc): Move check for valid alignment to operator new.
	Remove redundant check for non-zero size, it's enforced by the caller.
	(operator new): Move check for valid alignment here. Use
	__builtin_expect on check for zero size.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/libsupc++/new_opa.cc
Comment 7 Jonathan Wakely 2018-08-07 21:39:17 UTC
Author: redi
Date: Tue Aug  7 21:38:45 2018
New Revision: 263367

URL: https://gcc.gnu.org/viewcvs?rev=263367&root=gcc&view=rev
Log:
PR libstdc++/86861 Meet precondition for Solaris memalign

Solaris memalign requires alignment to be at least sizeof(int), so
increase it as needed.

Also move the check for valid alignments from the fallback
implementation of aligned_alloc into operator new, as it's required for
all of aligned_alloc, memalign, posix_memalign and __aligned_malloc.

Backport from mainline
2018-08-07  Jonathan Wakely  <jwakely@redhat.com>

	PR libstdc++/86861
	* libsupc++/new_opa.cc [_GLIBCXX_HAVE_MEMALIGN] (aligned_alloc):
	Replace macro with inline function.
	[__sun]: Increase alignment to meet memalign precondition.
	[!HAVE__ALIGNED_MALLOC && !HAVE_POSIX_MEMALIGN && !HAVE_MEMALIGN]
	(aligned_alloc): Move check for valid alignment to operator new.
	Remove redundant check for non-zero size, it's enforced by the caller.
	(operator new): Move check for valid alignment here. Use
	__builtin_expect on check for zero size.

Modified:
    branches/gcc-8-branch/libstdc++-v3/ChangeLog
    branches/gcc-8-branch/libstdc++-v3/libsupc++/new_opa.cc
Comment 8 Jonathan Wakely 2018-08-07 22:50:55 UTC
Author: redi
Date: Tue Aug  7 22:50:19 2018
New Revision: 263376

URL: https://gcc.gnu.org/viewcvs?rev=263376&root=gcc&view=rev
Log:
PR libstdc++/86861 Meet precondition for Solaris memalign

Solaris memalign requires alignment to be at least sizeof(int), so
increase it as needed.

Also move the check for valid alignments from the fallback
implementation of aligned_alloc into operator new, as it's required for
all of aligned_alloc, memalign, posix_memalign and __aligned_malloc.

Backport from mainline
2018-08-07  Jonathan Wakely  <jwakely@redhat.com>

	PR libstdc++/86861
	* libsupc++/new_opa.cc [_GLIBCXX_HAVE_MEMALIGN] (aligned_alloc):
	Replace macro with inline function.
	[__sun]: Increase alignment to meet memalign precondition.
	[!HAVE__ALIGNED_MALLOC && !HAVE_POSIX_MEMALIGN && !HAVE_MEMALIGN]
	(aligned_alloc): Move check for valid alignment to operator new.
	Remove redundant check for non-zero size, it's enforced by the caller.
	(operator new): Move check for valid alignment here. Use
	__builtin_expect on check for zero size.

Modified:
    branches/gcc-7-branch/libstdc++-v3/ChangeLog
    branches/gcc-7-branch/libstdc++-v3/libsupc++/new_opa.cc
Comment 9 Jonathan Wakely 2018-08-07 22:53:47 UTC
operator new(size_t, align_val_t) should work correctly for gcc 7.4 and 8.3 and trunk.