Re: [PATCH] Fix alignment of sem_t for AArch64 ILP32

[Andrew Pinski <pinskia at gmail dot com>]

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
>