Fix alignment of sem_t for AArch64 ILP32

Message ID mvmr3q2l1yr.fsf@hawking.suse.de
State New, archived
Headers

Commit Message

Andreas Schwab May 27, 2015, 11 a.m. UTC
  sem_t must have the same alignment as struct new_sem.

Andreas.

	* sysdeps/aarch64/nptl/bits/semaphore.h (sem_t) [!__LP64__]: Use
	long long for __align.
---
 sysdeps/aarch64/nptl/bits/semaphore.h | 4 ++++
 1 file changed, 4 insertions(+)
  

Comments

Chris Metcalf May 28, 2015, 7:32 p.m. UTC | #1
On 05/27/2015 07:00 AM, Andreas Schwab wrote:
> sem_t must have the same alignment as struct new_sem.
>
> Andreas.
>
> 	* sysdeps/aarch64/nptl/bits/semaphore.h (sem_t) [!__LP64__]: Use
> 	long long for __align.
> ---
>   sysdeps/aarch64/nptl/bits/semaphore.h | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/sysdeps/aarch64/nptl/bits/semaphore.h b/sysdeps/aarch64/nptl/bits/semaphore.h
> index 047dd4e..d9f4ab6 100644
> --- a/sysdeps/aarch64/nptl/bits/semaphore.h
> +++ b/sysdeps/aarch64/nptl/bits/semaphore.h
> @@ -31,5 +31,9 @@
>   typedef union
>   {
>     char __size[__SIZEOF_SEM_T];
> +#ifdef __LP64__
>     long int __align;
> +#else
> +  long long int __align;
> +#endif
>   } sem_t;

Why not unconditionally use "long long" as the type?  Or if you really 
prefer
the different types, just use "__INT64_TYPE__ __align" instead? Either way
seems like it makes the code a little more readable, though I'd prefer
the former (and it is more friendly to non-gcc compilers).

More generally, I'm not sure why we don't just use 
__attribute__((aligned(8)))
on a simple struct holding the __size[] array, rather than playing union 
games.
Obviously this still would have to be per-platform due to 
__HAVE_64B_ATOMICS,
but just explicitly specifying the alignment does seem cleaner.  I guess 
it is
arguably more friendly to non-gcc compilers using the public headers to
do it the way it has been done traditionally.

None of these suggestions change C++ type mangling as far as I can see,
so are presumably all safe from an ABI perspective.
  
Andrew Pinski May 29, 2015, 8:23 a.m. UTC | #2
On Fri, May 29, 2015 at 3:32 AM, Chris Metcalf <cmetcalf@ezchip.com> wrote:
> On 05/27/2015 07:00 AM, Andreas Schwab wrote:
>>
>> sem_t must have the same alignment as struct new_sem.
>>
>> Andreas.
>>
>>         * sysdeps/aarch64/nptl/bits/semaphore.h (sem_t) [!__LP64__]: Use
>>         long long for __align.
>> ---
>>   sysdeps/aarch64/nptl/bits/semaphore.h | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/sysdeps/aarch64/nptl/bits/semaphore.h
>> b/sysdeps/aarch64/nptl/bits/semaphore.h
>> index 047dd4e..d9f4ab6 100644
>> --- a/sysdeps/aarch64/nptl/bits/semaphore.h
>> +++ b/sysdeps/aarch64/nptl/bits/semaphore.h
>> @@ -31,5 +31,9 @@
>>   typedef union
>>   {
>>     char __size[__SIZEOF_SEM_T];
>> +#ifdef __LP64__
>>     long int __align;
>> +#else
>> +  long long int __align;
>> +#endif
>>   } sem_t;
>
>
> Why not unconditionally use "long long" as the type?  Or if you really
> prefer
> the different types, just use "__INT64_TYPE__ __align" instead? Either way
> seems like it makes the code a little more readable, though I'd prefer
> the former (and it is more friendly to non-gcc compilers).
>
> More generally, I'm not sure why we don't just use
> __attribute__((aligned(8)))
> on a simple struct holding the __size[] array, rather than playing union
> games.
> Obviously this still would have to be per-platform due to
> __HAVE_64B_ATOMICS,
> but just explicitly specifying the alignment does seem cleaner.  I guess it
> is
> arguably more friendly to non-gcc compilers using the public headers to
> do it the way it has been done traditionally.
>
> None of these suggestions change C++ type mangling as far as I can see,
> so are presumably all safe from an ABI perspective.

Or better yet use __kernel_signed_long type.

Thanks,
Andrew


>
> --
> Chris Metcalf, EZChip Semiconductor
> http://www.ezchip.com
>
  
Andreas Schwab June 8, 2015, 8:50 a.m. UTC | #3
Andrew Pinski <pinskia@gmail.com> writes:

> Or better yet use __kernel_signed_long type.

This has nothing to do with the kernel.  It's a pure user-space
requirement that sem_t must be 64-bit aligned.

Andreas.
  

Patch

diff --git a/sysdeps/aarch64/nptl/bits/semaphore.h b/sysdeps/aarch64/nptl/bits/semaphore.h
index 047dd4e..d9f4ab6 100644
--- a/sysdeps/aarch64/nptl/bits/semaphore.h
+++ b/sysdeps/aarch64/nptl/bits/semaphore.h
@@ -31,5 +31,9 @@ 
 typedef union
 {
   char __size[__SIZEOF_SEM_T];
+#ifdef __LP64__
   long int __align;
+#else
+  long long int __align;
+#endif
 } sem_t;