[PowerPC] Fix PR57949 (ABI alignment issue)

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.

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.

On Wed, Sep 11, 2013 at 09:08:46PM +0930, Alan Modra wrote:
> 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?

IMHO ABI changing patches shouldn't be backported to release branches.

	Jakub

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.  */
> 
>

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.  */
>