Summary: | 18_support/new_aligned.cc FAILs | ||
---|---|---|---|
Product: | gcc | Reporter: | Rainer Orth <ro> |
Component: | libstdc++ | Assignee: | Jonathan Wakely <redi> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | redi, webrown.cpp |
Priority: | P3 | ||
Version: | 9.0 | ||
Target Milestone: | 7.4 | ||
Host: | *-*-solaris2.10 | Target: | *-*-solaris2.10 |
Build: | *-*-solaris2.10 | Known to work: | |
Known to fail: | Last reconfirmed: | 2018-08-06 00:00:00 |
Description
Rainer Orth
2018-08-05 14:26:56 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 #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 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 #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 */ Perfect, thanks. 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 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 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 operator new(size_t, align_val_t) should work correctly for gcc 7.4 and 8.3 and trunk. |