* Re: [PATCH, PowerPC] Fix PR57949 (ABI alignment issue) [not found] <1376494321.17852.17.camel@oc8801110288.ibm.com> @ 2013-09-11 11:38 ` Alan Modra 2013-09-11 11:55 ` Jakub Jelinek 2013-09-11 12:56 ` Bill Schmidt 0 siblings, 2 replies; 7+ messages in thread From: Alan Modra @ 2013-09-11 11:38 UTC (permalink / raw) To: gcc-patches, libffi-discuss; +Cc: Bill Schmidt, dje 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. 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. */ -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH, PowerPC] Fix PR57949 (ABI alignment issue) 2013-09-11 11:38 ` [PATCH, PowerPC] Fix PR57949 (ABI alignment issue) Alan Modra @ 2013-09-11 11:55 ` Jakub Jelinek 2013-09-11 12:56 ` Bill Schmidt 1 sibling, 0 replies; 7+ messages in thread From: Jakub Jelinek @ 2013-09-11 11:55 UTC (permalink / raw) To: gcc-patches, libffi-discuss, Bill Schmidt, dje 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH, PowerPC] Fix PR57949 (ABI alignment issue) 2013-09-11 11:38 ` [PATCH, PowerPC] Fix PR57949 (ABI alignment issue) Alan Modra 2013-09-11 11:55 ` Jakub Jelinek @ 2013-09-11 12:56 ` Bill Schmidt 2013-09-11 17:03 ` Jay 2013-09-12 2:11 ` Alan Modra 1 sibling, 2 replies; 7+ messages in thread From: Bill Schmidt @ 2013-09-11 12:56 UTC (permalink / raw) To: Alan Modra; +Cc: gcc-patches, libffi-discuss, dje 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. */ > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH, PowerPC] Fix PR57949 (ABI alignment issue) 2013-09-11 12:56 ` Bill Schmidt @ 2013-09-11 17:03 ` Jay 2013-09-12 2:11 ` Alan Modra 1 sibling, 0 replies; 7+ messages in thread From: Jay @ 2013-09-11 17:03 UTC (permalink / raw) To: Bill Schmidt Cc: Alan Modra, <gcc-patches@gcc.gnu.org>, <libffi-discuss@sourceware.org>, <dje@gcc.gnu.org> 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. */ > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH, PowerPC] Fix PR57949 (ABI alignment issue) 2013-09-11 12:56 ` Bill Schmidt 2013-09-11 17:03 ` Jay @ 2013-09-12 2:11 ` Alan Modra 2013-09-12 8:33 ` Andrew Haley 1 sibling, 1 reply; 7+ messages in thread From: Alan Modra @ 2013-09-12 2:11 UTC (permalink / raw) To: Bill Schmidt; +Cc: gcc-patches, libffi-discuss, dje On Wed, Sep 11, 2013 at 07:55:43AM -0500, Bill Schmidt 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? We have precedent for compiling libffi based on gcc preprocessor defines, eg. __NO_FPRS__, so here's a way of making upstream libffi compatible with the various versions of gcc out there. I've taken the condition under which we align aggregates from rs6000_function_arg_boundary, and defined a macro with a value of the maximum alignment. Bootstrapped and regression tested powerpc64-linux. OK for mainline? gcc/ * config/rs6000/rs6000-c.c (rs6000_cpu_cpp_builtins): Define __STRUCT_PARM_ALIGN__. libffi/ * src/powerpc/ffi.c (ffi_prep_args64): Align FFI_TYPE_STRUCT. (ffi_closure_helper_LINUX64): Likewise. Index: gcc/config/rs6000/rs6000-c.c =================================================================== --- gcc/config/rs6000/rs6000-c.c (revision 202428) +++ gcc/config/rs6000/rs6000-c.c (working copy) @@ -473,6 +473,12 @@ rs6000_cpu_cpp_builtins (cpp_reader *pfile) if (TARGET_SOFT_FLOAT || !TARGET_FPRS) builtin_define ("__NO_FPRS__"); + /* Whether aggregates passed by value are aligned to a 16 byte boundary + if their alignment is 16 bytes or larger. */ + if ((TARGET_MACHO && rs6000_darwin64_abi) + || (DEFAULT_ABI == ABI_AIX && !rs6000_compat_align_parm)) + builtin_define ("__STRUCT_PARM_ALIGN__=16"); + /* Generate defines for Xilinx FPU. */ if (rs6000_xilinx_fpu) { Index: libffi/src/powerpc/ffi.c =================================================================== --- libffi/src/powerpc/ffi.c (revision 202428) +++ libffi/src/powerpc/ffi.c (working copy) @@ -462,6 +462,9 @@ ffi_prep_args64 (extended_cif *ecif, unsigned long double **d; } p_argv; unsigned long gprvalue; +#ifdef __STRUCT_PARM_ALIGN__ + unsigned long align; +#endif stacktop.c = (char *) stack + bytes; gpr_base.ul = stacktop.ul - ASM_NEEDS_REGISTERS64 - NUM_GPR_ARG_REGISTERS64; @@ -532,6 +535,12 @@ ffi_prep_args64 (extended_cif *ecif, unsigned long #endif case FFI_TYPE_STRUCT: +#ifdef __STRUCT_PARM_ALIGN__ + align = (*ptr)->alignment; + if (align > __STRUCT_PARM_ALIGN__) + align = __STRUCT_PARM_ALIGN__; + next_arg.ul = ALIGN (next_arg.ul, align); +#endif words = ((*ptr)->size + 7) / 8; if (next_arg.ul >= gpr_base.ul && next_arg.ul + words > gpr_end.ul) { @@ -1349,6 +1358,9 @@ ffi_closure_helper_LINUX64 (ffi_closure *closure, long i, avn; ffi_cif *cif; ffi_dblfl *end_pfr = pfr + NUM_FPR_ARG_REGISTERS64; +#ifdef __STRUCT_PARM_ALIGN__ + unsigned long align; +#endif cif = closure->cif; avalue = alloca (cif->nargs * sizeof (void *)); @@ -1399,6 +1411,12 @@ ffi_closure_helper_LINUX64 (ffi_closure *closure, break; case FFI_TYPE_STRUCT: +#ifdef __STRUCT_PARM_ALIGN__ + align = arg_types[i]->alignment; + if (align > __STRUCT_PARM_ALIGN__) + align = __STRUCT_PARM_ALIGN__; + pst = ALIGN (pst, align); +#endif #ifndef __LITTLE_ENDIAN__ /* Structures with size less than eight bytes are passed left-padded. */ -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH, PowerPC] Fix PR57949 (ABI alignment issue) 2013-09-12 2:11 ` Alan Modra @ 2013-09-12 8:33 ` Andrew Haley 2013-09-16 23:38 ` Alan Modra 0 siblings, 1 reply; 7+ messages in thread From: Andrew Haley @ 2013-09-12 8:33 UTC (permalink / raw) To: Bill Schmidt, gcc-patches, libffi-discuss, dje On 09/12/2013 03:11 AM, Alan Modra wrote: > We have precedent for compiling libffi based on gcc preprocessor > defines, eg. __NO_FPRS__, so here's a way of making upstream libffi > compatible with the various versions of gcc out there. I've taken the > condition under which we align aggregates from > rs6000_function_arg_boundary, and defined a macro with a value of the > maximum alignment. I would have thought that a runtime or configure-time test in libffi would be better. Andrew. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH, PowerPC] Fix PR57949 (ABI alignment issue) 2013-09-12 8:33 ` Andrew Haley @ 2013-09-16 23:38 ` Alan Modra 0 siblings, 0 replies; 7+ messages in thread From: Alan Modra @ 2013-09-16 23:38 UTC (permalink / raw) To: Andrew Haley; +Cc: Bill Schmidt, gcc-patches, libffi-discuss, dje On Thu, Sep 12, 2013 at 09:33:05AM +0100, Andrew Haley wrote: > On 09/12/2013 03:11 AM, Alan Modra wrote: > > We have precedent for compiling libffi based on gcc preprocessor > > defines, eg. __NO_FPRS__, so here's a way of making upstream libffi > > compatible with the various versions of gcc out there. I've taken the > > condition under which we align aggregates from > > rs6000_function_arg_boundary, and defined a macro with a value of the > > maximum alignment. > > I would have thought that a runtime or configure-time test in libffi > would be better. I don't see how a runtime check can be done, and a configure test just adds another layer of indirection. How is testing a cpp macro at configure time better than testing the same at compile time? -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-09-16 23:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1376494321.17852.17.camel@oc8801110288.ibm.com>
2013-09-11 11:38 ` [PATCH, PowerPC] Fix PR57949 (ABI alignment issue) Alan Modra
2013-09-11 11:55 ` Jakub Jelinek
2013-09-11 12:56 ` Bill Schmidt
2013-09-11 17:03 ` Jay
2013-09-12 2:11 ` Alan Modra
2013-09-12 8:33 ` Andrew Haley
2013-09-16 23:38 ` Alan Modra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).