Re: [PATCH, PowerPC] Fix PR57949 (ABI alignment issue)

[Jay <jay.krell@cornell.edu>]

Isn't mixing and matching and mismatching somewhat inevitable? Libffi & gcc don't always come along with each other? One must never change the ABI?

 - Jay

On Sep 11, 2013, at 5:55 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:

> On Wed, 2013-09-11 at 21:08 +0930, Alan Modra wrote:
>> On Wed, Aug 14, 2013 at 10:32:01AM -0500, Bill Schmidt wrote:
>>> This fixes a long-standing problem with GCC's implementation of the
>>> PPC64 ELF ABI.  If a structure contains a member requiring 128-bit
>>> alignment, and that structure is passed as a parameter, the parameter
>>> currently receives only 64-bit alignment.  This is an error, and is
>>> incompatible with correct code generated by the IBM XL compilers.
>> 
>> This caused multiple failures in the libffi testsuite:
>> libffi.call/cls_align_longdouble.c
>> libffi.call/cls_align_longdouble_split.c
>> libffi.call/cls_align_longdouble_split2.c
>> libffi.call/nested_struct5.c
>> 
>> Fixed by making the same alignment adjustment in libffi to structures
>> passed by value.  Bill, I think your patch needs to go on all active
>> gcc branches as otherwise we'll need different versions of libffi for
>> the next gcc releases.
> 
> Hm, the libffi case is unfortunate. :(
> 
> The alternative is to leave libffi alone, and require code that calls
> these interfaces with "bad" structs passed by value to be built using
> -mcompat-align-parm, which was provided for such compatibility issues.
> Hopefully there is a small number of cases where this can happen, and
> this could be documented with libffi and gcc.  What do you think?
> 
> Thanks,
> Bill
> 
>> 
>> The following was bootstrapped and regression checked powerpc64-linux.
>> OK for mainline, and the 4.7 and 4.8 branches when/if Bill's patch
>> goes in there?
>> 
>>    * src/powerpc/ffi.c (ffi_prep_args64): Align FFI_TYPE_STRUCT.
>>    (ffi_closure_helper_LINUX64): Likewise.
>> 
>> Index: libffi/src/powerpc/ffi.c
>> ===================================================================
>> --- libffi/src/powerpc/ffi.c    (revision 202428)
>> +++ libffi/src/powerpc/ffi.c    (working copy)
>> @@ -462,6 +462,7 @@ ffi_prep_args64 (extended_cif *ecif, unsigned long
>>     double **d;
>>   } p_argv;
>>   unsigned long gprvalue;
>> +  unsigned long align;
>> 
>>   stacktop.c = (char *) stack + bytes;
>>   gpr_base.ul = stacktop.ul - ASM_NEEDS_REGISTERS64 - NUM_GPR_ARG_REGISTERS64;
>> @@ -532,6 +533,10 @@ ffi_prep_args64 (extended_cif *ecif, unsigned long
>> #endif
>> 
>>    case FFI_TYPE_STRUCT:
>> +      align = (*ptr)->alignment;
>> +      if (align > 16)
>> +        align = 16;
>> +      next_arg.ul = ALIGN (next_arg.ul, align);
>>      words = ((*ptr)->size + 7) / 8;
>>      if (next_arg.ul >= gpr_base.ul && next_arg.ul + words > gpr_end.ul)
>>        {
>> @@ -1349,6 +1354,7 @@ ffi_closure_helper_LINUX64 (ffi_closure *closure,
>>   long i, avn;
>>   ffi_cif *cif;
>>   ffi_dblfl *end_pfr = pfr + NUM_FPR_ARG_REGISTERS64;
>> +  unsigned long align;
>> 
>>   cif = closure->cif;
>>   avalue = alloca (cif->nargs * sizeof (void *));
>> @@ -1399,6 +1405,10 @@ ffi_closure_helper_LINUX64 (ffi_closure *closure,
>>      break;
>> 
>>    case FFI_TYPE_STRUCT:
>> +      align = arg_types[i]->alignment;
>> +      if (align > 16)
>> +        align = 16;
>> +      pst = ALIGN (pst, align);
>> #ifndef __LITTLE_ENDIAN__
>>      /* Structures with size less than eight bytes are passed
>>         left-padded.  */
>