https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77728
--- Comment #18 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
If you want a testcase for aarch64, e.g.
template <int N>
struct alignas (16) A { char p[16]; };
A<0> v;
template <int N>
struct B
{
typedef A<N> T;
int i, j, k, l;
};
int
foo (int a, B<0> b)
{
return a + b.i;
}
int
bar (int a, B<1> b)
{
return a + b.i;
}
struct C
{
static int h alignas (16);
int i, j, k, l;
};
int
baz (int a, C b)
{
return a + b.i;
}
will do the job, there is a warning on foo's b argument, because A<0> has been
instantiated first and when instantiating B<0>, we already see A<0>'s alignment
and use it when creating the TYPE_DECL. Compared to that, for B<1> we don't
have it instantiated yet, so when TYPE_DECL is created, it gets minimal
alignment. You might want to create e.g. a compat testcase from it too, have
caller of foo with the A<0> v; in it and callee without and vice versa, check
if it is able to pass the argument around properly. Similarly with baz (in
that case it actually was a stable (but wrong) ABI before, the baz caller and
callee would agree, but they now don't in between unpatched and patched GCC.
For ARM you can easily adjust the testcase by s/16/8/g, and maybe removing k
and l fields too.
Comments on the patch:
#include "print-tree.h"
you don't really need this, that is from some debugging, right?
unsigned int alignment ; // alignment for FIELD_DECLS in function arguments.
extraneous space before ;, not sure if it is a good idea to mix // and /*
comments and on the next line it is way too long, so maybe better use /* */
comments above each field.
{
+
+ unsigned int alignment = 0;
+ unsigned int warning_alignment = 0;
extraneous vertical space. Also, what is the point to have a struct containing
those 2 fields and auto vars mirroring that? Just use the struct all the time?
alignment = std::max (alignment, DECL_ALIGN (field));
I think we usually use MAX rather than std::max.
+ if (nregs == 2 && (a.warning_alignment == 16 * BITS_PER_UNIT) && ncrn %
2)
+ if (a.warning_alignment < a.alignment
+ && warn_psabi)
+ warning (0, "Parameter passing changed for argument");
The if condition fits on one line. Do you need the ()s around
a.warning_alignment == 16 * BITS_PER_UNIT? Can't it be written as
if (warn_psabi
&& nregs == 2
&& a.warning_alignment == 16 * BITS_PER_UNIT
&& a.alignment != 16 * BITS_PER_UNIT
&& ncrn % 2)
? That is what is actually tested originally, whether alignment == 16 *
BITS_PER_UNIT. Also, warnings should not start with capital letter.
And it would be nice to tell user which argument it is (does cumulative_args_t
track the argument number or could it?).
on_stack:
pcum->aapcs_stack_words = size / UNITS_PER_WORD;
- if (aarch64_function_arg_alignment (mode, type) == 16 * BITS_PER_UNIT)
+ struct aarch64_fn_arg_alignment ab = aarch64_function_arg_alignment (mode,
type);
+ if (ab.alignment == 16 * BITS_PER_UNIT)
pcum->aapcs_stack_size = ROUND_UP (pcum->aapcs_stack_size,
16 / UNITS_PER_WORD);
No warning here, if ab.alignment is not 16 * BITS_PER_UNIT, but
ab.warning_alignment is and warn_psabi is on? I must say I don't really know
exactly what pcum->aapcs_stack_size influence, but doesn't it influence the ABI
when the function is varargs? I mean, with the above testcase, something like:
void
foo (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, ...)
with/without the A<0> v; early?
if (alignment < PARM_BOUNDARY)
alignment = PARM_BOUNDARY;
if (alignment > STACK_BOUNDARY)
alignment = STACK_BOUNDARY;
+
+ if (a.warning_alignment < PARM_BOUNDARY)
+ a.warning_alignment = PARM_BOUNDARY;
+
+ if (a.warning_alignment > STACK_BOUNDARY)
+ a.warning_alignment = STACK_BOUNDARY;
+
Isn't the above candidate for MAX/MIN to put it into fewer lines?
+ if (a.warning_alignment > alignment
+ && warn_psabi)
+ warning (0, "Parameter passing changed for argument in
function_arg_boundary");
+
+
Again, condition can fit onto a single line. Due to MAX/MIN, I guess the only
possible values of those 2 are 64 and 128, so the comparison is fine. But
again, warning should not start with a capital letter and mentioning "in
function_arg_boundary" is not helpful to the users, much better would be to
tell which argument it is or something similar. And, perhaps remove the
alignment variable and always use a.alignment? There is also extra vertical
space before return.
+ align = a.alignment / BITS_PER_UNIT;
Extraneous whitespace after =.
