Re: [ARM ABI PATCH] Change ARM ABI to match AAPCS, provide -Wpsabi notes (PR target/77728)

["Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>]

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.radhakrishnan@arm.com>
> 	    Jakub Jelinek  <jakub@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
>