| Message ID | 20180808085238.28201-1-sebastian.huber@embedded-brains.de |
|---|---|
| State | New |
| Headers | show |
| Series | libstdc++-v3: Have aligned_alloc() on Newlib | expand |
On 08/08/18 10:52 +0200, Sebastian Huber wrote: >While building for Newlib, some configure checks must be hard coded. >The aligned_alloc() is supported since 2015 in Newlib. > >libstdc++-v3 > > PR target/85904 > * configure.ac): Define HAVE_ALIGNED_ALLOC if building for There's a stray closing parenthesis here. > Newlib. > * configure: Regnerate. Typo "Regnerate". But the patch itself is fine - OK for trunk. I'm ambivalent about this being backported to gcc-7 and gcc-8 branches (gcc-6 is unaffected as it doesn't use aligned_alloc). It's strictly speaking an ABI change, because HAVE_ALIGNED_ALLOC affects the memory layout for allocations from operator new(size_t, align_val_t) (in new_opa.cc) which needs to agree with the corresponding operator delete (in del_opa.cc). Using static linking it might be possible to create a binary that has operator new using aligned_alloc, but operator delete expecting to do ((void**)ptr)[-1], which would be bad. Those operators are C++17, so "experimental", but maybe we shouldn't make the change on release branches.
On 08/08/18 13:18, Jonathan Wakely wrote: > On 08/08/18 10:52 +0200, Sebastian Huber wrote: >> While building for Newlib, some configure checks must be hard coded. >> The aligned_alloc() is supported since 2015 in Newlib. >> >> libstdc++-v3 >> >> PR target/85904 >> * configure.ac): Define HAVE_ALIGNED_ALLOC if building for > > There's a stray closing parenthesis here. > >> Newlib. >> * configure: Regnerate. > > Typo "Regnerate". > > But the patch itself is fine - OK for trunk. Oh, sorry, for all the typos. > > I'm ambivalent about this being backported to gcc-7 and gcc-8 branches > (gcc-6 is unaffected as it doesn't use aligned_alloc). > > It's strictly speaking an ABI change, because HAVE_ALIGNED_ALLOC > affects the memory layout for allocations from operator new(size_t, > align_val_t) (in new_opa.cc) which needs to agree with the > corresponding operator delete (in del_opa.cc). Using static linking it > might be possible to create a binary that has operator new using > aligned_alloc, but operator delete expecting to do ((void**)ptr)[-1], > which would be bad. > > Those operators are C++17, so "experimental", but maybe we shouldn't > make the change on release branches. > I was able to build a GCC 7.3.0 compiler, but now the build fails on the GCC 7 branch.
On 08/08/18 15:57 +0200, Ulrich Weigand wrote: >Jonathan Wakely wrote: >> On 08/08/18 10:52 +0200, Sebastian Huber wrote: >> >While building for Newlib, some configure checks must be hard coded. >> >The aligned_alloc() is supported since 2015 in Newlib. >> > >> >libstdc++-v3 >> > >> > PR target/85904 >> > * configure.ac): Define HAVE_ALIGNED_ALLOC if building for >> >> There's a stray closing parenthesis here. >> >> > Newlib. >> > * configure: Regnerate. >> >> Typo "Regnerate". >> >> But the patch itself is fine - OK for trunk. >> >> I'm ambivalent about this being backported to gcc-7 and gcc-8 branches >> (gcc-6 is unaffected as it doesn't use aligned_alloc). >> >> It's strictly speaking an ABI change, because HAVE_ALIGNED_ALLOC >> affects the memory layout for allocations from operator new(size_t, >> align_val_t) (in new_opa.cc) which needs to agree with the >> corresponding operator delete (in del_opa.cc). Using static linking it >> might be possible to create a binary that has operator new using >> aligned_alloc, but operator delete expecting to do ((void**)ptr)[-1], >> which would be bad. >> >> Those operators are C++17, so "experimental", but maybe we shouldn't >> make the change on release branches. > >The way it is now I'm getting build failures on new SPU target >(which is newlib based): > >/home/uweigand/dailybuild/spu-tc-2018-08-07/gcc-head/src/libstdc++-v3/libsupc++/new_opa.cc:58:1: error: 'void* aligned_alloc(std::size_t, std::size_t)' was declared 'extern' and later 'static' [-fpermissive] > aligned_alloc (std::size_t al, std::size_t sz) > ^~~~~~~~~~~~~ >In file included from /home/uweigand/dailybuild/spu-tc-2018-08-07/gcc-build/spu/libstdc++-v3/include/cstdlib:75:62, > from /home/uweigand/dailybuild/spu-tc-2018-08-07/gcc-build/spu/libstdc++-v3/include/stdlib.h:36, > from /home/uweigand/dailybuild/spu-tc-2018-08-07/gcc-head/src/libstdc++-v3/libsupc++/new_opa.cc:27: >/home/uweigand/dailybuild/spu-tc-2018-08-07/spu-toolchain/spu/include/stdlib.h:328:8: note: previous declaration of 'void* aligned_alloc(size_t, size_t)' > void * aligned_alloc(size_t, size_t) __malloc_like __alloc_align(1) > ^~~~~~~~~~~~~ > >This seems to be because configure is hard-coded to assume aligned_alloc >is not available, but then the new_opc.cc file tries to define its own >version, conflicting with the newlib prototype that is actually there. > >So one way or the other this needs to be fixed ... But that's on trunk, right? Sebastian is about to fix that. I expect the same problem exists on the branches too though. I'm surprised nobody has reported this until now. It's been like that since r244933 in Jan 2017. Maybe we should put the fallback implementation into a namespace to avoid the same problem on other targets where HAVE_ALIGNED_ALLOC isn't set correctly.
On 08/08/18 16:01, Jonathan Wakely wrote: > On 08/08/18 15:57 +0200, Ulrich Weigand wrote: >> Jonathan Wakely wrote: >>> On 08/08/18 10:52 +0200, Sebastian Huber wrote: >>> >While building for Newlib, some configure checks must be hard coded. >>> >The aligned_alloc() is supported since 2015 in Newlib. >>> > >>> >libstdc++-v3 >>> > >>> > PR target/85904 >>> > * configure.ac): Define HAVE_ALIGNED_ALLOC if building for >>> >>> There's a stray closing parenthesis here. >>> >>> > Newlib. >>> > * configure: Regnerate. >>> >>> Typo "Regnerate". >>> >>> But the patch itself is fine - OK for trunk. >>> >>> I'm ambivalent about this being backported to gcc-7 and gcc-8 branches >>> (gcc-6 is unaffected as it doesn't use aligned_alloc). >>> >>> It's strictly speaking an ABI change, because HAVE_ALIGNED_ALLOC >>> affects the memory layout for allocations from operator new(size_t, >>> align_val_t) (in new_opa.cc) which needs to agree with the >>> corresponding operator delete (in del_opa.cc). Using static linking it >>> might be possible to create a binary that has operator new using >>> aligned_alloc, but operator delete expecting to do ((void**)ptr)[-1], >>> which would be bad. >>> >>> Those operators are C++17, so "experimental", but maybe we shouldn't >>> make the change on release branches. >> >> The way it is now I'm getting build failures on new SPU target >> (which is newlib based): >> >> /home/uweigand/dailybuild/spu-tc-2018-08-07/gcc-head/src/libstdc++-v3/libsupc++/new_opa.cc:58:1: >> error: 'void* aligned_alloc(std::size_t, std::size_t)' was declared >> 'extern' and later 'static' [-fpermissive] >> aligned_alloc (std::size_t al, std::size_t sz) >> ^~~~~~~~~~~~~ >> In file included from >> /home/uweigand/dailybuild/spu-tc-2018-08-07/gcc-build/spu/libstdc++-v3/include/cstdlib:75:62, >> from >> /home/uweigand/dailybuild/spu-tc-2018-08-07/gcc-build/spu/libstdc++-v3/include/stdlib.h:36, >> from >> /home/uweigand/dailybuild/spu-tc-2018-08-07/gcc-head/src/libstdc++-v3/libsupc++/new_opa.cc:27: >> /home/uweigand/dailybuild/spu-tc-2018-08-07/spu-toolchain/spu/include/stdlib.h:328:8: >> note: previous declaration of 'void* aligned_alloc(size_t, size_t)' >> void * aligned_alloc(size_t, size_t) __malloc_like __alloc_align(1) >> ^~~~~~~~~~~~~ >> >> This seems to be because configure is hard-coded to assume aligned_alloc >> is not available, but then the new_opc.cc file tries to define its own >> version, conflicting with the newlib prototype that is actually there. >> >> So one way or the other this needs to be fixed ... > > But that's on trunk, right? Sebastian is about to fix that. > > I expect the same problem exists on the branches too though. > > I'm surprised nobody has reported this until now. It's been like that > since r244933 in Jan 2017. I notices this problem today with a build of the GCC 7 branch. I am able to build a GCC 7.3.0.
On 08/08/18 16:04 +0200, Sebastian Huber wrote: >On 08/08/18 16:01, Jonathan Wakely wrote: >>On 08/08/18 15:57 +0200, Ulrich Weigand wrote: >>>Jonathan Wakely wrote: >>>>On 08/08/18 10:52 +0200, Sebastian Huber wrote: >>>>>While building for Newlib, some configure checks must be hard coded. >>>>>The aligned_alloc() is supported since 2015 in Newlib. >>>>> >>>>>libstdc++-v3 >>>>> >>>>> PR target/85904 >>>>> * configure.ac): Define HAVE_ALIGNED_ALLOC if building for >>>> >>>>There's a stray closing parenthesis here. >>>> >>>>> Newlib. >>>>> * configure: Regnerate. >>>> >>>>Typo "Regnerate". >>>> >>>>But the patch itself is fine - OK for trunk. >>>> >>>>I'm ambivalent about this being backported to gcc-7 and gcc-8 branches >>>>(gcc-6 is unaffected as it doesn't use aligned_alloc). >>>> >>>>It's strictly speaking an ABI change, because HAVE_ALIGNED_ALLOC >>>>affects the memory layout for allocations from operator new(size_t, >>>>align_val_t) (in new_opa.cc) which needs to agree with the >>>>corresponding operator delete (in del_opa.cc). Using static linking it >>>>might be possible to create a binary that has operator new using >>>>aligned_alloc, but operator delete expecting to do ((void**)ptr)[-1], >>>>which would be bad. >>>> >>>>Those operators are C++17, so "experimental", but maybe we shouldn't >>>>make the change on release branches. >>> >>>The way it is now I'm getting build failures on new SPU target >>>(which is newlib based): >>> >>>/home/uweigand/dailybuild/spu-tc-2018-08-07/gcc-head/src/libstdc++-v3/libsupc++/new_opa.cc:58:1: >>>error: 'void* aligned_alloc(std::size_t, std::size_t)' was >>>declared 'extern' and later 'static' [-fpermissive] >>>aligned_alloc (std::size_t al, std::size_t sz) >>>^~~~~~~~~~~~~ >>>In file included from /home/uweigand/dailybuild/spu-tc-2018-08-07/gcc-build/spu/libstdc++-v3/include/cstdlib:75:62, >>> from /home/uweigand/dailybuild/spu-tc-2018-08-07/gcc-build/spu/libstdc++-v3/include/stdlib.h:36, >>> from /home/uweigand/dailybuild/spu-tc-2018-08-07/gcc-head/src/libstdc++-v3/libsupc++/new_opa.cc:27: >>>/home/uweigand/dailybuild/spu-tc-2018-08-07/spu-toolchain/spu/include/stdlib.h:328:8: >>>note: previous declaration of 'void* aligned_alloc(size_t, >>>size_t)' >>>void * aligned_alloc(size_t, size_t) __malloc_like __alloc_align(1) >>> ^~~~~~~~~~~~~ >>> >>>This seems to be because configure is hard-coded to assume aligned_alloc >>>is not available, but then the new_opc.cc file tries to define its own >>>version, conflicting with the newlib prototype that is actually there. >>> >>>So one way or the other this needs to be fixed ... >> >>But that's on trunk, right? Sebastian is about to fix that. >> >>I expect the same problem exists on the branches too though. >> >>I'm surprised nobody has reported this until now. It's been like that >>since r244933 in Jan 2017. > >I notices this problem today with a build of the GCC 7 branch. I am >able to build a GCC 7.3.0. Aha, so newlib was using memalign previously: @@ -53,20 +54,24 @@ 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 -#else +static inline void* +aligned_alloc (std::size_t al, std::size_t sz) +{ +#ifdef __sun + // Solaris 10 memalign requires that alignment is greater than or equal to + // the size of a word. + if (al < sizeof(int)) + al = sizeof(int); +#endif + return memalign (al, sz); +} +#else // !HAVE__ALIGNED_MALLOC && !HAVE_POSIX_MEMALIGN && !HAVE_MEMALIGN #include <stdint.h> // The C library doesn't provide any aligned allocation functions, define one. // This is a modified version of code from gcc/config/i386/gmm_malloc.h OK, I've regressed the branches then - I'll fix that.
On 08/08/18 16:22 +0200, Ulrich Weigand wrote: >Jonathan Wakely wrote: > >> Aha, so newlib was using memalign previously: >> >> @@ -53,20 +54,24 @@ 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 > >Yes, exactly ... this commit introduced the regression. > >> OK, I've regressed the branches then - I'll fix that. This should fix it. I'll finish testing and commit it. Sebastian, your patch to define HAVE_ALIGNED_ALLOC is OK for gcc-7-branch and gcc-8-branch, because changing newlib from using memalign to aligned_alloc is safe. Sorry for the mess.
On 08/08/18 15:33 +0100, Jonathan Wakely wrote: >On 08/08/18 16:22 +0200, Ulrich Weigand wrote: >>Jonathan Wakely wrote: >> >>>Aha, so newlib was using memalign previously: >>> >>>@@ -53,20 +54,24 @@ 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 >> >>Yes, exactly ... this commit introduced the regression. >> >>>OK, I've regressed the branches then - I'll fix that. > >This should fix it. I'll finish testing and commit it. > >Sebastian, your patch to define HAVE_ALIGNED_ALLOC is OK for >gcc-7-branch and gcc-8-branch, because changing newlib from using >memalign to aligned_alloc is safe. With the patch this time ... commit cb2a7aae2668b690cfaed5093e0107e0ee64bb0e Author: Jonathan Wakely <jwakely@redhat.com> Date: Wed Aug 8 15:28:48 2018 +0100 Prevent internal aligned_alloc clashing with libc version If configure fails to detect aligned_alloc we will try to define our own in new_opa.cc but that could clash with the libcversion in <stdlib.h>. Use a namespace to keep them distinct. * libsupc++/new_opa.cc (aligned_alloc): Declare inside namespace to avoid clashing with an ::aligned_alloc function that was not detected by configure. diff --git a/libstdc++-v3/libsupc++/new_opa.cc b/libstdc++-v3/libsupc++/new_opa.cc index 5be0cc2ca65..68eac5b8ceb 100644 --- a/libstdc++-v3/libsupc++/new_opa.cc +++ b/libstdc++-v3/libsupc++/new_opa.cc @@ -25,15 +25,30 @@ #include <bits/c++config.h> #include <stdlib.h> +#include <stdint.h> #include <bits/exception_defines.h> #include "new" +#if !_GLIBCXX_HAVE_ALIGNED_ALLOC && !_GLIBCXX_HAVE__ALIGNED_MALLOC \ + && !_GLIBCXX_HAVE_POSIX_MEMALIGN && _GLIBCXX_HAVE_MEMALIGN +# if _GLIBCXX_HOSTED && __has_include(<malloc.h>) +// Some C libraries declare memalign in <malloc.h> +# include <malloc.h> +# else +extern "C" void *memalign(std::size_t boundary, std::size_t size); +# endif +#endif + using std::new_handler; using std::bad_alloc; -#if !_GLIBCXX_HAVE_ALIGNED_ALLOC -#if _GLIBCXX_HAVE__ALIGNED_MALLOC -#define aligned_alloc(al,sz) _aligned_malloc(sz,al) +namespace __gnu_cxx { +#if _GLIBCXX_HAVE_ALIGNED_ALLOC +using ::aligned_alloc; +#elif _GLIBCXX_HAVE__ALIGNED_MALLOC +static inline void* +aligned_alloc (std::size_t al, std::size_t sz) +{ return _aligned_malloc(sz, al); } #elif _GLIBCXX_HAVE_POSIX_MEMALIGN static inline void* aligned_alloc (std::size_t al, std::size_t sz) @@ -49,11 +64,6 @@ aligned_alloc (std::size_t al, std::size_t sz) return nullptr; } #elif _GLIBCXX_HAVE_MEMALIGN -#if _GLIBCXX_HOSTED -#include <malloc.h> -#else -extern "C" void *memalign(std::size_t boundary, std::size_t size); -#endif static inline void* aligned_alloc (std::size_t al, std::size_t sz) { @@ -66,7 +76,6 @@ aligned_alloc (std::size_t al, std::size_t sz) return memalign (al, sz); } #else // !HAVE__ALIGNED_MALLOC && !HAVE_POSIX_MEMALIGN && !HAVE_MEMALIGN -#include <stdint.h> // The C library doesn't provide any aligned allocation functions, define one. // This is a modified version of code from gcc/config/i386/gmm_malloc.h static inline void* @@ -87,7 +96,7 @@ aligned_alloc (std::size_t al, std::size_t sz) return aligned_ptr; } #endif -#endif +} // namespace __gnu_cxx _GLIBCXX_WEAK_DEFINITION void * operator new (std::size_t sz, std::align_val_t al) @@ -116,6 +125,7 @@ operator new (std::size_t sz, std::align_val_t al) sz += align - rem; #endif + using __gnu_cxx::aligned_alloc; while (__builtin_expect ((p = aligned_alloc (align, sz)) == 0, false)) { new_handler handler = std::get_new_handler ();
On 08/08/18 15:46 +0100, Jonathan Wakely wrote: >On 08/08/18 15:33 +0100, Jonathan Wakely wrote: >>On 08/08/18 16:22 +0200, Ulrich Weigand wrote: >>>Jonathan Wakely wrote: >>> >>>>Aha, so newlib was using memalign previously: >>>> >>>>@@ -53,20 +54,24 @@ 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 >>> >>>Yes, exactly ... this commit introduced the regression. >>> >>>>OK, I've regressed the branches then - I'll fix that. >> >>This should fix it. I'll finish testing and commit it. >> >>Sebastian, your patch to define HAVE_ALIGNED_ALLOC is OK for >>gcc-7-branch and gcc-8-branch, because changing newlib from using >>memalign to aligned_alloc is safe. > >With the patch this time ... Committed to trunk, gcc-8-branch and gcc-7-branch. Rainer, please let me know if this still works on Solaris 10, thanks. commit 4f61feff8ad3615c0b686deac3852d98734ef964 Author: redi <redi@138bc75d-0d04-0410-961f-82ee72b054a4> Date: Wed Aug 8 15:16:43 2018 +0000 Prevent internal aligned_alloc clashing with libc version If configure fails to detect aligned_alloc we will try to define our own in new_opa.cc but that could clash with the libcversion in <stdlib.h>. Use a namespace to keep them distinct. * libsupc++/new_opa.cc (aligned_alloc): Declare inside namespace to avoid clashing with an ::aligned_alloc function that was not detected by configure. git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@263409 138bc75d-0d04-0410-961f-82ee72b054a4 diff --git a/libstdc++-v3/libsupc++/new_opa.cc b/libstdc++-v3/libsupc++/new_opa.cc index 5be0cc2ca65..68eac5b8ceb 100644 --- a/libstdc++-v3/libsupc++/new_opa.cc +++ b/libstdc++-v3/libsupc++/new_opa.cc @@ -25,15 +25,30 @@ #include <bits/c++config.h> #include <stdlib.h> +#include <stdint.h> #include <bits/exception_defines.h> #include "new" +#if !_GLIBCXX_HAVE_ALIGNED_ALLOC && !_GLIBCXX_HAVE__ALIGNED_MALLOC \ + && !_GLIBCXX_HAVE_POSIX_MEMALIGN && _GLIBCXX_HAVE_MEMALIGN +# if _GLIBCXX_HOSTED && __has_include(<malloc.h>) +// Some C libraries declare memalign in <malloc.h> +# include <malloc.h> +# else +extern "C" void *memalign(std::size_t boundary, std::size_t size); +# endif +#endif + using std::new_handler; using std::bad_alloc; -#if !_GLIBCXX_HAVE_ALIGNED_ALLOC -#if _GLIBCXX_HAVE__ALIGNED_MALLOC -#define aligned_alloc(al,sz) _aligned_malloc(sz,al) +namespace __gnu_cxx { +#if _GLIBCXX_HAVE_ALIGNED_ALLOC +using ::aligned_alloc; +#elif _GLIBCXX_HAVE__ALIGNED_MALLOC +static inline void* +aligned_alloc (std::size_t al, std::size_t sz) +{ return _aligned_malloc(sz, al); } #elif _GLIBCXX_HAVE_POSIX_MEMALIGN static inline void* aligned_alloc (std::size_t al, std::size_t sz) @@ -49,11 +64,6 @@ aligned_alloc (std::size_t al, std::size_t sz) return nullptr; } #elif _GLIBCXX_HAVE_MEMALIGN -#if _GLIBCXX_HOSTED -#include <malloc.h> -#else -extern "C" void *memalign(std::size_t boundary, std::size_t size); -#endif static inline void* aligned_alloc (std::size_t al, std::size_t sz) { @@ -66,7 +76,6 @@ aligned_alloc (std::size_t al, std::size_t sz) return memalign (al, sz); } #else // !HAVE__ALIGNED_MALLOC && !HAVE_POSIX_MEMALIGN && !HAVE_MEMALIGN -#include <stdint.h> // The C library doesn't provide any aligned allocation functions, define one. // This is a modified version of code from gcc/config/i386/gmm_malloc.h static inline void* @@ -87,7 +96,7 @@ aligned_alloc (std::size_t al, std::size_t sz) return aligned_ptr; } #endif -#endif +} // namespace __gnu_cxx _GLIBCXX_WEAK_DEFINITION void * operator new (std::size_t sz, std::align_val_t al) @@ -116,6 +125,7 @@ operator new (std::size_t sz, std::align_val_t al) sz += align - rem; #endif + using __gnu_cxx::aligned_alloc; while (__builtin_expect ((p = aligned_alloc (align, sz)) == 0, false)) { new_handler handler = std::get_new_handler ();
On 08/08/18 16:33, Jonathan Wakely wrote: > On 08/08/18 16:22 +0200, Ulrich Weigand wrote: >> Jonathan Wakely wrote: >> >>> Aha, so newlib was using memalign previously: >>> >>> @@ -53,20 +54,24 @@ 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 >> >> Yes, exactly ... this commit introduced the regression. >> >>> OK, I've regressed the branches then - I'll fix that. > > This should fix it. I'll finish testing and commit it. > > Sebastian, your patch to define HAVE_ALIGNED_ALLOC is OK for > gcc-7-branch and gcc-8-branch, because changing newlib from using > memalign to aligned_alloc is safe. Should I check in my patch in addition to your patch?
On 09/08/18 06:56 +0200, Sebastian Huber wrote: >On 08/08/18 16:33, Jonathan Wakely wrote: >>On 08/08/18 16:22 +0200, Ulrich Weigand wrote: >>>Jonathan Wakely wrote: >>> >>>>Aha, so newlib was using memalign previously: >>>> >>>>@@ -53,20 +54,24 @@ 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 >>> >>>Yes, exactly ... this commit introduced the regression. >>> >>>>OK, I've regressed the branches then - I'll fix that. >> >>This should fix it. I'll finish testing and commit it. >> >>Sebastian, your patch to define HAVE_ALIGNED_ALLOC is OK for >>gcc-7-branch and gcc-8-branch, because changing newlib from using >>memalign to aligned_alloc is safe. > >Should I check in my patch in addition to your patch? Yes please, on trunk and 7 and 8. It's better to use the standard aligned_alloc if available.
Hi Jonathan, > Committed to trunk, gcc-8-branch and gcc-7-branch. > > Rainer, please let me know if this still works on Solaris 10, thanks. I ran i386-pc-solaris2.1[01] and sparc-sun-solaris2.1[01] bootstraps last night: no regressions. Thanks. Rainer
On 09/08/18 10:08, Jonathan Wakely wrote: > On 09/08/18 06:56 +0200, Sebastian Huber wrote: >> On 08/08/18 16:33, Jonathan Wakely wrote: >>> On 08/08/18 16:22 +0200, Ulrich Weigand wrote: >>>> Jonathan Wakely wrote: >>>> >>>>> Aha, so newlib was using memalign previously: >>>>> >>>>> @@ -53,20 +54,24 @@ 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 >>>> >>>> Yes, exactly ... this commit introduced the regression. >>>> >>>>> OK, I've regressed the branches then - I'll fix that. >>> >>> This should fix it. I'll finish testing and commit it. >>> >>> Sebastian, your patch to define HAVE_ALIGNED_ALLOC is OK for >>> gcc-7-branch and gcc-8-branch, because changing newlib from using >>> memalign to aligned_alloc is safe. >> >> Should I check in my patch in addition to your patch? > > Yes please, on trunk and 7 and 8. It's better to use the standard > aligned_alloc if available. > but the newlib aligned_alloc is broken on baremetal targets, it is implemented using posix_memalign which is not provided by the newlib malloc implementation (except on cygwin) in libstdc++ test log i see spawn /B/gcc/xg++ -shared-libgcc -B/B/gcc -nostdinc++ -L/B/aarch64-none-elf/libstdc++-v3/src -L/B/aarch64-none-elf/libstdc++-v3/src/.libs -L/B/aarch64-none-elf/libstdc++-v3/libsupc++/.libs -B/P/aarch64-none-elf/bin/ -B/P/aarch64-none-elf/lib/ -isystem /P/aarch64-none-elf/include -isystem /P/aarch64-none-elf/sys-include -B/B/aarch64-none-elf/./libstdc++-v3/src/.libs -fmessage-length=0 -fno-show-column -ffunction-sections -fdata-sections -g -ffunction-sections -fdata-sections -O2 -DLOCALEDIR="." -nostdinc++ -I/B/aarch64-none-elf/libstdc++-v3/include/aarch64-none-elf -I/B/aarch64-none-elf/libstdc++-v3/include -I/S/gcc/libstdc++-v3/libsupc++ -I/S/gcc/libstdc++-v3/include/backward -I/S/gcc/libstdc++-v3/testsuite/util /S/gcc/libstdc++-v3/testsuite/18_support/aligned_alloc/aligned_alloc.cc -std=gnu++17 -fno-diagnostics-show-caret -fdiagnostics-color=never ./libtestc++.a -L/B/aarch64-none-elf/libstdc++-v3/src/filesystem/.libs -specs=aem-ve.specs -lm -mcmodel=small -o ./aligned_alloc.exe /P/aarch64-none-elf/bin/ld: /P/aarch64-none-elf/lib/libg.a(lib_a-aligned_alloc.o): in function `aligned_alloc': /S/newlib-cygwin/newlib/libc/stdlib/aligned_alloc.c:35: undefined reference to `posix_memalign' /P/aarch64-none-elf/bin/ld: /S/newlib-cygwin/newlib/libc/stdlib/aligned_alloc.c:35:(.text.aligned_alloc+0x14): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `posix_memalign' collect2: error: ld returned 1 exit status FAIL: 18_support/aligned_alloc/aligned_alloc.cc (test for excess errors) FAIL: 18_support/new_aligned.cc (test for excess errors) FAIL: 20_util/allocator/overaligned.cc (test for excess errors) FAIL: 20_util/memory_resource/2.cc (test for excess errors) FAIL: 20_util/monotonic_buffer_resource/allocate.cc (test for excess errors) FAIL: 20_util/monotonic_buffer_resource/deallocate.cc (test for excess errors) FAIL: 20_util/monotonic_buffer_resource/release.cc (test for excess errors) FAIL: 20_util/monotonic_buffer_resource/upstream_resource.cc (test for excess errors) FAIL: 20_util/polymorphic_allocator/resource.cc (test for excess errors) FAIL: 20_util/polymorphic_allocator/select.cc (test for excess errors) FAIL: ext/bitmap_allocator/overaligned.cc (test for excess errors) FAIL: ext/mt_allocator/overaligned.cc (test for excess errors) FAIL: ext/new_allocator/overaligned.cc (test for excess errors) FAIL: ext/pool_allocator/overaligned.cc (test for excess errors) FAIL: g++.dg/cpp1z/aligned-new1.C (test for excess errors) FAIL: g++.dg/cpp1z/aligned-new2.C (test for excess errors) FAIL: g++.dg/cpp1z/aligned-new5.C -std=gnu++11 (test for excess errors) FAIL: g++.dg/cpp1z/aligned-new5.C -std=gnu++14 (test for excess errors) FAIL: g++.dg/cpp1z/aligned-new5.C -std=gnu++98 (test for excess errors) FAIL: g++.dg/cpp1z/aligned-new6.C -std=gnu++11 (test for excess errors) FAIL: g++.dg/cpp1z/aligned-new6.C -std=gnu++14 (test for excess errors) FAIL: g++.dg/cpp1z/aligned-new8.C (test for excess errors) UNRESOLVED: 18_support/aligned_alloc/aligned_alloc.cc compilation failed to produce executable UNRESOLVED: 18_support/new_aligned.cc compilation failed to produce executable UNRESOLVED: 20_util/allocator/overaligned.cc compilation failed to produce executable UNRESOLVED: 20_util/memory_resource/2.cc compilation failed to produce executable UNRESOLVED: 20_util/monotonic_buffer_resource/allocate.cc compilation failed to produce executable UNRESOLVED: 20_util/monotonic_buffer_resource/deallocate.cc compilation failed to produce executable UNRESOLVED: 20_util/monotonic_buffer_resource/release.cc compilation failed to produce executable UNRESOLVED: 20_util/monotonic_buffer_resource/upstream_resource.cc compilation failed to produce executable UNRESOLVED: 20_util/polymorphic_allocator/resource.cc compilation failed to produce executable UNRESOLVED: 20_util/polymorphic_allocator/select.cc compilation failed to produce executable UNRESOLVED: ext/bitmap_allocator/overaligned.cc compilation failed to produce executable UNRESOLVED: ext/mt_allocator/overaligned.cc compilation failed to produce executable UNRESOLVED: ext/new_allocator/overaligned.cc compilation failed to produce executable UNRESOLVED: ext/pool_allocator/overaligned.cc compilation failed to produce executable UNRESOLVED: g++.dg/cpp1z/aligned-new1.C compilation failed to produce executable UNRESOLVED: g++.dg/cpp1z/aligned-new2.C compilation failed to produce executable UNRESOLVED: g++.dg/cpp1z/aligned-new5.C -std=gnu++11 compilation failed to produce executable UNRESOLVED: g++.dg/cpp1z/aligned-new5.C -std=gnu++14 compilation failed to produce executable UNRESOLVED: g++.dg/cpp1z/aligned-new5.C -std=gnu++98 compilation failed to produce executable UNRESOLVED: g++.dg/cpp1z/aligned-new6.C -std=gnu++11 compilation failed to produce executable UNRESOLVED: g++.dg/cpp1z/aligned-new6.C -std=gnu++14 compilation failed to produce executable UNRESOLVED: g++.dg/cpp1z/aligned-new8.C compilation failed to produce executable
On 13/08/18 12:55 +0100, Szabolcs Nagy wrote: >On 09/08/18 10:08, Jonathan Wakely wrote: >>On 09/08/18 06:56 +0200, Sebastian Huber wrote: >>>On 08/08/18 16:33, Jonathan Wakely wrote: >>>>On 08/08/18 16:22 +0200, Ulrich Weigand wrote: >>>>>Jonathan Wakely wrote: >>>>> >>>>>>Aha, so newlib was using memalign previously: >>>>>> >>>>>>@@ -53,20 +54,24 @@ 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 >>>>> >>>>>Yes, exactly ... this commit introduced the regression. >>>>> >>>>>>OK, I've regressed the branches then - I'll fix that. >>>> >>>>This should fix it. I'll finish testing and commit it. >>>> >>>>Sebastian, your patch to define HAVE_ALIGNED_ALLOC is OK for >>>>gcc-7-branch and gcc-8-branch, because changing newlib from using >>>>memalign to aligned_alloc is safe. >>> >>>Should I check in my patch in addition to your patch? >> >>Yes please, on trunk and 7 and 8. It's better to use the standard >>aligned_alloc if available. >> > >but the newlib aligned_alloc is broken on baremetal targets, >it is implemented using posix_memalign which is not provided >by the newlib malloc implementation (except on cygwin) Ouch, OK, let's revert it. Using memalign is fine. The original problem that I think Sebastian was trying to solve should be fixed by r263409 anyway (and was backported to all branches).
On 13/08/18 13:04 +0100, Jonathan Wakely wrote: >On 13/08/18 12:55 +0100, Szabolcs Nagy wrote: >>On 09/08/18 10:08, Jonathan Wakely wrote: >>>On 09/08/18 06:56 +0200, Sebastian Huber wrote: >>>>On 08/08/18 16:33, Jonathan Wakely wrote: >>>>>On 08/08/18 16:22 +0200, Ulrich Weigand wrote: >>>>>>Jonathan Wakely wrote: >>>>>> >>>>>>>Aha, so newlib was using memalign previously: >>>>>>> >>>>>>>@@ -53,20 +54,24 @@ 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 >>>>>> >>>>>>Yes, exactly ... this commit introduced the regression. >>>>>> >>>>>>>OK, I've regressed the branches then - I'll fix that. >>>>> >>>>>This should fix it. I'll finish testing and commit it. >>>>> >>>>>Sebastian, your patch to define HAVE_ALIGNED_ALLOC is OK for >>>>>gcc-7-branch and gcc-8-branch, because changing newlib from using >>>>>memalign to aligned_alloc is safe. >>>> >>>>Should I check in my patch in addition to your patch? >>> >>>Yes please, on trunk and 7 and 8. It's better to use the standard >>>aligned_alloc if available. >>> >> >>but the newlib aligned_alloc is broken on baremetal targets, >>it is implemented using posix_memalign which is not provided >>by the newlib malloc implementation (except on cygwin) > >Ouch, OK, let's revert it. Using memalign is fine. > >The original problem that I think Sebastian was trying to solve should >be fixed by r263409 anyway (and was backported to all branches). I've committed this to trunk and will do so on the branches too. commit 6fe6d2c2256a73bca9ca3754761bd724b654db13 Author: Jonathan Wakely <jwakely@redhat.com> Date: Mon Aug 13 14:05:46 2018 +0100 Revert "libstdc++-v3: Have aligned_alloc() on Newlib" This reverts commit r263461 / 2e920cd849b3cf0a72df4f172e27676a3e70b73f because aligned_alloc is not defined for baremetal newlib targets, see https://gcc.gnu.org/ml/libstdc++/2018-08/msg00065.html Revert 2018-08-10 Sebastian Huber <sebastian.huber@embedded-brains.de> PR target/85904 * configure.ac: Define HAVE_ALIGNED_ALLOC if building for Newlib. * configure: Regenerate. diff --git a/libstdc++-v3/configure.ac b/libstdc++-v3/configure.ac index e15228dde5e..332af3706d3 100644 --- a/libstdc++-v3/configure.ac +++ b/libstdc++-v3/configure.ac @@ -330,7 +330,6 @@ else AC_DEFINE(HAVE_TANF) AC_DEFINE(HAVE_TANHF) - AC_DEFINE(HAVE_ALIGNED_ALLOC) AC_DEFINE(HAVE_ICONV) AC_DEFINE(HAVE_MEMALIGN) else
On 13/08/18 19:57, Jonathan Wakely wrote: > On 13/08/18 13:04 +0100, Jonathan Wakely wrote: >> On 13/08/18 12:55 +0100, Szabolcs Nagy wrote: >>> On 09/08/18 10:08, Jonathan Wakely wrote: >>>> Yes please, on trunk and 7 and 8. It's better to use the standard >>>> aligned_alloc if available. >>>> >>> >>> but the newlib aligned_alloc is broken on baremetal targets, >>> it is implemented using posix_memalign which is not provided >>> by the newlib malloc implementation (except on cygwin) >> >> Ouch, OK, let's revert it. Using memalign is fine. >> >> The original problem that I think Sebastian was trying to solve should >> be fixed by r263409 anyway (and was backported to all branches). > > I've committed this to trunk and will do so on the branches too. > thanks, this fixed the issue for me on trunk.
diff --git a/libstdc++-v3/configure b/libstdc++-v3/configure index d33081d544c..53803b7e599 100755 --- a/libstdc++-v3/configure +++ b/libstdc++-v3/configure @@ -28967,6 +28967,8 @@ else $as_echo "#define HAVE_TANHF 1" >>confdefs.h + $as_echo "#define HAVE_ALIGNED_ALLOC 1" >>confdefs.h + $as_echo "#define HAVE_ICONV 1" >>confdefs.h $as_echo "#define HAVE_MEMALIGN 1" >>confdefs.h diff --git a/libstdc++-v3/configure.ac b/libstdc++-v3/configure.ac index 332af3706d3..e15228dde5e 100644 --- a/libstdc++-v3/configure.ac +++ b/libstdc++-v3/configure.ac @@ -330,6 +330,7 @@ else AC_DEFINE(HAVE_TANF) AC_DEFINE(HAVE_TANHF) + AC_DEFINE(HAVE_ALIGNED_ALLOC) AC_DEFINE(HAVE_ICONV) AC_DEFINE(HAVE_MEMALIGN) else