Re: [PATCH] arm: fix c23 0-named-args caller-side stdarg

[Matthew Malcomson <matthew.malcomson@arm.com>]

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 <oliva@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?
>