On 25/04/17 11:00, Jakub Jelinek wrote: > Hi! > > As mentioned in the PR, r225465 aka PR65956 changed the ABI > on ARM to match updated AAPCS, but the change had a bug - for structures > it considered DECL_ALIGN of any TYPE_FIELDS, rather than just > actual data components (AAPCS says members, for C++ and Itanium C++ ABI > that is likely direct non-static data members and non-virtual base classes; > that means it also considered alignment of static data members (at least > this was consistent ABI difference), or DECL_ALIGN of TYPE_DECLs (which is > bigger problem, because that alignment is pretty randomish, it has different > value in types in templates depending on whether they have been instantiated > earlier or not)). > > The following patch fixes the ABI bug and adds -Wpsabi diagnostics (inform > rather than warning, so it doesn't break with -Werror and matches i386.c > -Wpsabi notes where there is no bug on the compiled code side). > > Earlier version of the patch has been bootstrapped/regtested on > armv7hl-linux-gnueabi, but there have been various changes since then. > Ok for trunk/7.1 if it passes testing? > > 2017-04-25 Ramana Radhakrishnan <ramana.radhakrish...@arm.com> > Jakub Jelinek <ja...@redhat.com> > > PR target/77728 > * config/arm/arm.c: Include gimple.h. > (aapcs_layout_arg): Emit -Wpsabi note if arm_needs_doubleword_align > returns negative, increment ncrn only if it returned positive. > (arm_needs_doubleword_align): Return int instead of bool, > ignore DECL_ALIGN of non-FIELD_DECL TYPE_FIELDS chain > members, but if there is any such non-FIELD_DECL > > PARM_BOUNDARY aligned decl, return -1 instead of false. > (arm_function_arg): Emit -Wpsabi note if arm_needs_doubleword_align > returns negative, increment nregs only if it returned positive. > (arm_setup_incoming_varargs): Likewise. > (arm_function_arg_boundary): Emit -Wpsabi note if > arm_needs_doubleword_align returns negative, return > DOUBLEWORD_ALIGNMENT only if it returned positive. > testsuite/ > * g++.dg/abi/pr77728-1.C: New test.
This is ok if it passes testing. R. > > --- gcc/config/arm/arm.c.jj 2017-04-25 09:20:49.740670794 +0200 > +++ gcc/config/arm/arm.c 2017-04-25 11:07:11.003121070 +0200 > @@ -64,6 +64,7 @@ > #include "rtl-iter.h" > #include "optabs-libfuncs.h" > #include "gimplify.h" > +#include "gimple.h" > > /* This file should be included last. */ > #include "target-def.h" > @@ -81,7 +82,7 @@ struct four_ints > > /* Forward function declarations. */ > static bool arm_const_not_ok_for_debug_p (rtx); > -static bool arm_needs_doubleword_align (machine_mode, const_tree); > +static int arm_needs_doubleword_align (machine_mode, const_tree); > static int arm_compute_static_chain_stack_bytes (void); > static arm_stack_offsets *arm_get_frame_offsets (void); > static void arm_add_gc_roots (void); > @@ -6349,8 +6350,20 @@ aapcs_layout_arg (CUMULATIVE_ARGS *pcum, > /* C3 - For double-word aligned arguments, round the NCRN up to the > next even number. */ > ncrn = pcum->aapcs_ncrn; > - if ((ncrn & 1) && arm_needs_doubleword_align (mode, type)) > - ncrn++; > + if (ncrn & 1) > + { > + int res = arm_needs_doubleword_align (mode, type); > + /* Only warn during RTL expansion of call stmts, otherwise we would > + warn e.g. during gimplification even on functions that will be > + always inlined, and we'd warn multiple times. Don't warn when > + called in expand_function_start either, as we warn instead in > + arm_function_arg_boundary in that case. */ > + if (res < 0 && warn_psabi && currently_expanding_gimple_stmt) > + inform (input_location, "parameter passing for argument of type " > + "%qT changed in GCC 7.1", type); > + else if (res > 0) > + ncrn++; > + } > > nregs = ARM_NUM_REGS2(mode, type); > > @@ -6455,12 +6468,16 @@ arm_init_cumulative_args (CUMULATIVE_ARG > } > } > > -/* Return true if mode/type need doubleword alignment. */ > -static bool > +/* Return 1 if double word alignment is required for argument passing. > + Return -1 if double word alignment used to be required for argument > + passing before PR77728 ABI fix, but is not required anymore. > + Return 0 if double word alignment is not required and wasn't requried > + before either. */ > +static int > arm_needs_doubleword_align (machine_mode mode, const_tree type) > { > if (!type) > - return PARM_BOUNDARY < GET_MODE_ALIGNMENT (mode); > + return GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY; > > /* Scalar and vector types: Use natural alignment, i.e. of base type. */ > if (!AGGREGATE_TYPE_P (type)) > @@ -6470,12 +6487,21 @@ arm_needs_doubleword_align (machine_mode > if (TREE_CODE (type) == ARRAY_TYPE) > return TYPE_ALIGN (TREE_TYPE (type)) > PARM_BOUNDARY; > > + int ret = 0; > /* Record/aggregate types: Use greatest member alignment of any member. > */ > for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field)) > if (DECL_ALIGN (field) > PARM_BOUNDARY) > - return true; > + { > + if (TREE_CODE (field) == FIELD_DECL) > + return 1; > + else > + /* Before PR77728 fix, we were incorrectly considering also > + other aggregate fields, like VAR_DECLs, TYPE_DECLs etc. > + Make sure we can warn about that with -Wpsabi. */ > + ret = -1; > + } > > - return false; > + return ret; > } > > > @@ -6532,10 +6558,15 @@ arm_function_arg (cumulative_args_t pcum > } > > /* Put doubleword aligned quantities in even register pairs. */ > - if (pcum->nregs & 1 > - && ARM_DOUBLEWORD_ALIGN > - && arm_needs_doubleword_align (mode, type)) > - pcum->nregs++; > + if ((pcum->nregs & 1) && ARM_DOUBLEWORD_ALIGN) > + { > + int res = arm_needs_doubleword_align (mode, type); > + if (res < 0 && warn_psabi) > + inform (input_location, "parameter passing for argument of type " > + "%qT changed in GCC 7.1", type); > + else if (res > 0) > + pcum->nregs++; > + } > > /* Only allow splitting an arg between regs and memory if all preceding > args were allocated to regs. For args passed by reference we only count > @@ -6554,9 +6585,15 @@ arm_function_arg (cumulative_args_t pcum > static unsigned int > arm_function_arg_boundary (machine_mode mode, const_tree type) > { > - return (ARM_DOUBLEWORD_ALIGN && arm_needs_doubleword_align (mode, type) > - ? DOUBLEWORD_ALIGNMENT > - : PARM_BOUNDARY); > + if (!ARM_DOUBLEWORD_ALIGN) > + return PARM_BOUNDARY; > + > + int res = arm_needs_doubleword_align (mode, type); > + if (res < 0 && warn_psabi) > + inform (input_location, "parameter passing for argument of type %qT " > + "changed in GCC 7.1", type); > + > + return res > 0 ? DOUBLEWORD_ALIGNMENT : PARM_BOUNDARY; > } > > static int > @@ -26516,8 +26553,15 @@ arm_setup_incoming_varargs (cumulative_a > if (pcum->pcs_variant <= ARM_PCS_AAPCS_LOCAL) > { > nregs = pcum->aapcs_ncrn; > - if ((nregs & 1) && arm_needs_doubleword_align (mode, type)) > - nregs++; > + if (nregs & 1) > + { > + int res = arm_needs_doubleword_align (mode, type); > + if (res < 0 && warn_psabi) > + inform (input_location, "parameter passing for argument of " > + "type %qT changed in GCC 7.1", type); > + else if (res > 0) > + nregs++; > + } > } > else > nregs = pcum->nregs; > --- gcc/testsuite/g++.dg/abi/pr77728-1.C.jj 2017-04-25 09:30:39.262786365 > +0200 > +++ gcc/testsuite/g++.dg/abi/pr77728-1.C 2017-04-25 10:14:59.134254239 > +0200 > @@ -0,0 +1,171 @@ > +// { dg-do compile { target arm_eabi } } > +// { dg-options "-Wpsabi" } > + > +#include <stdarg.h> > + > +template <int N> > +struct A { double p; }; > + > +A<0> v; > + > +template <int N> > +struct B > +{ > + typedef A<N> T; > + int i, j; > +}; > + > +struct C : public B<0> {}; > +struct D {}; > +struct E : public D, C {}; > +struct F : public B<1> {}; > +struct G : public F { static double y; }; > +struct H : public G {}; > +struct I : public D { long long z; }; > +struct J : public D { static double z; int i, j; }; > + > +template <int N> > +struct K : public D { typedef A<N> T; int i, j; }; > + > +struct L { static double h; int i, j; }; > + > +int > +fn1 (int a, B<0> b) // { dg-message "note: parameter passing for argument > of type \[^\n\r]* changed in GCC 7\.1" } > +{ > + return a + b.i; > +} > + > +int > +fn2 (int a, B<1> b) > +{ > + return a + b.i; > +} > + > +int > +fn3 (int a, L b) // { dg-message "note: parameter passing for argument > of type \[^\n\r]* changed in GCC 7\.1" } > +{ > + return a + b.i; > +} > + > +int > +fn4 (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, > int k, int l, int m, B<0> n, ...) > +// { dg-message "note: parameter passing for argument of type \[^\n\r]* > changed in GCC 7\.1" "" { target *-*-* } .-1 } > +{ > + va_list ap; > + va_start (ap, n); > + int x = va_arg (ap, int); > + va_end (ap); > + return x; > +} > + > +int > +fn5 (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, > int k, int l, int m, B<1> n, ...) > +{ > + va_list ap; > + va_start (ap, n); > + int x = va_arg (ap, int); > + va_end (ap); > + return x; > +} > + > +int > +fn6 (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, > int k, int l, int m, C n, ...) > +{ > + va_list ap; > + va_start (ap, n); > + int x = va_arg (ap, int); > + va_end (ap); > + return x; > +} > + > +int > +fn7 (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, > int k, int l, int m, E n, ...) > +{ > + va_list ap; > + va_start (ap, n); > + int x = va_arg (ap, int); > + va_end (ap); > + return x; > +} > + > +int > +fn8 (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, > int k, int l, int m, H n, ...) > +{ > + va_list ap; > + va_start (ap, n); > + int x = va_arg (ap, int); > + va_end (ap); > + return x; > +} > + > +int > +fn9 (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, > int k, int l, int m, I n, ...) > +{ > + va_list ap; > + va_start (ap, n); > + int x = va_arg (ap, int); > + va_end (ap); > + return x; > +} > + > +int > +fn10 (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, > int k, int l, int m, J n, ...) > +// { dg-message "note: parameter passing for argument of type \[^\n\r]* > changed in GCC 7\.1" "" { target *-*-* } .-1 } > +{ > + va_list ap; > + va_start (ap, n); > + int x = va_arg (ap, int); > + va_end (ap); > + return x; > +} > + > +int > +fn11 (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, > int k, int l, int m, K<0> n, ...) > +// { dg-message "note: parameter passing for argument of type \[^\n\r]* > changed in GCC 7\.1" "" { target *-*-* } .-1 } > +{ > + va_list ap; > + va_start (ap, n); > + int x = va_arg (ap, int); > + va_end (ap); > + return x; > +} > + > +int > +fn12 (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, > int k, int l, int m, K<2> n, ...) > +{ > + va_list ap; > + va_start (ap, n); > + int x = va_arg (ap, int); > + va_end (ap); > + return x; > +} > + > +void > +test () > +{ > + static B<0> b0; > + static B<1> b1; > + static L l; > + static C c; > + static E e; > + static H h; > + static I i; > + static J j; > + static K<0> k0; > + static K<2> k2; > + fn1 (1, b0); // { dg-message "note: parameter passing for argument > of type \[^\n\r]* changed in GCC 7\.1" } > + fn2 (1, b1); > + fn3 (1, l); // { dg-message "note: parameter passing for argument > of type \[^\n\r]* changed in GCC 7\.1" } > + fn4 (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, b0, 1, 2, 3, 4); > + // { dg-message "note: parameter passing for argument of type \[^\n\r]* > changed in GCC 7\.1" "" { target *-*-* } .-1 } > + fn5 (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, b1, 1, 2, 3, 4); > + fn6 (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, c, 1, 2, 3, 4); > + fn7 (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, e, 1, 2, 3, 4); > + fn8 (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, h, 1, 2, 3, 4); > + fn9 (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, i, 1, 2, 3, 4); > + fn10 (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, j, 1, 2, 3, 4); > + // { dg-message "note: parameter passing for argument of type \[^\n\r]* > changed in GCC 7\.1" "" { target *-*-* } .-1 } > + fn11 (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, k0, 1, 2, 3, 4); > + // { dg-message "note: parameter passing for argument of type \[^\n\r]* > changed in GCC 7\.1" "" { target *-*-* } .-1 } > + fn12 (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, k2, 1, 2, 3, 4); > +} > > Jakub >