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