* [PATCH] calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] @ 2023-01-09 10:32 Jakub Jelinek 2023-01-09 11:58 ` Richard Biener 2024-02-27 16:41 ` Richard Earnshaw 0 siblings, 2 replies; 19+ messages in thread From: Jakub Jelinek @ 2023-01-09 10:32 UTC (permalink / raw) To: Joseph S. Myers, Richard Biener, Jeff Law; +Cc: gcc-patches 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. --- 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2023-01-09 10:32 [PATCH] calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] Jakub Jelinek @ 2023-01-09 11:58 ` Richard Biener 2024-02-27 16:41 ` Richard Earnshaw 1 sibling, 0 replies; 19+ messages in thread From: Richard Biener @ 2023-01-09 11:58 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Joseph S. Myers, Jeff Law, gcc-patches 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 > > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg) ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2023-01-09 10:32 [PATCH] calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] Jakub Jelinek 2023-01-09 11:58 ` Richard Biener @ 2024-02-27 16:41 ` Richard Earnshaw 2024-02-27 17:25 ` Jakub Jelinek ` (2 more replies) 1 sibling, 3 replies; 19+ messages in thread From: Richard Earnshaw @ 2024-02-27 16:41 UTC (permalink / raw) To: Jakub Jelinek, Joseph S. Myers, Richard Biener, Jeff Law Cc: gcc-patches, Alexandre Ferreira, Torbjörn SVENSSON 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-02-27 16:41 ` Richard Earnshaw @ 2024-02-27 17:25 ` Jakub Jelinek 2024-02-27 17:54 ` Jakub Jelinek 2024-02-29 14:10 ` Richard Earnshaw (lists) 2024-02-27 17:25 ` [PATCH] calls: Fix up " Richard Earnshaw 2024-03-01 4:53 ` Alexandre Oliva 2 siblings, 2 replies; 19+ messages in thread From: Jakub Jelinek @ 2024-02-27 17:25 UTC (permalink / raw) To: Richard Earnshaw Cc: Joseph S. Myers, Richard Biener, Jeff Law, gcc-patches, Alexandre Ferreira, Torbjörn SVENSSON 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-02-27 17:25 ` Jakub Jelinek @ 2024-02-27 17:54 ` Jakub Jelinek 2024-02-28 8:31 ` Jakub Jelinek 2024-02-29 14:10 ` Richard Earnshaw (lists) 1 sibling, 1 reply; 19+ messages in thread From: Jakub Jelinek @ 2024-02-27 17:54 UTC (permalink / raw) To: Richard Earnshaw, Joseph S. Myers, Richard Biener, Jeff Law, gcc-patches, Alexandre Ferreira, Torbjörn SVENSSON 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-02-27 17:54 ` Jakub Jelinek @ 2024-02-28 8:31 ` Jakub Jelinek 0 siblings, 0 replies; 19+ messages in thread From: Jakub Jelinek @ 2024-02-28 8:31 UTC (permalink / raw) To: Richard Earnshaw, Joseph S. Myers, Richard Biener, Jeff Law, gcc-patches, Alexandre Ferreira, Torbjörn SVENSSON 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-02-27 17:25 ` Jakub Jelinek 2024-02-27 17:54 ` Jakub Jelinek @ 2024-02-29 14:10 ` Richard Earnshaw (lists) 2024-02-29 14:14 ` Richard Earnshaw 1 sibling, 1 reply; 19+ messages in thread From: Richard Earnshaw (lists) @ 2024-02-29 14:10 UTC (permalink / raw) To: Jakub Jelinek Cc: Joseph S. Myers, Richard Biener, Jeff Law, gcc-patches, Torbjörn SVENSSON, oliva 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-02-29 14:10 ` Richard Earnshaw (lists) @ 2024-02-29 14:14 ` Richard Earnshaw 2024-02-29 15:55 ` [PATCH] calls: Further fixes for " Jakub Jelinek 0 siblings, 1 reply; 19+ messages in thread From: Richard Earnshaw @ 2024-02-29 14:14 UTC (permalink / raw) To: Jakub Jelinek Cc: Joseph S. Myers, Richard Biener, Jeff Law, gcc-patches, Torbjörn SVENSSON, oliva 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 >> > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] calls: Further fixes for TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-02-29 14:14 ` Richard Earnshaw @ 2024-02-29 15:55 ` Jakub Jelinek 2024-02-29 17:23 ` Richard Earnshaw (lists) 2024-03-01 14:16 ` Richard Earnshaw (lists) 0 siblings, 2 replies; 19+ messages in thread From: Jakub Jelinek @ 2024-02-29 15:55 UTC (permalink / raw) To: Richard Earnshaw, Joseph S. Myers, Richard Biener, Jeff Law Cc: gcc-patches, Torbjörn SVENSSON, oliva On Thu, Feb 29, 2024 at 02:14:05PM +0000, Richard Earnshaw wrote: > > 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). So far I've just checked that the new testcase passes not just on x86_64/i686-linux, but also on {powerpc64le,s390x,aarch64}-linux with vanilla trunk. Haven't posted this patch in patch form, plus while I'm not really sure whether setting n_named_args to 0 or not changing in the !pretend_outgoing_varargs_named is right, the setting to 0 feels more correct to me. If structure_value_addr_parm is 1, the function effectively has a single named argument and then ... args and if the target wants n_named_args to be number of named arguments except the last, then that should be 0 rather than 1. Thus, is the following patch ok for trunk then? 2024-02-29 Jakub Jelinek <jakub@redhat.com> PR target/107453 * calls.cc (expand_call): For TYPE_NO_NAMED_ARGS_STDARG_P set n_named_args initially before INIT_CUMULATIVE_ARGS to structure_value_addr_parm rather than 0, after it don't modify it if strict_argument_naming and clear only if !pretend_outgoing_varargs_named. --- gcc/calls.cc.jj 2024-01-22 11:48:08.045847508 +0100 +++ gcc/calls.cc 2024-02-29 16:24:47.799855912 +0100 @@ -2938,7 +2938,7 @@ expand_call (tree exp, rtx target, int i /* 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; + n_named_args = structure_value_addr_parm; else /* If we know nothing, treat all args as named. */ n_named_args = num_actuals; @@ -2970,14 +2970,15 @@ expand_call (tree exp, rtx target, int i we do not have any reliable way to pass unnamed args in registers, so we must force them into memory. */ - if (type_arg_types != 0 + 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)) + else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype) + && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far)) n_named_args = 0; else /* Treat all args as named. */ Jakub ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Further fixes for TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-02-29 15:55 ` [PATCH] calls: Further fixes for " Jakub Jelinek @ 2024-02-29 17:23 ` Richard Earnshaw (lists) 2024-02-29 17:38 ` Jakub Jelinek 2024-03-01 14:16 ` Richard Earnshaw (lists) 1 sibling, 1 reply; 19+ messages in thread From: Richard Earnshaw (lists) @ 2024-02-29 17:23 UTC (permalink / raw) To: Jakub Jelinek, Joseph S. Myers, Richard Biener, Jeff Law Cc: gcc-patches, Torbjörn SVENSSON, oliva On 29/02/2024 15:55, Jakub Jelinek wrote: > On Thu, Feb 29, 2024 at 02:14:05PM +0000, Richard Earnshaw wrote: >>> 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). > > So far I've just checked that the new testcase passes not just on > x86_64/i686-linux, but also on {powerpc64le,s390x,aarch64}-linux > with vanilla trunk. > Haven't posted this patch in patch form, plus while I'm not really sure > whether setting n_named_args to 0 or not changing in the > !pretend_outgoing_varargs_named is right, the setting to 0 feels more > correct to me. If structure_value_addr_parm is 1, the function effectively > has a single named argument and then ... args and if the target wants > n_named_args to be number of named arguments except the last, then that > should be 0 rather than 1. > > Thus, is the following patch ok for trunk then? The comment at the start of the section says /* Now possibly adjust the number of named args. Normally, don't include the last named arg if anonymous args follow. We do include the last named arg if targetm.calls.strict_argument_naming() returns nonzero. (If no anonymous args follow, the result of list_length is actually one too large. This is harmless.) So in the case of strict_argument_naming perhaps it should return 1, but 0 for other cases. R. > > 2024-02-29 Jakub Jelinek <jakub@redhat.com> > > PR target/107453 > * calls.cc (expand_call): For TYPE_NO_NAMED_ARGS_STDARG_P set > n_named_args initially before INIT_CUMULATIVE_ARGS to > structure_value_addr_parm rather than 0, after it don't modify > it if strict_argument_naming and clear only if > !pretend_outgoing_varargs_named. > > --- gcc/calls.cc.jj 2024-01-22 11:48:08.045847508 +0100 > +++ gcc/calls.cc 2024-02-29 16:24:47.799855912 +0100 > @@ -2938,7 +2938,7 @@ expand_call (tree exp, rtx target, int i > /* 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; > + n_named_args = structure_value_addr_parm; > else > /* If we know nothing, treat all args as named. */ > n_named_args = num_actuals; > @@ -2970,14 +2970,15 @@ expand_call (tree exp, rtx target, int i > we do not have any reliable way to pass unnamed args in > registers, so we must force them into memory. */ > > - if (type_arg_types != 0 > + 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)) > + else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype) > + && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far)) > n_named_args = 0; > else > /* Treat all args as named. */ > > Jakub > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Further fixes for TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-02-29 17:23 ` Richard Earnshaw (lists) @ 2024-02-29 17:38 ` Jakub Jelinek 2024-02-29 17:51 ` Richard Earnshaw (lists) 0 siblings, 1 reply; 19+ messages in thread From: Jakub Jelinek @ 2024-02-29 17:38 UTC (permalink / raw) To: Richard Earnshaw (lists) Cc: Joseph S. Myers, Richard Biener, Jeff Law, gcc-patches, Torbjörn SVENSSON, oliva On Thu, Feb 29, 2024 at 05:23:25PM +0000, Richard Earnshaw (lists) wrote: > On 29/02/2024 15:55, Jakub Jelinek wrote: > > On Thu, Feb 29, 2024 at 02:14:05PM +0000, Richard Earnshaw wrote: > >>> 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). > > > > So far I've just checked that the new testcase passes not just on > > x86_64/i686-linux, but also on {powerpc64le,s390x,aarch64}-linux > > with vanilla trunk. > > Haven't posted this patch in patch form, plus while I'm not really sure > > whether setting n_named_args to 0 or not changing in the > > !pretend_outgoing_varargs_named is right, the setting to 0 feels more > > correct to me. If structure_value_addr_parm is 1, the function effectively > > has a single named argument and then ... args and if the target wants > > n_named_args to be number of named arguments except the last, then that > > should be 0 rather than 1. > > > > Thus, is the following patch ok for trunk then? > > The comment at the start of the section says > > /* Now possibly adjust the number of named args. > Normally, don't include the last named arg if anonymous args follow. > We do include the last named arg if > targetm.calls.strict_argument_naming() returns nonzero. > (If no anonymous args follow, the result of list_length is actually > one too large. This is harmless.) > > So in the case of strict_argument_naming perhaps it should return 1, but 0 for other cases. The TYPE_NO_NAMED_ARGS_STDARG_P (funtype) case is as if type_arg_types != 0 and list_length (type_arg_types) == 0, i.e. no user named arguments. As list_length (NULL) returns 0, perhaps it could be even handled just the by changing all the type_arg_types != 0 checks to type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype) There are just 2 cases I'm worried about, one is that I think rest of calls.cc nor the backends are prepared to see n_named_args -1 after the adjustments, I think it is better to use 0, and then the question is what the !strict_argument_naming && !pretend_outgoing_varargs_named case wants to do for the aggregate return. The patch as posted for void foo (...); void bar () { foo (1, 2, 3); } will set n_named_args initially to 0 (no named args) and with the adjustments for strict_argument_naming 0, otherwise for !pretend 0 as well, otherwise 3. For struct { char buf[4096]; } baz (...); void qux () { baz (1, 2, 3); } the patch sets n_named_args initially to 1 (the hidden return) and with the arguments for strict keep it at 1, for !pretend 0 and otherwise 3. So, which case do you think is handled incorrectly with that? Jakub ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Further fixes for TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-02-29 17:38 ` Jakub Jelinek @ 2024-02-29 17:51 ` Richard Earnshaw (lists) 2024-02-29 17:56 ` Jakub Jelinek 0 siblings, 1 reply; 19+ messages in thread From: Richard Earnshaw (lists) @ 2024-02-29 17:51 UTC (permalink / raw) To: Jakub Jelinek Cc: Joseph S. Myers, Richard Biener, Jeff Law, gcc-patches, Torbjörn SVENSSON, oliva On 29/02/2024 17:38, Jakub Jelinek wrote: > On Thu, Feb 29, 2024 at 05:23:25PM +0000, Richard Earnshaw (lists) wrote: >> On 29/02/2024 15:55, Jakub Jelinek wrote: >>> On Thu, Feb 29, 2024 at 02:14:05PM +0000, Richard Earnshaw wrote: >>>>> 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). >>> >>> So far I've just checked that the new testcase passes not just on >>> x86_64/i686-linux, but also on {powerpc64le,s390x,aarch64}-linux >>> with vanilla trunk. >>> Haven't posted this patch in patch form, plus while I'm not really sure >>> whether setting n_named_args to 0 or not changing in the >>> !pretend_outgoing_varargs_named is right, the setting to 0 feels more >>> correct to me. If structure_value_addr_parm is 1, the function effectively >>> has a single named argument and then ... args and if the target wants >>> n_named_args to be number of named arguments except the last, then that >>> should be 0 rather than 1. >>> >>> Thus, is the following patch ok for trunk then? >> >> The comment at the start of the section says >> >> /* Now possibly adjust the number of named args. >> Normally, don't include the last named arg if anonymous args follow. >> We do include the last named arg if >> targetm.calls.strict_argument_naming() returns nonzero. >> (If no anonymous args follow, the result of list_length is actually >> one too large. This is harmless.) >> >> So in the case of strict_argument_naming perhaps it should return 1, but 0 for other cases. > > The TYPE_NO_NAMED_ARGS_STDARG_P (funtype) case is as if type_arg_types != 0 > and list_length (type_arg_types) == 0, i.e. no user named arguments. > As list_length (NULL) returns 0, perhaps it could be even handled just the > by changing all the type_arg_types != 0 checks to > type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype) > There are just 2 cases I'm worried about, one is that I think rest of > calls.cc nor the backends are prepared to see n_named_args -1 after the > adjustments, I think it is better to use 0, and then the question is what > the !strict_argument_naming && !pretend_outgoing_varargs_named case > wants to do for the aggregate return. The patch as posted for > void foo (...); void bar () { foo (1, 2, 3); } > will set n_named_args initially to 0 (no named args) and with the > adjustments for strict_argument_naming 0, otherwise for !pretend > 0 as well, otherwise 3. > For > struct { char buf[4096]; } baz (...); void qux () { baz (1, 2, 3); } > the patch sets n_named_args initially to 1 (the hidden return) and > with the arguments for strict keep it at 1, for !pretend 0 and otherwise > 3. > > So, which case do you think is handled incorrectly with that? The way I was thinking about it (and testing it on Arm) was to look at n_named_args for the cases of a traditional varargs case, then reduce that by one (except it can't ever be negative). So for void f(...); void g(int, ...); struct S { int a[32]; }; struct S h (...); struct S i (int, ...); void a () { struct S x; f(1, 2, 3, 4); g(1, 2, 3, 4); x = h (1, 2, 3, 4); x = i (1, 2, 3, 4); } There are various permutations that could lead to answers of 0, 1, 2, 4 and 5 depending on how those various targets treat each case and how the result pointer address is handled. My suspicion is that for a target that has strict argument naming and the result pointer passed as a first argument, the answer for the 'h()' call should be 1, not zero. Oh, but wait! Perhaps that now falls into the initial 'if' clause and we never reach the point where you pick zero. So perhaps I'm worrying about nothing. R. > > Jakub > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Further fixes for TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-02-29 17:51 ` Richard Earnshaw (lists) @ 2024-02-29 17:56 ` Jakub Jelinek 2024-03-01 13:53 ` Richard Earnshaw (lists) 0 siblings, 1 reply; 19+ messages in thread From: Jakub Jelinek @ 2024-02-29 17:56 UTC (permalink / raw) To: Richard Earnshaw (lists) Cc: Joseph S. Myers, Richard Biener, Jeff Law, gcc-patches, Torbjörn SVENSSON, oliva On Thu, Feb 29, 2024 at 05:51:03PM +0000, Richard Earnshaw (lists) wrote: > Oh, but wait! Perhaps that now falls into the initial 'if' clause and we never reach the point where you pick zero. So perhaps I'm worrying about nothing. If you are worried about the + else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype) + && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far)) n_named_args = 0; case in the patch, we know at that point that the initial n_named_args is equal to structure_value_addr_parm, so either 0, in that case --n_named_args; would yield the undesirable negative value, so we want 0 instead; for that case we could as well just have ; in there instead of n_named_args = 0;, or it is 1, in that case --n_named_args; would turn that into 0. Jakub ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Further fixes for TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-02-29 17:56 ` Jakub Jelinek @ 2024-03-01 13:53 ` Richard Earnshaw (lists) 2024-03-01 14:00 ` Jakub Jelinek 0 siblings, 1 reply; 19+ messages in thread From: Richard Earnshaw (lists) @ 2024-03-01 13:53 UTC (permalink / raw) To: Jakub Jelinek Cc: Joseph S. Myers, Richard Biener, Jeff Law, gcc-patches, Torbjörn SVENSSON, oliva On 29/02/2024 17:56, Jakub Jelinek wrote: > On Thu, Feb 29, 2024 at 05:51:03PM +0000, Richard Earnshaw (lists) wrote: >> Oh, but wait! Perhaps that now falls into the initial 'if' clause and we never reach the point where you pick zero. So perhaps I'm worrying about nothing. > > If you are worried about the > + else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype) > + && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far)) > n_named_args = 0; > case in the patch, we know at that point that the initial n_named_args is > equal to structure_value_addr_parm, so either 0, in that case > --n_named_args; > would yield the undesirable negative value, so we want 0 instead; for that > case we could as well just have ; in there instead of n_named_args = 0;, > or it is 1, in that case --n_named_args; would turn that into 0. > > Jakub > No, I was thinking about the case of strict_argument_naming when the first argument is the artificial return value pointer. In that case we'd want n_named_args=1. But I think it's a non-issue as that will be caught by if ((type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype)) && targetm.calls.strict_argument_naming (args_so_far)) ; R. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Further fixes for TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-03-01 13:53 ` Richard Earnshaw (lists) @ 2024-03-01 14:00 ` Jakub Jelinek 0 siblings, 0 replies; 19+ messages in thread From: Jakub Jelinek @ 2024-03-01 14:00 UTC (permalink / raw) To: Richard Earnshaw (lists) Cc: Joseph S. Myers, Richard Biener, Jeff Law, gcc-patches, Torbjörn SVENSSON, oliva On Fri, Mar 01, 2024 at 01:53:08PM +0000, Richard Earnshaw (lists) wrote: > On 29/02/2024 17:56, Jakub Jelinek wrote: > > On Thu, Feb 29, 2024 at 05:51:03PM +0000, Richard Earnshaw (lists) wrote: > >> Oh, but wait! Perhaps that now falls into the initial 'if' clause and we never reach the point where you pick zero. So perhaps I'm worrying about nothing. > > > > If you are worried about the > > + else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype) > > + && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far)) > > n_named_args = 0; > > case in the patch, we know at that point that the initial n_named_args is > > equal to structure_value_addr_parm, so either 0, in that case > > --n_named_args; > > would yield the undesirable negative value, so we want 0 instead; for that > > case we could as well just have ; in there instead of n_named_args = 0;, > > or it is 1, in that case --n_named_args; would turn that into 0. > > > > Jakub > > > > No, I was thinking about the case of strict_argument_naming when the first argument is the artificial return value pointer. In that case we'd want n_named_args=1. > > But I think it's a non-issue as that will be caught by > > if ((type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype)) > && targetm.calls.strict_argument_naming (args_so_far)) > ; Yes, that for strict argument naming and calls to struct large_struct foo (...); with the patch we set n_named_args = 1 early: else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype)) n_named_args = structure_value_addr_parm; and then if ((type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype)) && targetm.calls.strict_argument_naming (args_so_far)) ; doesn't change it. Jakub ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Further fixes for TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-02-29 15:55 ` [PATCH] calls: Further fixes for " Jakub Jelinek 2024-02-29 17:23 ` Richard Earnshaw (lists) @ 2024-03-01 14:16 ` Richard Earnshaw (lists) 1 sibling, 0 replies; 19+ messages in thread From: Richard Earnshaw (lists) @ 2024-03-01 14:16 UTC (permalink / raw) To: Jakub Jelinek, Joseph S. Myers, Richard Biener, Jeff Law Cc: gcc-patches, Torbjörn SVENSSON, oliva On 29/02/2024 15:55, Jakub Jelinek wrote: > On Thu, Feb 29, 2024 at 02:14:05PM +0000, Richard Earnshaw wrote: >>> 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). > > So far I've just checked that the new testcase passes not just on > x86_64/i686-linux, but also on {powerpc64le,s390x,aarch64}-linux > with vanilla trunk. > Haven't posted this patch in patch form, plus while I'm not really sure > whether setting n_named_args to 0 or not changing in the > !pretend_outgoing_varargs_named is right, the setting to 0 feels more > correct to me. If structure_value_addr_parm is 1, the function effectively > has a single named argument and then ... args and if the target wants > n_named_args to be number of named arguments except the last, then that > should be 0 rather than 1. > > Thus, is the following patch ok for trunk then? > > 2024-02-29 Jakub Jelinek <jakub@redhat.com> > > PR target/107453 PR 114136 Would be more appropriate for this, I think. Otherwise, OK. R. > * calls.cc (expand_call): For TYPE_NO_NAMED_ARGS_STDARG_P set > n_named_args initially before INIT_CUMULATIVE_ARGS to > structure_value_addr_parm rather than 0, after it don't modify > it if strict_argument_naming and clear only if > !pretend_outgoing_varargs_named. > > --- gcc/calls.cc.jj 2024-01-22 11:48:08.045847508 +0100 > +++ gcc/calls.cc 2024-02-29 16:24:47.799855912 +0100 > @@ -2938,7 +2938,7 @@ expand_call (tree exp, rtx target, int i > /* 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; > + n_named_args = structure_value_addr_parm; > else > /* If we know nothing, treat all args as named. */ > n_named_args = num_actuals; > @@ -2970,14 +2970,15 @@ expand_call (tree exp, rtx target, int i > we do not have any reliable way to pass unnamed args in > registers, so we must force them into memory. */ > > - if (type_arg_types != 0 > + 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)) > + else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype) > + && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far)) > n_named_args = 0; > else > /* Treat all args as named. */ > > Jakub > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-02-27 16:41 ` Richard Earnshaw 2024-02-27 17:25 ` Jakub Jelinek @ 2024-02-27 17:25 ` Richard Earnshaw 2024-03-01 4:53 ` Alexandre Oliva 2 siblings, 0 replies; 19+ messages in thread From: Richard Earnshaw @ 2024-02-27 17:25 UTC (permalink / raw) To: Jakub Jelinek, Joseph S. Myers, Richard Biener, Jeff Law Cc: gcc-patches, Torbjörn SVENSSON, oliva [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 >> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-02-27 16:41 ` Richard Earnshaw 2024-02-27 17:25 ` Jakub Jelinek 2024-02-27 17:25 ` [PATCH] calls: Fix up " Richard Earnshaw @ 2024-03-01 4:53 ` Alexandre Oliva 2024-03-01 7:53 ` Jakub Jelinek 2 siblings, 1 reply; 19+ messages in thread From: Alexandre Oliva @ 2024-03-01 4:53 UTC (permalink / raw) To: Richard Earnshaw Cc: Jakub Jelinek, Joseph S. Myers, Richard Biener, Jeff Law, gcc-patches, Torbjörn SVENSSON, Matthew Malcomson 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. -- Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer Disinformation flourishes because many people care deeply about injustice but very few check the facts. Think Assange & Stallman. The empires strike back ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] 2024-03-01 4:53 ` Alexandre Oliva @ 2024-03-01 7:53 ` Jakub Jelinek 0 siblings, 0 replies; 19+ messages in thread From: Jakub Jelinek @ 2024-03-01 7:53 UTC (permalink / raw) To: Alexandre Oliva Cc: Richard Earnshaw, Joseph S. Myers, Richard Biener, Jeff Law, gcc-patches, Torbjörn SVENSSON, Matthew Malcomson 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2024-03-01 14:16 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-09 10:32 [PATCH] calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] Jakub Jelinek 2023-01-09 11:58 ` Richard Biener 2024-02-27 16:41 ` Richard Earnshaw 2024-02-27 17:25 ` Jakub Jelinek 2024-02-27 17:54 ` Jakub Jelinek 2024-02-28 8:31 ` Jakub Jelinek 2024-02-29 14:10 ` Richard Earnshaw (lists) 2024-02-29 14:14 ` Richard Earnshaw 2024-02-29 15:55 ` [PATCH] calls: Further fixes for " Jakub Jelinek 2024-02-29 17:23 ` Richard Earnshaw (lists) 2024-02-29 17:38 ` Jakub Jelinek 2024-02-29 17:51 ` Richard Earnshaw (lists) 2024-02-29 17:56 ` Jakub Jelinek 2024-03-01 13:53 ` Richard Earnshaw (lists) 2024-03-01 14:00 ` Jakub Jelinek 2024-03-01 14:16 ` Richard Earnshaw (lists) 2024-02-27 17:25 ` [PATCH] calls: Fix up " Richard Earnshaw 2024-03-01 4:53 ` Alexandre Oliva 2024-03-01 7:53 ` Jakub Jelinek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).