Hi Alexandre,
I don't have the ability to OK the patch, but I'm attempting to do a
review in
order to reduce the workload for any maintainer. (Apologies for the slow
response).
I think you're right that the AAPCS32 requires all arguments to be passed in
registers for this testcase.
(Nit on the commit-message: It says that your reading of the AAPCS32
suggests
that the *caller* is correct -- I believe based on the change you
suggested you
meant *callee* is correct in expecting arguments in registers.)
The approach you suggest looks OK to me -- I do notice that it doesn't
fix the
legacy ABI's of `atpcs` and `apcs` and guess it would be nicer to have them
working at the same time though would defer to maintainers on how
important that
is.
(For the benefit of others reading) I don't believe there is any ABI concern
with this since it's fixing something that is currently not working at
all and
only applies to c23 (so a change shouldn't have too much of an impact).
You mention you chose to make the change in the arm backend rather than
general
code due to hesitancy to change the generic ABI-affecting code. That makes
sense to me, certainly at this late stage in the development cycle.
That said, I do expect the more robust solution would be to change a part of
the generic ABI code and would like to check that this makes sense to
you and
anyone else upstream in the discussion (justification below).
So would suggest to the maintainers that maybe this goes in with a note to
remember to look at a possible generic code change later on.
Does that sound reasonable to you?
------------------------------
Justification for why a change to the generic ABI code would be better
in the
end:
IIUC (from the documentation of the hooks) `pretend_outgoing_varargs_named`
really should mean that `n_named_args = num_actuals`. That seems to be
what the
documentation for `strict_argument_naming` indicates should happen.
``` (Documentation for TARGET_STRICT_ARGUMENT_NAMING):
If it returns 'false', but 'TARGET_PRETEND_OUTGOING_VARARGS_NAMED' returns
'true', then all arguments are treated as named.
```
Because of this I guess that if `pretend_outgoing_varargs_named` only
means to
pretend the above *except* in the c23 0-named-args case that seems like
it would
be a bit awkward to account for in backends.
From a quick check on c23-stdarg-4.c it does look like the below change
ends up
with the same codegen as your patch (except in the case of those legacy
ABI's,
where the below does make the caller and callee ABI match AFAICT):
```
diff --git a/gcc/calls.cc b/gcc/calls.cc
index 01f44734743..0b302f633ed 100644
--- a/gcc/calls.cc
+++ b/gcc/calls.cc
@@ -2970,14 +2970,15 @@ expand_call (tree exp, rtx target, int ignore)
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. */
```
Do you agree that this makes sense (i.e. is there something I'm
completely missing)?
FWIW I attempted to try and find other targets which have
`strict_argument_naming` returning `false`, `pretend_outgoing_varargs_named`
returning true, and some use of the `.named` member of function
arguments. I
got the below list. I recognise it doesn't mean there's a bug in these
backends, but thought it might help the discussion.
(lm32 mcore msp430 gcn cris fr30 frv h8300 arm v850 rx pru)
On 1/23/24 08:26, Alexandre Oliva wrote:
On Dec 5, 2023, Alexandre Oliva <ol...@adacore.com> wrote:
arm: fix c23 0-named-args caller-side stdarg
Ping?
https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639472.html
The commit message doesn't name explicitly the fixed testsuite
failures. Here they are:
FAIL: gcc.dg/c23-stdarg-4.c execution test
FAIL: gcc.dg/torture/c23-stdarg-split-1a.c -O0 execution test
FAIL: gcc.dg/torture/c23-stdarg-split-1a.c -O1 execution test
FAIL: gcc.dg/torture/c23-stdarg-split-1a.c -O2 execution test
FAIL: gcc.dg/torture/c23-stdarg-split-1a.c -O2 -flto -fno-use-linker-plugin
-flto-partition=none execution test
FAIL: gcc.dg/torture/c23-stdarg-split-1a.c -O2 -flto -fuse-linker-plugin
-fno-fat-lto-objects execution test
FAIL: gcc.dg/torture/c23-stdarg-split-1a.c -O3 -g execution test
FAIL: gcc.dg/torture/c23-stdarg-split-1a.c -Os execution test
Tested on arm-eabi. Ok to install?
