calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453]
Checks
Commit Message
Hi!
On powerpc64le-linux, the following patch fixes
-FAIL: gcc.dg/c2x-stdarg-4.c execution test
-FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O0 execution test
-FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O1 execution test
-FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O2 execution test
-FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test
-FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects execution test
-FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O3 -g execution test
-FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -Os execution test
The problem is mismatch between the caller and callee side.
On the callee side, we do:
/* NAMED_ARG is a misnomer. We really mean 'non-variadic'. */
if (!cfun->stdarg)
data->arg.named = 1; /* No variadic parms. */
else if (DECL_CHAIN (parm))
data->arg.named = 1; /* Not the last non-variadic parm. */
else if (targetm.calls.strict_argument_naming (all->args_so_far))
data->arg.named = 1; /* Only variadic ones are unnamed. */
else
data->arg.named = 0; /* Treat as variadic. */
which is later passed to the target hooks to determine if a particular
argument is named or not. Now, cfun->stdarg is determined from the stdarg_p
call, which for the new C2X TYPE_NO_NAMED_ARGS_STDARG_P function types
(rettype fn (...)) returns true. Such functions have no named arguments,
so data->arg.named will be 0 in function.cc. But on the caller side,
as TYPE_NO_NAMED_ARGS_STDARG_P function types have TYPE_ARG_TYPES NULL,
we instead treat those calls as unprototyped even when they are prototyped
- /* If we know nothing, treat all args as named. */ n_named_args = num_actuals;
in 2 spots. We need to treat the TYPE_NO_NAMED_ARGS_STDARG_P cases as
prototyped with no named arguments.
Bootstrapped/regtested on x86_64-linux, i686-linux, powerpc64le-linux (where
it fixes the above failures), aarch64-linux and s390x-linux, ok for trunk?
2023-01-09 Jakub Jelinek <jakub@redhat.com>
PR target/107453
* calls.cc (expand_call): For calls with
TYPE_NO_NAMED_ARGS_STDARG_P (funtype) use zero for n_named_args.
Formatting fix.
Jakub
Comments
On Mon, 9 Jan 2023, Jakub Jelinek wrote:
> Hi!
>
> On powerpc64le-linux, the following patch fixes
> -FAIL: gcc.dg/c2x-stdarg-4.c execution test
> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O0 execution test
> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O1 execution test
> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O2 execution test
> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test
> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects execution test
> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O3 -g execution test
> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -Os execution test
> The problem is mismatch between the caller and callee side.
> On the callee side, we do:
> /* NAMED_ARG is a misnomer. We really mean 'non-variadic'. */
> if (!cfun->stdarg)
> data->arg.named = 1; /* No variadic parms. */
> else if (DECL_CHAIN (parm))
> data->arg.named = 1; /* Not the last non-variadic parm. */
> else if (targetm.calls.strict_argument_naming (all->args_so_far))
> data->arg.named = 1; /* Only variadic ones are unnamed. */
> else
> data->arg.named = 0; /* Treat as variadic. */
> which is later passed to the target hooks to determine if a particular
> argument is named or not. Now, cfun->stdarg is determined from the stdarg_p
> call, which for the new C2X TYPE_NO_NAMED_ARGS_STDARG_P function types
> (rettype fn (...)) returns true. Such functions have no named arguments,
> so data->arg.named will be 0 in function.cc. But on the caller side,
> as TYPE_NO_NAMED_ARGS_STDARG_P function types have TYPE_ARG_TYPES NULL,
> we instead treat those calls as unprototyped even when they are prototyped
> - /* If we know nothing, treat all args as named. */ n_named_args = num_actuals;
> in 2 spots. We need to treat the TYPE_NO_NAMED_ARGS_STDARG_P cases as
> prototyped with no named arguments.
>
> Bootstrapped/regtested on x86_64-linux, i686-linux, powerpc64le-linux (where
> it fixes the above failures), aarch64-linux and s390x-linux, ok for trunk?
LGTM.
Richard.
> 2023-01-09 Jakub Jelinek <jakub@redhat.com>
>
> PR target/107453
> * calls.cc (expand_call): For calls with
> TYPE_NO_NAMED_ARGS_STDARG_P (funtype) use zero for n_named_args.
> Formatting fix.
>
> --- gcc/calls.cc.jj 2023-01-02 09:32:28.834192105 +0100
> +++ gcc/calls.cc 2023-01-06 14:52:14.740594896 +0100
> @@ -2908,8 +2908,8 @@ expand_call (tree exp, rtx target, int i
> }
>
> /* Count the arguments and set NUM_ACTUALS. */
> - num_actuals =
> - call_expr_nargs (exp) + num_complex_actuals + structure_value_addr_parm;
> + num_actuals
> + = call_expr_nargs (exp) + num_complex_actuals + structure_value_addr_parm;
>
> /* Compute number of named args.
> First, do a raw count of the args for INIT_CUMULATIVE_ARGS. */
> @@ -2919,6 +2919,8 @@ expand_call (tree exp, rtx target, int i
> = (list_length (type_arg_types)
> /* Count the struct value address, if it is passed as a parm. */
> + structure_value_addr_parm);
> + else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
> + n_named_args = 0;
> else
> /* If we know nothing, treat all args as named. */
> n_named_args = num_actuals;
> @@ -2957,6 +2959,8 @@ expand_call (tree exp, rtx target, int i
> && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
> /* Don't include the last named arg. */
> --n_named_args;
> + else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
> + n_named_args = 0;
> else
> /* Treat all args as named. */
> n_named_args = num_actuals;
>
> Jakub
>
>
On 09/01/2023 10:32, Jakub Jelinek via Gcc-patches wrote:
> Hi!
>
> On powerpc64le-linux, the following patch fixes
> -FAIL: gcc.dg/c2x-stdarg-4.c execution test
> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O0 execution test
> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O1 execution test
> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O2 execution test
> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test
> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects execution test
> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O3 -g execution test
> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -Os execution test
> The problem is mismatch between the caller and callee side.
> On the callee side, we do:
> /* NAMED_ARG is a misnomer. We really mean 'non-variadic'. */
> if (!cfun->stdarg)
> data->arg.named = 1; /* No variadic parms. */
> else if (DECL_CHAIN (parm))
> data->arg.named = 1; /* Not the last non-variadic parm. */
> else if (targetm.calls.strict_argument_naming (all->args_so_far))
> data->arg.named = 1; /* Only variadic ones are unnamed. */
> else
> data->arg.named = 0; /* Treat as variadic. */
> which is later passed to the target hooks to determine if a particular
> argument is named or not. Now, cfun->stdarg is determined from the stdarg_p
> call, which for the new C2X TYPE_NO_NAMED_ARGS_STDARG_P function types
> (rettype fn (...)) returns true. Such functions have no named arguments,
> so data->arg.named will be 0 in function.cc. But on the caller side,
> as TYPE_NO_NAMED_ARGS_STDARG_P function types have TYPE_ARG_TYPES NULL,
> we instead treat those calls as unprototyped even when they are prototyped
> - /* If we know nothing, treat all args as named. */ n_named_args = num_actuals;
> in 2 spots. We need to treat the TYPE_NO_NAMED_ARGS_STDARG_P cases as
> prototyped with no named arguments.
>
> Bootstrapped/regtested on x86_64-linux, i686-linux, powerpc64le-linux (where
> it fixes the above failures), aarch64-linux and s390x-linux, ok for trunk?
>
> 2023-01-09 Jakub Jelinek <jakub@redhat.com>
>
> PR target/107453
> * calls.cc (expand_call): For calls with
> TYPE_NO_NAMED_ARGS_STDARG_P (funtype) use zero for n_named_args.
> Formatting fix.
This one has been festering for a while; both Alexandre and Torbjorn have attempted to fix it recently, but I'm not sure either is really right...
On Arm this is causing all anonymous arguments to be passed on the stack, which is incorrect per the ABI. On a target that uses 'pretend_outgoing_vararg_named', why is it correct to set n_named_args to zero? Is it enough to guard both the statements you've added with !targetm.calls.pretend_outgoing_args_named?
R.
>
> --- gcc/calls.cc.jj 2023-01-02 09:32:28.834192105 +0100
> +++ gcc/calls.cc 2023-01-06 14:52:14.740594896 +0100
> @@ -2908,8 +2908,8 @@ expand_call (tree exp, rtx target, int i
> }
>
> /* Count the arguments and set NUM_ACTUALS. */
> - num_actuals =
> - call_expr_nargs (exp) + num_complex_actuals + structure_value_addr_parm;
> + num_actuals
> + = call_expr_nargs (exp) + num_complex_actuals + structure_value_addr_parm;
>
> /* Compute number of named args.
> First, do a raw count of the args for INIT_CUMULATIVE_ARGS. */
> @@ -2919,6 +2919,8 @@ expand_call (tree exp, rtx target, int i
> = (list_length (type_arg_types)
> /* Count the struct value address, if it is passed as a parm. */
> + structure_value_addr_parm);
> + else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
> + n_named_args = 0;
> else
> /* If we know nothing, treat all args as named. */
> n_named_args = num_actuals;
> @@ -2957,6 +2959,8 @@ expand_call (tree exp, rtx target, int i
> && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
> /* Don't include the last named arg. */
> --n_named_args;
> + else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
> + n_named_args = 0;
> else
> /* Treat all args as named. */
> n_named_args = num_actuals;
>
> Jakub
>
On Tue, Feb 27, 2024 at 04:41:32PM +0000, Richard Earnshaw wrote:
> > 2023-01-09 Jakub Jelinek <jakub@redhat.com>
> >
> > PR target/107453
> > * calls.cc (expand_call): For calls with
> > TYPE_NO_NAMED_ARGS_STDARG_P (funtype) use zero for n_named_args.
> > Formatting fix.
>
> This one has been festering for a while; both Alexandre and Torbjorn have attempted to fix it recently, but I'm not sure either is really right...
>
> On Arm this is causing all anonymous arguments to be passed on the stack,
> which is incorrect per the ABI. On a target that uses
> 'pretend_outgoing_vararg_named', why is it correct to set n_named_args to
> zero? Is it enough to guard both the statements you've added with
> !targetm.calls.pretend_outgoing_args_named?
I'm afraid I haven't heard of that target hook before.
All I was doing with that change was fixing a regression reported in the PR
for ppc64le/sparc/nvptx/loongarch at least.
The TYPE_NO_NAMED_ARGS_STDARG_P functions (C23 fns like void foo (...) {})
have NULL type_arg_types, so the list_length (type_arg_types) isn't done for
it, but it should be handled as if it was non-NULL but list length was 0.
So, for the
if (type_arg_types != 0)
n_named_args
= (list_length (type_arg_types)
/* Count the struct value address, if it is passed as a parm. */
+ structure_value_addr_parm);
else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
n_named_args = 0;
else
/* If we know nothing, treat all args as named. */
n_named_args = num_actuals;
case, I think guarding it by any target hooks is wrong, although
I guess it should have been
n_named_args = structure_value_addr_parm;
instead of
n_named_args = 0;
For the second
if (type_arg_types != 0
&& targetm.calls.strict_argument_naming (args_so_far))
;
else if (type_arg_types != 0
&& ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
/* Don't include the last named arg. */
--n_named_args;
else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
n_named_args = 0;
else
/* Treat all args as named. */
n_named_args = num_actuals;
bet (but no testing done, don't even know which targets return what for
those hooks) we should treat those as if type_arg_types was non-NULL
with 0 elements in the list, except the --n_named_args doesn't make sense
because that would decrease it to -1.
So perhaps
if ((type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
&& targetm.calls.strict_argument_naming (args_so_far))
;
else if (type_arg_types != 0
&& ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
/* Don't include the last named arg. */
--n_named_args;
else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype)
&& ! targetm.calls.pretend_outgoing_varargs_named (args_so_far)))
;
else
/* Treat all args as named. */
n_named_args = num_actuals;
(or n_named_args = 0; instead of ; before the final else? Dunno).
I guess we need some testsuite coverage for caller/callee ABI match of
struct S { char p[64]; };
struct S foo (...);
Jakub
[resending, apologies, I accidentally CC'd the wrong person last time]
On 27/02/2024 16:41, Richard Earnshaw wrote:
>
>
> On 09/01/2023 10:32, Jakub Jelinek via Gcc-patches wrote:
>> Hi!
>>
>> On powerpc64le-linux, the following patch fixes
>> -FAIL: gcc.dg/c2x-stdarg-4.c execution test
>> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O0 execution test
>> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O1 execution test
>> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O2 execution test
>> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test
>> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects execution test
>> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -O3 -g execution test
>> -FAIL: gcc.dg/torture/c2x-stdarg-split-1a.c -Os execution test
>> The problem is mismatch between the caller and callee side.
>> On the callee side, we do:
>> /* NAMED_ARG is a misnomer. We really mean 'non-variadic'. */
>> if (!cfun->stdarg)
>> data->arg.named = 1; /* No variadic parms. */
>> else if (DECL_CHAIN (parm))
>> data->arg.named = 1; /* Not the last non-variadic parm. */
>> else if (targetm.calls.strict_argument_naming (all->args_so_far))
>> data->arg.named = 1; /* Only variadic ones are unnamed. */
>> else
>> data->arg.named = 0; /* Treat as variadic. */
>> which is later passed to the target hooks to determine if a particular
>> argument is named or not. Now, cfun->stdarg is determined from the stdarg_p
>> call, which for the new C2X TYPE_NO_NAMED_ARGS_STDARG_P function types
>> (rettype fn (...)) returns true. Such functions have no named arguments,
>> so data->arg.named will be 0 in function.cc. But on the caller side,
>> as TYPE_NO_NAMED_ARGS_STDARG_P function types have TYPE_ARG_TYPES NULL,
>> we instead treat those calls as unprototyped even when they are prototyped
>> - /* If we know nothing, treat all args as named. */ n_named_args = num_actuals;
>> in 2 spots. We need to treat the TYPE_NO_NAMED_ARGS_STDARG_P cases as
>> prototyped with no named arguments.
>>
>> Bootstrapped/regtested on x86_64-linux, i686-linux, powerpc64le-linux (where
>> it fixes the above failures), aarch64-linux and s390x-linux, ok for trunk?
>>
>> 2023-01-09 Jakub Jelinek <jakub@redhat.com>
>>
>> PR target/107453
>> * calls.cc (expand_call): For calls with
>> TYPE_NO_NAMED_ARGS_STDARG_P (funtype) use zero for n_named_args.
>> Formatting fix.
>
> This one has been festering for a while; both Alexandre and Torbjorn have attempted to fix it recently, but I'm not sure either is really right...
>
> On Arm this is causing all anonymous arguments to be passed on the stack, which is incorrect per the ABI. On a target that uses 'pretend_outgoing_vararg_named', why is it correct to set n_named_args to zero? Is it enough to guard both the statements you've added with !targetm.calls.pretend_outgoing_args_named?
>
> R.
>
>>
>> --- gcc/calls.cc.jj 2023-01-02 09:32:28.834192105 +0100
>> +++ gcc/calls.cc 2023-01-06 14:52:14.740594896 +0100
>> @@ -2908,8 +2908,8 @@ expand_call (tree exp, rtx target, int i
>> }
>>
>> /* Count the arguments and set NUM_ACTUALS. */
>> - num_actuals =
>> - call_expr_nargs (exp) + num_complex_actuals + structure_value_addr_parm;
>> + num_actuals
>> + = call_expr_nargs (exp) + num_complex_actuals + structure_value_addr_parm;
>>
>> /* Compute number of named args.
>> First, do a raw count of the args for INIT_CUMULATIVE_ARGS. */
>> @@ -2919,6 +2919,8 @@ expand_call (tree exp, rtx target, int i
>> = (list_length (type_arg_types)
>> /* Count the struct value address, if it is passed as a parm. */
>> + structure_value_addr_parm);
>> + else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
>> + n_named_args = 0;
>> else
>> /* If we know nothing, treat all args as named. */
>> n_named_args = num_actuals;
>> @@ -2957,6 +2959,8 @@ expand_call (tree exp, rtx target, int i
>> && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
>> /* Don't include the last named arg. */
>> --n_named_args;
>> + else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
>> + n_named_args = 0;
>> else
>> /* Treat all args as named. */
>> n_named_args = num_actuals;
>>
>> Jakub
>>
On Tue, Feb 27, 2024 at 06:25:21PM +0100, Jakub Jelinek wrote:
> I guess we need some testsuite coverage for caller/callee ABI match of
> struct S { char p[64]; };
> struct S foo (...);
Maybe the test below? Passes on x86_64 -m32/-m64, but I guess that doesn't
care at all about the named vs. not named distinction.
The test is a copy of c23-stdarg-4.c, just with all the functions returning
a large struct.
2024-02-27 Jakub Jelinek <jakub@redhat.com>
* gcc.dg/c23-stdarg-6.c: New test.
--- gcc/testsuite/gcc.dg/c23-stdarg-6.c.jj 2024-02-27 18:39:04.807821107 +0100
+++ gcc/testsuite/gcc.dg/c23-stdarg-6.c 2024-02-27 18:51:44.706308490 +0100
@@ -0,0 +1,217 @@
+/* Test C23 variadic functions with no named parameters, or last named
+ parameter with a declaration not allowed in C17. Execution tests. */
+/* { dg-do run } */
+/* { dg-options "-std=c23 -pedantic-errors" } */
+
+#include <stdarg.h>
+
+extern void abort (void);
+extern void exit (int);
+struct s { char c[1000]; };
+
+struct s
+f (...)
+{
+ va_list ap;
+ va_start (ap);
+ double r = va_arg (ap, int);
+ r += va_arg (ap, double);
+ r += va_arg (ap, int);
+ r += va_arg (ap, double);
+ va_end (ap);
+ struct s ret = {};
+ ret.c[0] = r;
+ ret.c[999] = 42;
+ return ret;
+}
+
+struct s
+g (...)
+{
+ va_list ap;
+ va_start (ap, random ! ignored, ignored ** text);
+ for (int i = 0; i < 10; i++)
+ if (va_arg (ap, double) != i)
+ abort ();
+ va_end (ap);
+ struct s ret = {};
+ ret.c[0] = 17;
+ ret.c[999] = 58;
+ return ret;
+}
+
+struct s
+h1 (register int x, ...)
+{
+ va_list ap;
+ va_start (ap);
+ for (int i = 0; i < 10; i++)
+ {
+ if (va_arg (ap, double) != i)
+ abort ();
+ i++;
+ if (va_arg (ap, int) != i)
+ abort ();
+ }
+ va_end (ap);
+ struct s ret = {};
+ ret.c[0] = 32;
+ ret.c[999] = 95;
+ return ret;
+}
+
+struct s
+h2 (int x(), ...)
+{
+ va_list ap;
+ va_start (ap);
+ for (int i = 0; i < 10; i++)
+ {
+ if (va_arg (ap, double) != i)
+ abort ();
+ i++;
+ if (va_arg (ap, int) != i)
+ abort ();
+ }
+ va_end (ap);
+ struct s ret = {};
+ ret.c[0] = 5;
+ ret.c[999] = 125;
+ return ret;
+}
+
+struct s
+h3 (int x[10], ...)
+{
+ va_list ap;
+ va_start (ap);
+ for (int i = 0; i < 10; i++)
+ {
+ if (va_arg (ap, double) != i)
+ abort ();
+ i++;
+ if (va_arg (ap, int) != i)
+ abort ();
+ }
+ va_end (ap);
+ struct s ret = {};
+ ret.c[0] = 8;
+ ret.c[999] = 12;
+ return ret;
+}
+
+struct s
+h4 (char x, ...)
+{
+ va_list ap;
+ va_start (ap);
+ for (int i = 0; i < 10; i++)
+ {
+ if (va_arg (ap, double) != i)
+ abort ();
+ i++;
+ if (va_arg (ap, int) != i)
+ abort ();
+ }
+ va_end (ap);
+ struct s ret = {};
+ ret.c[0] = 18;
+ ret.c[999] = 28;
+ return ret;
+}
+
+struct s
+h5 (float x, ...)
+{
+ va_list ap;
+ va_start (ap);
+ for (int i = 0; i < 10; i++)
+ {
+ if (va_arg (ap, double) != i)
+ abort ();
+ i++;
+ if (va_arg (ap, int) != i)
+ abort ();
+ }
+ va_end (ap);
+ struct s ret = {};
+ ret.c[0] = 38;
+ ret.c[999] = 48;
+ return ret;
+}
+
+struct s
+h6 (volatile long x, ...)
+{
+ va_list ap;
+ va_start (ap);
+ for (int i = 0; i < 10; i++)
+ {
+ if (va_arg (ap, double) != i)
+ abort ();
+ i++;
+ if (va_arg (ap, int) != i)
+ abort ();
+ }
+ va_end (ap);
+ struct s ret = {};
+ ret.c[0] = 58;
+ ret.c[999] = 68;
+ return ret;
+}
+
+struct s
+h7 (volatile struct s x, ...)
+{
+ va_list ap;
+ va_start (ap);
+ for (int i = 0; i < 10; i++)
+ {
+ if (va_arg (ap, double) != i)
+ abort ();
+ i++;
+ if (va_arg (ap, int) != i)
+ abort ();
+ }
+ va_end (ap);
+ struct s ret = {};
+ ret.c[0] = 78;
+ ret.c[999] = 88;
+ return ret;
+}
+
+int
+main ()
+{
+ struct s x = f (1, 2.0, 3, 4.0);
+ if (x.c[0] != 10 || x.c[999] != 42)
+ abort ();
+ x = g (0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0);
+ if (x.c[0] != 17 || x.c[999] != 58)
+ abort ();
+ x = g (0.0f, 1.0f, 2.0f, 3.0f, 4.0f, 5.0f, 6.0f, 7.0f, 8.0f, 9.0f);
+ if (x.c[0] != 17 || x.c[999] != 58)
+ abort ();
+ x = h1 (0, 0.0, 1, 2.0, 3, 4.0, 5, 6.0, 7, 8.0, 9);
+ if (x.c[0] != 32 || x.c[999] != 95)
+ abort ();
+ x = h2 (0, 0.0, 1, 2.0, 3, 4.0, 5, 6.0, 7, 8.0, 9);
+ if (x.c[0] != 5 || x.c[999] != 125)
+ abort ();
+ x = h3 (0, 0.0, 1, 2.0, 3, 4.0, 5, 6.0, 7, 8.0, 9);
+ if (x.c[0] != 8 || x.c[999] != 12)
+ abort ();
+ x = h4 (0, 0.0, 1, 2.0, 3, 4.0, 5, 6.0, 7, 8.0, 9);
+ if (x.c[0] != 18 || x.c[999] != 28)
+ abort ();
+ x = h5 (0, 0.0, 1, 2.0, 3, 4.0, 5, 6.0, 7, 8.0, 9);
+ if (x.c[0] != 38 || x.c[999] != 48)
+ abort ();
+ x = h6 (0, 0.0, 1, 2.0, 3, 4.0, 5, 6.0, 7, 8.0, 9);
+ if (x.c[0] != 58 || x.c[999] != 68)
+ abort ();
+ x = h7 ((struct s) {}, 0.0, 1, 2.0, 3, 4.0, 5, 6.0, 7, 8.0, 9);
+ if (x.c[0] != 78 || x.c[999] != 88)
+ abort ();
+ exit (0);
+}
Jakub
On Tue, Feb 27, 2024 at 06:54:49PM +0100, Jakub Jelinek wrote:
> On Tue, Feb 27, 2024 at 06:25:21PM +0100, Jakub Jelinek wrote:
> > I guess we need some testsuite coverage for caller/callee ABI match of
> > struct S { char p[64]; };
> > struct S foo (...);
>
> Maybe the test below? Passes on x86_64 -m32/-m64, but I guess that doesn't
> care at all about the named vs. not named distinction.
> The test is a copy of c23-stdarg-4.c, just with all the functions returning
> a large struct.
>
> 2024-02-27 Jakub Jelinek <jakub@redhat.com>
>
> * gcc.dg/c23-stdarg-6.c: New test.
I've committed the testcase to trunk now as obvious, so that we can see what
targets are broken.
Jakub
On 27/02/2024 17:25, Jakub Jelinek wrote:
> On Tue, Feb 27, 2024 at 04:41:32PM +0000, Richard Earnshaw wrote:
>>> 2023-01-09 Jakub Jelinek <jakub@redhat.com>
>>>
>>> PR target/107453
>>> * calls.cc (expand_call): For calls with
>>> TYPE_NO_NAMED_ARGS_STDARG_P (funtype) use zero for n_named_args.
>>> Formatting fix.
>>
>> This one has been festering for a while; both Alexandre and Torbjorn have attempted to fix it recently, but I'm not sure either is really right...
>>
>> On Arm this is causing all anonymous arguments to be passed on the stack,
>> which is incorrect per the ABI. On a target that uses
>> 'pretend_outgoing_vararg_named', why is it correct to set n_named_args to
>> zero? Is it enough to guard both the statements you've added with
>> !targetm.calls.pretend_outgoing_args_named?
>
> I'm afraid I haven't heard of that target hook before.
> All I was doing with that change was fixing a regression reported in the PR
> for ppc64le/sparc/nvptx/loongarch at least.
>
> The TYPE_NO_NAMED_ARGS_STDARG_P functions (C23 fns like void foo (...) {})
> have NULL type_arg_types, so the list_length (type_arg_types) isn't done for
> it, but it should be handled as if it was non-NULL but list length was 0.
>
> So, for the
> if (type_arg_types != 0)
> n_named_args
> = (list_length (type_arg_types)
> /* Count the struct value address, if it is passed as a parm. */
> + structure_value_addr_parm);
> else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
> n_named_args = 0;
> else
> /* If we know nothing, treat all args as named. */
> n_named_args = num_actuals;
> case, I think guarding it by any target hooks is wrong, although
> I guess it should have been
> n_named_args = structure_value_addr_parm;
> instead of
> n_named_args = 0;
>
> For the second
> if (type_arg_types != 0
> && targetm.calls.strict_argument_naming (args_so_far))
> ;
> else if (type_arg_types != 0
> && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
> /* Don't include the last named arg. */
> --n_named_args;
> else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
> n_named_args = 0;
> else
> /* Treat all args as named. */
> n_named_args = num_actuals;
> bet (but no testing done, don't even know which targets return what for
> those hooks) we should treat those as if type_arg_types was non-NULL
> with 0 elements in the list, except the --n_named_args doesn't make sense
> because that would decrease it to -1.
> So perhaps
> if ((type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
> && targetm.calls.strict_argument_naming (args_so_far))
> ;
> else if (type_arg_types != 0
> && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
> /* Don't include the last named arg. */
> --n_named_args;
> else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype)
> && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far)))
> ;
> else
> /* Treat all args as named. */
> n_named_args = num_actuals;
I tried the above on arm, aarch64 and x86_64 and that seems fine, including the new testcase you added.
R.
>
> (or n_named_args = 0; instead of ; before the final else? Dunno).
> I guess we need some testsuite coverage for caller/callee ABI match of
> struct S { char p[64]; };
> struct S foo (...);
>
> Jakub
>
On 29/02/2024 14:10, Richard Earnshaw (lists) wrote:
> On 27/02/2024 17:25, Jakub Jelinek wrote:
>> On Tue, Feb 27, 2024 at 04:41:32PM +0000, Richard Earnshaw wrote:
>>>> 2023-01-09 Jakub Jelinek <jakub@redhat.com>
>>>>
>>>> PR target/107453
>>>> * calls.cc (expand_call): For calls with
>>>> TYPE_NO_NAMED_ARGS_STDARG_P (funtype) use zero for n_named_args.
>>>> Formatting fix.
>>>
>>> This one has been festering for a while; both Alexandre and Torbjorn have attempted to fix it recently, but I'm not sure either is really right...
>>>
>>> On Arm this is causing all anonymous arguments to be passed on the stack,
>>> which is incorrect per the ABI. On a target that uses
>>> 'pretend_outgoing_vararg_named', why is it correct to set n_named_args to
>>> zero? Is it enough to guard both the statements you've added with
>>> !targetm.calls.pretend_outgoing_args_named?
>>
>> I'm afraid I haven't heard of that target hook before.
>> All I was doing with that change was fixing a regression reported in the PR
>> for ppc64le/sparc/nvptx/loongarch at least.
>>
>> The TYPE_NO_NAMED_ARGS_STDARG_P functions (C23 fns like void foo (...) {})
>> have NULL type_arg_types, so the list_length (type_arg_types) isn't done for
>> it, but it should be handled as if it was non-NULL but list length was 0.
>>
>> So, for the
>> if (type_arg_types != 0)
>> n_named_args
>> = (list_length (type_arg_types)
>> /* Count the struct value address, if it is passed as a parm. */
>> + structure_value_addr_parm);
>> else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
>> n_named_args = 0;
>> else
>> /* If we know nothing, treat all args as named. */
>> n_named_args = num_actuals;
>> case, I think guarding it by any target hooks is wrong, although
>> I guess it should have been
>> n_named_args = structure_value_addr_parm;
>> instead of
>> n_named_args = 0;
>>
>> For the second
>> if (type_arg_types != 0
>> && targetm.calls.strict_argument_naming (args_so_far))
>> ;
>> else if (type_arg_types != 0
>> && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
>> /* Don't include the last named arg. */
>> --n_named_args;
>> else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
>> n_named_args = 0;
>> else
>> /* Treat all args as named. */
>> n_named_args = num_actuals;
>> bet (but no testing done, don't even know which targets return what for
>> those hooks) we should treat those as if type_arg_types was non-NULL
>> with 0 elements in the list, except the --n_named_args doesn't make sense
>> because that would decrease it to -1.
>> So perhaps
>> if ((type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
>> && targetm.calls.strict_argument_naming (args_so_far))
>> ;
>> else if (type_arg_types != 0
>> && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
>> /* Don't include the last named arg. */
>> --n_named_args;
>> else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype)
>> && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far)))
>> ;
>> else
>> /* Treat all args as named. */
>> n_named_args = num_actuals;
>
> I tried the above on arm, aarch64 and x86_64 and that seems fine, including the new testcase you added.
>
I should mention though, that INIT_CUMULATIVE_ARGS on arm ignores n_named_args entirely, it doesn't need it (I don't think it even existed when the AAPCS code was added).
R.
> R.
>
>>
>> (or n_named_args = 0; instead of ; before the final else? Dunno).
>> I guess we need some testsuite coverage for caller/callee ABI match of
>> struct S { char p[64]; };
>> struct S foo (...);
>>
>> Jakub
>>
>
On Feb 27, 2024, Richard Earnshaw <Richard.Earnshaw@arm.com> wrote:
> This one has been festering for a while; both Alexandre and Torbjorn
> have attempted to fix it recently, but I'm not sure either is really
> right...
*nod* xref https://gcc.gnu.org/pipermail/gcc-patches/2024-March/646926.html
The patch I proposed was indeed far too limited in scope.
> On Arm this is causing all anonymous arguments to be passed on the
> stack, which is incorrect per the ABI. On a target that uses
> 'pretend_outgoing_vararg_named', why is it correct to set n_named_args
> to zero? Is it enough to guard both the statements you've added with
> !targetm.calls.pretend_outgoing_args_named?
ISTM that the change you suggest over Jakub's patch would address the
inconsistency on ARM.
Matthew suggested a patch along these lines in the other thread, that I
xrefed above, that seems sound to me, but I also suspect it won't fix
the ppc64le issue. My hunch is that we'll need a combination of both,
possibly with further tweaks to adjust for Jakub's just-added test.
On Fri, Mar 01, 2024 at 01:53:54AM -0300, Alexandre Oliva wrote:
> On Feb 27, 2024, Richard Earnshaw <Richard.Earnshaw@arm.com> wrote:
>
> > This one has been festering for a while; both Alexandre and Torbjorn
> > have attempted to fix it recently, but I'm not sure either is really
> > right...
>
> *nod* xref https://gcc.gnu.org/pipermail/gcc-patches/2024-March/646926.html
> The patch I proposed was indeed far too limited in scope.
>
> > On Arm this is causing all anonymous arguments to be passed on the
> > stack, which is incorrect per the ABI. On a target that uses
> > 'pretend_outgoing_vararg_named', why is it correct to set n_named_args
> > to zero? Is it enough to guard both the statements you've added with
> > !targetm.calls.pretend_outgoing_args_named?
>
> ISTM that the change you suggest over Jakub's patch would address the
> inconsistency on ARM.
At least in my understanding, the only part of my patch that was being
discussed was the !strict_argument_naming && !pretend_outgoing_args_named
case with structure_value_addr_parm, I don't see how that would affect
ARM, given that it is a !strict_argument_naming && pretend_outgoing_args_named
target. In that case with the patch as posted n_named_args will be
structure_value_addr_parm before INIT_CUMULATIVE_ARGS and num_actuals
afterwards, I don't see any disagreement on that.
Jakub
@@ -2908,8 +2908,8 @@ expand_call (tree exp, rtx target, int i
}
/* Count the arguments and set NUM_ACTUALS. */
- num_actuals =
- call_expr_nargs (exp) + num_complex_actuals + structure_value_addr_parm;
+ num_actuals
+ = call_expr_nargs (exp) + num_complex_actuals + structure_value_addr_parm;
/* Compute number of named args.
First, do a raw count of the args for INIT_CUMULATIVE_ARGS. */
@@ -2919,6 +2919,8 @@ expand_call (tree exp, rtx target, int i
= (list_length (type_arg_types)
/* Count the struct value address, if it is passed as a parm. */
+ structure_value_addr_parm);
+ else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
+ n_named_args = 0;
else
/* If we know nothing, treat all args as named. */
n_named_args = num_actuals;
@@ -2957,6 +2959,8 @@ expand_call (tree exp, rtx target, int i
&& ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
/* Don't include the last named arg. */
--n_named_args;
+ else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
+ n_named_args = 0;
else
/* Treat all args as named. */
n_named_args = num_actuals;