Re: [PATCH] calls: Fix up TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453]

["Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com>]

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
>