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

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

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

[Szabolcs Nagy]

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  

* 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  

* g++.dg/other/mm_malloc.C: New.
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 
 
 /* We can't depend on  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 (, alignment, size) == 0)
+  if (_mm_posix_memalign (, 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 000..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 
+
+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 (, alignment, size) == 0)
+  if (_mm_posix_memalign (, 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

[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 (, alignment, size) == 0)
+  if (_mm_posix_memalign (, 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

[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 (, alignment, size) == 0)
> >+  if (_mm_posix_memalign (, 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