* [PATCH, x86] Fix posix_memalign declaration in mm_malloc.h
@ 2015-11-13 17:11 Szabolcs Nagy
2015-11-13 20:58 ` Bernd Schmidt
0 siblings, 1 reply; 6+ messages in thread
From: Szabolcs Nagy @ 2015-11-13 17:11 UTC (permalink / raw)
To: gcc-patches; +Cc: H.J. Lu, Rich Felker
[-- 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" } } */
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, x86] Fix posix_memalign declaration in mm_malloc.h
2015-11-13 17:11 [PATCH, x86] Fix posix_memalign declaration in mm_malloc.h Szabolcs Nagy
@ 2015-11-13 20:58 ` Bernd Schmidt
2015-11-13 22:29 ` Rich Felker
2015-11-13 22:30 ` Marc Glisse
0 siblings, 2 replies; 6+ messages in thread
From: Bernd Schmidt @ 2015-11-13 20:58 UTC (permalink / raw)
To: Szabolcs Nagy, gcc-patches; +Cc: H.J. Lu, Rich Felker
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, x86] Fix posix_memalign declaration in mm_malloc.h
2015-11-13 20:58 ` Bernd Schmidt
@ 2015-11-13 22:29 ` Rich Felker
2015-11-13 22:30 ` Marc Glisse
1 sibling, 0 replies; 6+ messages in thread
From: Rich Felker @ 2015-11-13 22:29 UTC (permalink / raw)
To: Bernd Schmidt; +Cc: Szabolcs Nagy, gcc-patches, H.J. Lu
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, x86] Fix posix_memalign declaration in mm_malloc.h
2015-11-13 20:58 ` Bernd Schmidt
2015-11-13 22:29 ` Rich Felker
@ 2015-11-13 22:30 ` Marc Glisse
2015-11-16 13:42 ` Bernd Schmidt
1 sibling, 1 reply; 6+ messages in thread
From: Marc Glisse @ 2015-11-13 22:30 UTC (permalink / raw)
To: Bernd Schmidt; +Cc: Szabolcs Nagy, gcc-patches, H.J. Lu, Rich Felker
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, x86] Fix posix_memalign declaration in mm_malloc.h
2015-11-13 22:30 ` Marc Glisse
@ 2015-11-16 13:42 ` Bernd Schmidt
2015-11-16 16:35 ` Szabolcs Nagy
0 siblings, 1 reply; 6+ messages in thread
From: Bernd Schmidt @ 2015-11-16 13:42 UTC (permalink / raw)
To: gcc-patches; +Cc: Szabolcs Nagy, H.J. Lu, Rich Felker
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, x86] Fix posix_memalign declaration in mm_malloc.h
2015-11-16 13:42 ` Bernd Schmidt
@ 2015-11-16 16:35 ` Szabolcs Nagy
0 siblings, 0 replies; 6+ messages in thread
From: Szabolcs Nagy @ 2015-11-16 16:35 UTC (permalink / raw)
To: Bernd Schmidt, gcc-patches; +Cc: H.J. Lu, Rich Felker
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.)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-11-16 16:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-13 17:11 [PATCH, x86] Fix posix_memalign declaration in mm_malloc.h Szabolcs Nagy
2015-11-13 20:58 ` Bernd Schmidt
2015-11-13 22:29 ` Rich Felker
2015-11-13 22:30 ` Marc Glisse
2015-11-16 13:42 ` Bernd Schmidt
2015-11-16 16:35 ` Szabolcs Nagy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).