Re: [arm][RFC] PR target/88469 fix incorrect argument passing with 64-bit bitfields

["Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 2494 bytes --]

After discussions with Jakub on IRC, I've committed this patch along
with a couple of other tweaks to the testsuite.  New ChangeLog below.

This issue has existed since GCC-6 but has only come to light following
the release of gcc-8 where code was added to the compiler sources that
exposed the bug.  I'm therefore reasonably confident that this idiom
won't be a widely hit problem.  I'm therefore looking to mitigate it in
the GCC-7/8 sources rather than try to back-port an ABI changing bug.
I'll post a patch for that shortly.

R.

On 17/12/2018 16:04, Richard Earnshaw (lists) wrote:
> Unfortunately another PCS bug has come to light with the layout of
> structs whose alignment is dominated by a 64-bit bitfield element.  Such
> fields in the type list appear to have alignment 1, but in reality, for
> the purposes of alignment of the underlying structure, the alignment is
> derived from the underlying bitfield's type.  We've been getting this
> wrong since support for over-aligned record types was added several
> releases back.  Worse still, the existing code may generate unaligned
> memory accesses that may fault on some versions of the architecture.
> 
> I'd appreciate a comment from someone with a bit more knowledge of
> record layout - the code in stor-layout.c looks hideously complex when
> it comes to bit-field layout; but it's quite possible that all of that
> is irrelevant on Arm.
> 
> PR target/88469
> 	* config/arm/arm.c (arm_needs_doubleword_align): Return 2 if a
> 	record's alignment is dominated by a bitfield with 64-bit
> 	aligned base type.
> 	(arm_function_arg): Emit a warning if the alignment has changed
> 	since earlier GCC releases.
> 	(arm_function_arg_boundary): Likewise.
> 	(arm_setup_incoming_varargs): Likewise.
> 	* testsuite/gcc.target/arm/aapcs/bitfield1.c: New test.
> 
> I'm not committing this yet, as I'll await comments as to whether folks
> think this is sufficient.
> 
> R.
> 

gcc:
	PR target/88469
	* config/arm/arm.c (arm_needs_doubleword_align): Return 2 if a record's
	alignment is dominated by a bitfield with 64-bit aligned base type.
	(arm_function_arg): Emit a warning if the alignment has changed since
	earlier GCC releases.
	(arm_function_arg_boundary): Likewise.
	(arm_setup_incoming_varargs): Likewise.

gcc/testsuite:
	* gcc.target/arm/aapcs/bitfield1.c: New test.
	* gcc.target/arm/aapcs/overalign_rec1.c: New test.
	* gcc.target/arm/aapcs/overalign_rec2.c: New test.
	* gcc.target/arm/aapcs/overalign_rec3.c: New test.

[-- Attachment #2: pcs-bitfield.patch --]
[-- Type: text/x-patch, Size: 6196 bytes --]

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 73cb8df9af1..c6fbda25e96 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -6598,7 +6598,9 @@ arm_init_cumulative_args (CUMULATIVE_ARGS *pcum, tree fntype,
     }
 }
 
-/* Return 1 if double word alignment is required for argument passing.
+/* Return 2 if double word alignment is required for argument passing,
+   but wasn't required before the fix for PR88469.
+   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
@@ -6618,7 +6620,8 @@ arm_needs_doubleword_align (machine_mode mode, const_tree type)
     return TYPE_ALIGN (TREE_TYPE (type)) > PARM_BOUNDARY;
 
   int ret = 0;
-  /* Record/aggregate types: Use greatest member alignment of any member.  */ 
+  int ret2 = 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)
       {
@@ -6630,6 +6633,13 @@ arm_needs_doubleword_align (machine_mode mode, const_tree type)
 	     Make sure we can warn about that with -Wpsabi.  */
 	  ret = -1;
       }
+    else if (TREE_CODE (field) == FIELD_DECL
+	     && DECL_BIT_FIELD (field)
+	     && TYPE_ALIGN (DECL_BIT_FIELD_TYPE (field)) > PARM_BOUNDARY)
+      ret2 = 1;
+
+  if (ret2)
+    return 2;
 
   return ret;
 }
