| Message ID | 564619AE.4020108@arm.com |
|---|---|
| State | New |
| Headers | show |
On 11/13/2015 06:11 PM, Szabolcs Nagy wrote: > Followup to https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01433.html > > The posix_memalign declaration is incompatible with musl libc in C++, > because of the exception specification (matters with -std=c++11 > -pedantic-errors). It also pollutes the namespace and lacks protection > against a potential macro definition that is allowed by POSIX. The > fix avoids source level namespace pollution but retains the dependency > on the posix_memalign extern libc symbol. > #ifndef __cplusplus > -extern int posix_memalign (void **, size_t, size_t); > +extern int _mm_posix_memalign (void **, size_t, size_t) > #else > -extern "C" int posix_memalign (void **, size_t, size_t) throw (); > +extern "C" int _mm_posix_memalign (void **, size_t, size_t) throw () > #endif > +__asm__("posix_memalign"); Can't say I like the __asm__ after the #if/#else/#endif block. > static __inline void * > _mm_malloc (size_t size, size_t alignment) > @@ -42,7 +43,7 @@ _mm_malloc (size_t size, size_t alignment) > return malloc (size); > if (alignment == 2 || (sizeof (void *) == 8 && alignment == 4)) > alignment = sizeof (void *); > - if (posix_memalign (&ptr, alignment, size) == 0) > + if (_mm_posix_memalign (&ptr, alignment, size) == 0) > return ptr; > else > return NULL; Random observation: this seems overly conservative as malloc is defined to return something aligned to more than one byte. Couldn't this bug be fixed by either - just overallocating and aligning manually (eliminating the dependence entirely) - or moving _mm_malloc into libgcc.a? Bernd
On Fri, Nov 13, 2015 at 09:58:30PM +0100, Bernd Schmidt wrote: > On 11/13/2015 06:11 PM, Szabolcs Nagy wrote: > >Followup to https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01433.html > > > >The posix_memalign declaration is incompatible with musl libc in C++, > >because of the exception specification (matters with -std=c++11 > >-pedantic-errors). It also pollutes the namespace and lacks protection > >against a potential macro definition that is allowed by POSIX. The > >fix avoids source level namespace pollution but retains the dependency > >on the posix_memalign extern libc symbol. > > > #ifndef __cplusplus > >-extern int posix_memalign (void **, size_t, size_t); > >+extern int _mm_posix_memalign (void **, size_t, size_t) > > #else > >-extern "C" int posix_memalign (void **, size_t, size_t) throw (); > >+extern "C" int _mm_posix_memalign (void **, size_t, size_t) throw () > > #endif > >+__asm__("posix_memalign"); > > Can't say I like the __asm__ after the #if/#else/#endif block. It could trivially be moved inside. > > static __inline void * > > _mm_malloc (size_t size, size_t alignment) > >@@ -42,7 +43,7 @@ _mm_malloc (size_t size, size_t alignment) > > return malloc (size); > > if (alignment == 2 || (sizeof (void *) == 8 && alignment == 4)) > > alignment = sizeof (void *); > >- if (posix_memalign (&ptr, alignment, size) == 0) > >+ if (_mm_posix_memalign (&ptr, alignment, size) == 0) > > return ptr; > > else > > return NULL; > > Random observation: this seems overly conservative as malloc is > defined to return something aligned to more than one byte. You can assume it returns memory aligned to _Alignof(max_align_t), nothing more. But on some broken library implementations (windows?) you might not even get that. > Couldn't this bug be fixed by either > - just overallocating and aligning manually (eliminating the dependence > entirely) I don't think so; then the memory is not freeable, at least not without extra hacks. > - or moving _mm_malloc into libgcc.a? I wouldn't oppose that, but it seems like a more invasive change than is necessary to fix this bug. Rich
On Fri, 13 Nov 2015, Bernd Schmidt wrote: > On 11/13/2015 06:11 PM, Szabolcs Nagy wrote: >> Followup to https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01433.html >> >> The posix_memalign declaration is incompatible with musl libc in C++, >> because of the exception specification (matters with -std=c++11 >> -pedantic-errors). It also pollutes the namespace and lacks protection >> against a potential macro definition that is allowed by POSIX. The >> fix avoids source level namespace pollution but retains the dependency >> on the posix_memalign extern libc symbol. > >> #ifndef __cplusplus >> -extern int posix_memalign (void **, size_t, size_t); >> +extern int _mm_posix_memalign (void **, size_t, size_t) >> #else >> -extern "C" int posix_memalign (void **, size_t, size_t) throw (); >> +extern "C" int _mm_posix_memalign (void **, size_t, size_t) throw () >> #endif >> +__asm__("posix_memalign"); > > Can't say I like the __asm__ after the #if/#else/#endif block. It might also cause trouble if some systems like to prepend an underscore, maybe? >> static __inline void * >> _mm_malloc (size_t size, size_t alignment) >> @@ -42,7 +43,7 @@ _mm_malloc (size_t size, size_t alignment) >> return malloc (size); >> if (alignment == 2 || (sizeof (void *) == 8 && alignment == 4)) >> alignment = sizeof (void *); >> - if (posix_memalign (&ptr, alignment, size) == 0) >> + if (_mm_posix_memalign (&ptr, alignment, size) == 0) >> return ptr; >> else >> return NULL; > > Random observation: this seems overly conservative as malloc is defined to > return something aligned to more than one byte. What do we assume in the compiler? MALLOC_ABI_ALIGNMENT seems to be be the default BITS_PER_WORD on x86 (or I grepped badly), which also seems quite conservative but not as much. > Couldn't this bug be fixed by either > - just overallocating and aligning manually (eliminating the dependence > entirely) gmm_malloc.h does that, the whole point of pmm_malloc.h is to avoid it. > - or moving _mm_malloc into libgcc.a?
On 11/13/2015 11:30 PM, Marc Glisse wrote: >>> +__asm__("posix_memalign"); >> >> Can't say I like the __asm__ after the #if/#else/#endif block. > > It might also cause trouble if some systems like to prepend an > underscore, maybe? Yeah, that's one of the things I had in mind when I suggested moving this code to libgcc.a instead. Referring to a library symbol in this way makes me nervous. Bernd
On 16/11/15 13:42, Bernd Schmidt wrote: > On 11/13/2015 11:30 PM, Marc Glisse wrote: >>>> +__asm__("posix_memalign"); >>> >>> Can't say I like the __asm__ after the #if/#else/#endif block. >> >> It might also cause trouble if some systems like to prepend an >> underscore, maybe? > > Yeah, that's one of the things I had in mind when I suggested moving this code to libgcc.a instead. Referring > to a library symbol in this way makes me nervous. > an alternative is to leave posix_memalign declaration there for c as is, but remove it for c++. (the namespace issue i think is mostly relevant for c and even there it should not cause problems in practice, g++ defines _GNU_SOURCE so stdlib.h does not provide a clean namespace anyway. but the incompatible exception specifier can easily break in c++ with -pedantic-errors, and removing the declaration should work in practice because _GNU_SOURCE makes posix_memalign visible in stdlib.h.)
diff --git a/gcc/config/i386/pmm_malloc.h b/gcc/config/i386/pmm_malloc.h index 901001b..0696c20 100644 --- a/gcc/config/i386/pmm_malloc.h +++ b/gcc/config/i386/pmm_malloc.h @@ -27,12 +27,13 @@ #include <stdlib.h> /* We can't depend on <stdlib.h> since the prototype of posix_memalign - may not be visible. */ + may not be visible and we can't pollute the namespace either. */ #ifndef __cplusplus -extern int posix_memalign (void **, size_t, size_t); +extern int _mm_posix_memalign (void **, size_t, size_t) #else -extern "C" int posix_memalign (void **, size_t, size_t) throw (); +extern "C" int _mm_posix_memalign (void **, size_t, size_t) throw () #endif +__asm__("posix_memalign"); static __inline void * _mm_malloc (size_t size, size_t alignment) @@ -42,7 +43,7 @@ _mm_malloc (size_t size, size_t alignment) return malloc (size); if (alignment == 2 || (sizeof (void *) == 8 && alignment == 4)) alignment = sizeof (void *); - if (posix_memalign (&ptr, alignment, size) == 0) + if (_mm_posix_memalign (&ptr, alignment, size) == 0) return ptr; else return NULL; diff --git a/gcc/testsuite/g++.dg/other/mm_malloc.C b/gcc/testsuite/g++.dg/other/mm_malloc.C new file mode 100644 index 0000000..00582cc --- /dev/null +++ b/gcc/testsuite/g++.dg/other/mm_malloc.C @@ -0,0 +1,17 @@ +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ +/* { dg-options "-std=c++11" } */ + +/* Suppress POSIX declarations in libc headers in standard C++ mode. */ +#undef _GNU_SOURCE + +#define posix_memalign user_can_do_this + +#include <mm_malloc.h> + +void * +foo () +{ + return _mm_malloc (16, 16); +} + +/* { dg-final { scan-assembler "call\\tposix_memalign" } } */