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

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

[Bernd Edlinger]

Hi,

I know I am a bit late on the party.

But I have a question...

Consider this test case:

$ cat test.c
struct s {
  int a, b;
} __attribute__((aligned(8)));

struct s f0;
int f(int a, int b, int c, int d, int e, struct s f)
{
  f0 = f;
  return __alignof(f);
}

$ arm-linux-gnueabihf-gcc -march=armv5te -O3 -S test.c
$ cat test.s
f:
	@ args = 12, pretend = 0, frame = 0
	@ frame_needed = 0, uses_anonymous_args = 0
	@ link register save eliminated.
	push	{r4, r5}
	mov	r0, #8
	ldrd	r4, [sp, #12]
	ldr	r3, .L4
	strd	r4, [r3]
	pop	{r4, r5}
	bx	lr

I am pretty sure, although there is no warning, this ABI changed in GCC 5.2

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0473m/dom1361290002364.html
says:

"In ARMv5TE, or in ARMv6 when SCTLR.U is 0, LDRD and STRD doubleword data transfers must be
eight-byte aligned.  Use ALIGN 8 before memory allocation directives such as DCQ if the data
is to be accessed using LDRD or STRD.  This is not required in ARMv6 when SCTLR.U is 1, or in
ARMv7, because in these versions, doubleword data transfers can be word-aligned."


So isn't this wrong code, returning 8 for alignof when it is really 4,
and wouldn't it crash on armv5 and armv6 with SCTLR.U=0 ?


Bernd.

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

[Richard Earnshaw (lists)]

On 27/01/2019 11:20, Bernd Edlinger wrote:
> Hi,
> 
> I know I am a bit late on the party.

Sorry for the delay replying, I've been off sick...

> 
> But I have a question...
> 
> Consider this test case:
> 
> $ cat test.c
> struct s {
>   int a, b;
> } __attribute__((aligned(8)));
> 
> struct s f0;
> int f(int a, int b, int c, int d, int e, struct s f)
> {
>   f0 = f;
>   return __alignof(f);

This is equivalent to writing __alignof (struct s), so the returned
value should be 8.

HOWEVER, f itself is just a value in terms of the PCS, so it is correct
for it to be passed at SP+4.

> }
> 
> $ arm-linux-gnueabihf-gcc -march=armv5te -O3 -S test.c
> $ cat test.s
> f:
> 	@ args = 12, pretend = 0, frame = 0
> 	@ frame_needed = 0, uses_anonymous_args = 0
> 	@ link register save eliminated.
> 	push	{r4, r5}
> 	mov	r0, #8
> 	ldrd	r4, [sp, #12]

So this is wrong; before the compiler can use 'f' it has to copy it to a
suitably aligned location; it can't directly reuse the value on the
stack as that is not sufficiently aligned for the type.

> 	ldr	r3, .L4
> 	strd	r4, [r3]
> 	pop	{r4, r5}
> 	bx	lr
> 
> I am pretty sure, although there is no warning, this ABI changed in GCC 5.2

There was no explicit ABI for overaligned types in the past; so anything
could have happened.

> 
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0473m/dom1361290002364.html
> says:
> 
> "In ARMv5TE, or in ARMv6 when SCTLR.U is 0, LDRD and STRD doubleword data transfers must be
> eight-byte aligned.  Use ALIGN 8 before memory allocation directives such as DCQ if the data
> is to be accessed using LDRD or STRD.  This is not required in ARMv6 when SCTLR.U is 1, or in
> ARMv7, because in these versions, doubleword data transfers can be word-aligned."
> 
> 
> So isn't this wrong code, returning 8 for alignof when it is really 4,
> and wouldn't it crash on armv5 and armv6 with SCTLR.U=0 ?

Returning 8 is correct; since that is the alignment of the type; but GCC
does need to copy underaligned types to suitably aligned memory before
it uses them; it must not use the *value* that is passed directly,
unless it can prove that doing so is safe (and as you point out, on
armv5 it is not).

I think technically, this is separate bug from the PCS one that was
fixed, so needs a new PR.

> 
> 
> Bernd.
> 

Thanks,

R.

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

[Bernd Edlinger]

On 2/28/19 1:10 PM, Richard Earnshaw (lists) wrote:
> On 27/01/2019 11:20, Bernd Edlinger wrote:
>>
>> $ arm-linux-gnueabihf-gcc -march=armv5te -O3 -S test.c
>> $ cat test.s
>> f:
>> 	@ args = 12, pretend = 0, frame = 0
>> 	@ frame_needed = 0, uses_anonymous_args = 0
>> 	@ link register save eliminated.
>> 	push	{r4, r5}
>> 	mov	r0, #8
>> 	ldrd	r4, [sp, #12]
> 
> So this is wrong; before the compiler can use 'f' it has to copy it to a
> suitably aligned location; it can't directly reuse the value on the
> stack as that is not sufficiently aligned for the type.
> 

Yes, meanwhile I found out that the value would be copied in a new place
if an address was taken, but there is an issue with
output_move_double not checking the value's MEM_ALIGN.

>>
>> So isn't this wrong code, returning 8 for alignof when it is really 4,
>> and wouldn't it crash on armv5 and armv6 with SCTLR.U=0 ?
> 
> Returning 8 is correct; since that is the alignment of the type; but GCC
> does need to copy underaligned types to suitably aligned memory before
> it uses them; it must not use the *value* that is passed directly,
> unless it can prove that doing so is safe (and as you point out, on
> armv5 it is not).
> 
> I think technically, this is separate bug from the PCS one that was
> fixed, so needs a new PR.
> 

Could you have a look at the patch I sent to fix the wrong code issue:
https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00248.html

Is there a chance that this can still go into gcc-9?
Or do I have to to open a PR for it first?


Thanks
Bernd.

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

[Richard Earnshaw (lists)]

On 28/02/2019 14:51, Bernd Edlinger wrote:
> On 2/28/19 1:10 PM, Richard Earnshaw (lists) wrote:
>> On 27/01/2019 11:20, Bernd Edlinger wrote:
>>>
>>> $ arm-linux-gnueabihf-gcc -march=armv5te -O3 -S test.c
>>> $ cat test.s
>>> f:
>>> 	@ args = 12, pretend = 0, frame = 0
>>> 	@ frame_needed = 0, uses_anonymous_args = 0
>>> 	@ link register save eliminated.
>>> 	push	{r4, r5}
>>> 	mov	r0, #8
>>> 	ldrd	r4, [sp, #12]
>>
>> So this is wrong; before the compiler can use 'f' it has to copy it to a
>> suitably aligned location; it can't directly reuse the value on the
>> stack as that is not sufficiently aligned for the type.
>>
> 
> Yes, meanwhile I found out that the value would be copied in a new place
> if an address was taken, but there is an issue with
> output_move_double not checking the value's MEM_ALIGN.

I think that's a symptom of an earlier problem: why is gen_movdi being
called with a 32-bit aligned DImode memory object?

R.

> 
>>>
>>> So isn't this wrong code, returning 8 for alignof when it is really 4,
>>> and wouldn't it crash on armv5 and armv6 with SCTLR.U=0 ?
>>
>> Returning 8 is correct; since that is the alignment of the type; but GCC
>> does need to copy underaligned types to suitably aligned memory before
>> it uses them; it must not use the *value* that is passed directly,
>> unless it can prove that doing so is safe (and as you point out, on
>> armv5 it is not).
>>
>> I think technically, this is separate bug from the PCS one that was
>> fixed, so needs a new PR.
>>
> 
> Could you have a look at the patch I sent to fix the wrong code issue:
> https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00248.html
> 
> Is there a chance that this can still go into gcc-9?
> Or do I have to to open a PR for it first?
> 
> 
> Thanks
> Bernd.
> 

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

[Richard Earnshaw (lists)]

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

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
> 


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

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

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

[Jakub Jelinek]

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

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

[Richard Earnshaw (lists)]

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

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

[Bernd Edlinger]

Hi,

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


I am not really an expert here, Joseph might know the right answers.

I think there are two different alignment values, one is the minimum
alignment for a type within another structure, that can be computed
with:

min_align_of_type (type)

and there is a possibly greater value that tells how a type is aligned
when used alone:

TYPE_ALIGN_UNIT (type)

I thought both are always identical on arm, but maybe they are not.
They are for instance different for double on x86_64 in structures that
type is 4-byte aligned, and used alone, it is 8-byte aligned.


I wonder why you have to iterate over all the type members.


for instance this also gets the right alignment of the bit field type:

--- arm/arm.c	(Revision 267164)
+++ arm/arm.c	(Arbeitskopie)
@@ -6622,6 +6622,8 @@
  	  ret = -1;
        }
  
+  if (min_align_of_type (const_cast<tree>(type)) > PARM_BOUNDARY / BITS_PER_UNIT)
+    return 1;
    return ret;
  }
  


But I might have missed the point why this needs to be dome this way.


Regards
Bernd.

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

[Richard Earnshaw (lists)]

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

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.

[-- Attachment #2: aapcs.patch --]
[-- Type: text/x-patch, Size: 3656 bytes --]

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