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 <ja...@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
>