@@ -6695,7 +6705,12 @@ arm_function_arg (cumulative_args_t pcum_v, machine_mode mode,
 	inform (input_location, "parameter passing for argument of type "
 		"%qT changed in GCC 7.1", type);
       else if (res > 0)
-	pcum->nregs++;
+	{
+	  pcum->nregs++;
+	  if (res > 1 && warn_psabi)
+	    inform (input_location, "parameter passing for argument of type "
+		    "%qT changed in GCC 9.1", type);
+	}
     }
 
   /* Only allow splitting an arg between regs and memory if all preceding
@@ -6722,6 +6737,9 @@ arm_function_arg_boundary (machine_mode mode, const_tree type)
   if (res < 0 && warn_psabi)
     inform (input_location, "parameter passing for argument of type %qT "
 	    "changed in GCC 7.1", type);
+  if (res > 1 && warn_psabi)
+    inform (input_location, "parameter passing for argument of type "
+	    "%qT changed in GCC 9.1", type);
 
   return res > 0 ? DOUBLEWORD_ALIGNMENT : PARM_BOUNDARY;
 }
@@ -26999,7 +27017,13 @@ arm_setup_incoming_varargs (cumulative_args_t pcum_v,
 	    inform (input_location, "parameter passing for argument of "
 		    "type %qT changed in GCC 7.1", type);
 	  else if (res > 0)
-	    nregs++;
+	    {
+	      nregs++;
+	      if (res > 1 && warn_psabi)
+		inform (input_location,
+			"parameter passing for argument of type "
+			"%qT changed in GCC 9.1", type);
+	    }
 	}
     }
   else
diff --git a/gcc/testsuite/gcc.target/arm/aapcs/bitfield1.c b/gcc/testsuite/gcc.target/arm/aapcs/bitfield1.c
new file mode 100644
index 00000000000..cac786eec37
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/aapcs/bitfield1.c
@@ -0,0 +1,24 @@
+/* Test AAPCS layout (alignment).  */
+
+/* { dg-do run { target arm_eabi } } */
+/* { dg-require-effective-target arm32 } */
+/* { dg-options "-O" } */
+
+#ifndef IN_FRAMEWORK
+#define TESTFILE "bitfield1.c"
+
+struct bf
+{
+  unsigned long long a: 61;
+  unsigned b: 3;
+} v = {1, 1};
+
+#include "abitest.h"
+#else
+  ARG (int, 7, R0)
+  ARG (int, 9, R1)
+  ARG (int, 11, R2)
+  /* Alignment of the bitfield type should affect alignment of the overall
+     type, so R3 not used.  */
+  LAST_ARG (struct bf, v, STACK)
+#endif
diff --git a/gcc/testsuite/gcc.target/arm/aapcs/overalign_rec1.c b/gcc/testsuite/gcc.target/arm/aapcs/overalign_rec1.c
new file mode 100644
index 00000000000..1d33da42bdf
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/aapcs/overalign_rec1.c
@@ -0,0 +1,27 @@
+/* Test AAPCS layout (alignment).  */
+
+/* { dg-do run { target arm_eabi } } */
+/* { dg-require-effective-target arm32 } */
+/* { dg-options "-O" } */
+
+#ifndef IN_FRAMEWORK
+#define TESTFILE "overalign_rec1.c"
+
+typedef struct __attribute__((aligned(8)))
+{
+  int a;
+  int b;
+} overaligned;
+
+overaligned v = {1, 3};
+overaligned w = {33, 99};
+
+#include "abitest.h"
+#else
+  ARG (int, 7, R0)
+  /* Overalignment is ignored for the purposes of parameter passing.  */
+  ARG (overaligned, v, R1)
+  ARG (int, 11, R3)
+  ARG (int, 9, STACK)
+  LAST_ARG (overaligned, w, STACK+4)
+#endif
diff --git a/gcc/testsuite/gcc.target/arm/aapcs/overalign_rec2.c b/gcc/testsuite/gcc.target/arm/aapcs/overalign_rec2.c
new file mode 100644
index 00000000000..b19fa70c591
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/aapcs/overalign_rec2.c
@@ -0,0 +1,25 @@
+/* Test AAPCS layout (alignment).  */
+
+/* { dg-do run { target arm_eabi } } */
+/* { dg-require-effective-target arm32 } */
+/* { dg-options "-O" } */
+
+#ifndef IN_FRAMEWORK
+#define TESTFILE "overalign_rec2.c"
+
+typedef struct
+{
+  int  __attribute__((aligned(8))) a;
+  int b;
+} overaligned;
+
+overaligned v = {1, 3};
+overaligned w = {33, 99};
+
+#include "abitest.h"
+#else
+  ARG (int, 7, R0)
+  ARG (overaligned, v, R2)
+  ARG (int, 9, STACK)
+  LAST_ARG (overaligned, w, STACK+8)
+#endif
diff --git a/gcc/testsuite/gcc.target/arm/aapcs/overalign_rec3.c b/gcc/testsuite/gcc.target/arm/aapcs/overalign_rec3.c
new file mode 100644
index 00000000000..b1c793e04e6
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/aapcs/overalign_rec3.c
@@ -0,0 +1,28 @@
+/* Test AAPCS layout (alignment).  */
+
+/* { dg-do run { target arm_eabi } } */
+/* { dg-require-effective-target arm32 } */
+/* { dg-options "-O" } */
+
+#ifndef IN_FRAMEWORK
+#define TESTFILE "overalign_rec3.c"
+
+typedef struct
+{
+  int  __attribute__((aligned(16))) a;
+  int b;
+} overaligned;
+
+overaligned v = {1, 3};
+overaligned w = {33, 99};
+
+#include "abitest.h"
+#else
+  ARG (int, 7, R0)
+  /* Objects with alignment > 8 are passed with alignment 8.  */
+  ARG (overaligned, v, R2)
+  ARG (int, 9, STACK+8)
+  ARG (int, 10, STACK+12)
+  ARG (int, 11, STACK+16)
+  LAST_ARG (overaligned, w, STACK+24)
+#endif