On 25/04/17 14:57, Kyrill Tkachov wrote:
Hi all,

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?

This patch passed bootstrap and testing on arm-none-linux-gnueabihf
configured with --with-arch=armv7-a --with-fpu=neon --with-float=hard 
--with-mode=thumb.


And the same with --with-mode=arm also passed bootstrap and is halfway through 
the testsuite
I don't expect it to show any problems at this point (a previous full bootstrap 
and test
with an earlier but functionally identical version of Ramana's patch didn't 
show any problems
in this configuration).

Thanks,
Kyrill

So the patch looks ok to me.
Ramana, did you have testing running in another configuration that you wanted 
to report on?

Thanks,
Kyrill

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.

--- 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


Reply via email to