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

Message ID bd74f23f-38d1-9d0b-6cb5-8168dffad174@arm.com
State New
Headers show
Series
  • [arm,RFC] PR target/88469 fix incorrect argument passing with 64-bit bitfields
Related show

Commit Message

Richard Earnshaw (lists) Dec. 17, 2018, 4:04 p.m.
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.

Comments

Richard Earnshaw (lists) Jan. 22, 2019, 2:10 p.m. | #1
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.
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
Jakub Jelinek Jan. 22, 2019, 2:50 p.m. | #2
On Tue, Jan 22, 2019 at 02:10:59PM +0000, Richard Earnshaw (lists) wrote:
> @@ -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;


Can you double check what behavior you want e.g. for:
typedef int alint __attribute__((aligned (8)));
struct S1 { alint a : 17; alint b : 15; };
struct __attribute__((packed)) S2 { char a; long long b : 12; long long c : 20; long long d : 32; };
struct __attribute__((packed)) S3 { char a; alint b : 12; alint c : 20; };
and passing/returning of S1/S2/S3?
TYPE_ALIGN (DECL_BIT_FIELD_TYPE (field)) is I think alint in this case,
and for packed structures, I guess DECL_ALIGN of the fields is generally
small, but TYPE_ALIGN might not be.

	Jakub
Richard Earnshaw (lists) Jan. 22, 2019, 6:01 p.m. | #3
On 22/01/2019 14:50, Jakub Jelinek wrote:
> On Tue, Jan 22, 2019 at 02:10:59PM +0000, Richard Earnshaw (lists) wrote:

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

> 

> Can you double check what behavior you want e.g. for:

> typedef int alint __attribute__((aligned (8)));

> struct S1 { alint a : 17; alint b : 15; };

> struct __attribute__((packed)) S2 { char a; long long b : 12; long long c : 20; long long d : 32; };

> struct __attribute__((packed)) S3 { char a; alint b : 12; alint c : 20; };

> and passing/returning of S1/S2/S3?

> TYPE_ALIGN (DECL_BIT_FIELD_TYPE (field)) is I think alint in this case,

> and for packed structures, I guess DECL_ALIGN of the fields is generally

> small, but TYPE_ALIGN might not be.


If TYPE_ALIGN says that we need DWORD alignment then we never look
inside the bitfield, it's only when that is less than 64-bit alignment
that we look deeper.

But Richi pointed out that

int64_t a : 16;

can be internally transformed into a 'short' so we need to look at
DECL_BIT_FIELD_TYPE not at DECL_BIT_FIELD.

I've also added a test for bitfields that are based on overaligned types.

Fixed thusly.

PR target/88469
gcc:
	* config/arm/arm.c (arm_needs_double_word_align): Check
	DECL_BIT_FIELD_TYPE.

gcc/testsuite:
	* gcc.target/arm/aapcs/bitfield2.c: New test.
	* gcc.target/arm/aapcs/bitfield3.c: New test.


> 

> 	Jakub

>
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index c6fbda25e96..16e22eed871 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -6634,7 +6634,7 @@ arm_needs_doubleword_align (machine_mode mode, const_tree type)
 	  ret = -1;
       }
     else if (TREE_CODE (field) == FIELD_DECL
-	     && DECL_BIT_FIELD (field)
+	     && DECL_BIT_FIELD_TYPE (field)
 	     && TYPE_ALIGN (DECL_BIT_FIELD_TYPE (field)) > PARM_BOUNDARY)
       ret2 = 1;
 
diff --git a/gcc/testsuite/gcc.target/arm/aapcs/bitfield2.c b/gcc/testsuite/gcc.target/arm/aapcs/bitfield2.c
new file mode 100644
index 00000000000..9cbe2b08962
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/aapcs/bitfield2.c
@@ -0,0 +1,26 @@
+/* Test AAPCS layout (alignment).  */
+
+/* { dg-do run { target arm_eabi } } */
+/* { dg-require-effective-target arm32 } */
+/* { dg-options "-O" } */
+
+#ifndef IN_FRAMEWORK
+#define TESTFILE "bitfield2.c"
+
+typedef unsigned int alint __attribute__((aligned (8)));
+
+struct bf
+{
+  alint a: 17;
+  alint b: 15;
+} 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/bitfield3.c b/gcc/testsuite/gcc.target/arm/aapcs/bitfield3.c
new file mode 100644
index 00000000000..0386e669c2d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/aapcs/bitfield3.c
@@ -0,0 +1,26 @@
+/* Test AAPCS layout (alignment).  */
+
+/* { dg-do run { target arm_eabi } } */
+/* { dg-require-effective-target arm32 } */
+/* { dg-options "-O" } */
+
+#ifndef IN_FRAMEWORK
+#define TESTFILE "bitfield3.c"
+
+struct bf
+{
+  /* Internally this may be mapped to unsigned short.  Ensure we still
+     check the original declaration.  */
+  unsigned long long a: 16;
+  unsigned b: 3;
+} v = {1, 3};
+
+#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

Patch

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 40f0574e32e..5f5269c71c9 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -6589,7 +6589,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
@@ -6609,7 +6611,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)
       {
@@ -6621,6 +6624,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;
 }
@@ -6686,7 +6696,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
@@ -6713,6 +6728,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;
 }
@@ -26953,7 +26971,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..7d8b7065484
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/aapcs/bitfield1.c
@@ -0,0 +1,23 @@ 
+/* 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)
+  /* Attribute suggests R2, but we should use only natural alignment:  */
+  ARG (int, 9, R1)
+  ARG (int, 11, R2)
+  LAST_ARG (struct bf, v, STACK)
+#endif