[PATCH, x86] Fix posix_memalign declaration in mm_malloc.h

[PATCH, x86] Fix posix_memalign declaration in mm_malloc.h

[Szabolcs Nagy]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

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.

Added a test case for the namespace issue.

OK for trunk?

gcc/ChangeLog:

2015-11-13  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	* config/i386/pmm_malloc.h (posix_memalign): Renamed to ...
	(_mm_posix_memalign): This.  Use posix_memalign as extern
	symbol only.

gcc/testsuite/ChangeLog:

2015-11-13  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	* g++.dg/other/mm_malloc.C: New.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: memalign2.diff --]
[-- Type: text/x-patch; name=memalign2.diff, Size: 1749 bytes --]

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" } } */

Re: [PATCH, x86] Fix posix_memalign declaration in mm_malloc.h

[Bernd Schmidt]

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

Re: [PATCH, x86] Fix posix_memalign declaration in mm_malloc.h

[Rich Felker]

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

Re: [PATCH, x86] Fix posix_memalign declaration in mm_malloc.h

[Marc Glisse]

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?

-- 
Marc Glisse

Re: [PATCH, x86] Fix posix_memalign declaration in mm_malloc.h

[Bernd Schmidt]

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

Re: [PATCH, x86] Fix posix_memalign declaration in mm_malloc.h

[Szabolcs Nagy]

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.)