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