Message ID | 20130911113845.GF2643@bubble.grove.modra.org |
---|---|
State | New |
Headers | show |
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. */ >
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. */