Re: [PATCH] calls: Further fixes for TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453]

[Jakub Jelinek <jakub@redhat.com>]

On Thu, Feb 29, 2024 at 05:23:25PM +0000, Richard Earnshaw (lists) wrote:
> On 29/02/2024 15:55, Jakub Jelinek wrote:
> > On Thu, Feb 29, 2024 at 02:14:05PM +0000, Richard Earnshaw wrote:
> >>> 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).
> > 
> > So far I've just checked that the new testcase passes not just on
> > x86_64/i686-linux, but also on {powerpc64le,s390x,aarch64}-linux
> > with vanilla trunk.
> > Haven't posted this patch in patch form, plus while I'm not really sure
> > whether setting n_named_args to 0 or not changing in the
> > !pretend_outgoing_varargs_named is right, the setting to 0 feels more
> > correct to me.  If structure_value_addr_parm is 1, the function effectively
> > has a single named argument and then ... args and if the target wants
> > n_named_args to be number of named arguments except the last, then that
> > should be 0 rather than 1.
> > 
> > Thus, is the following patch ok for trunk then?
> 
> The comment at the start of the section says
> 
>   /* Now possibly adjust the number of named args.
>      Normally, don't include the last named arg if anonymous args follow.
>      We do include the last named arg if
>      targetm.calls.strict_argument_naming() returns nonzero.
>      (If no anonymous args follow, the result of list_length is actually
>      one too large.  This is harmless.)
> 
> So in the case of strict_argument_naming perhaps it should return 1, but 0 for other cases.

The TYPE_NO_NAMED_ARGS_STDARG_P (funtype) case is as if type_arg_types != 0
and list_length (type_arg_types) == 0, i.e. no user named arguments.
As list_length (NULL) returns 0, perhaps it could be even handled just the
by changing all the type_arg_types != 0 checks to
type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype)
There are just 2 cases I'm worried about, one is that I think rest of
calls.cc nor the backends are prepared to see n_named_args -1 after the
adjustments, I think it is better to use 0, and then the question is what
the !strict_argument_naming && !pretend_outgoing_varargs_named case
wants to do for the aggregate return.  The patch as posted for
void foo (...); void bar () { foo (1, 2, 3); }
will set n_named_args initially to 0 (no named args) and with the
adjustments for strict_argument_naming 0, otherwise for !pretend
0 as well, otherwise 3.
For
struct { char buf[4096]; } baz (...); void qux () { baz (1, 2, 3); }
the patch sets n_named_args initially to 1 (the hidden return) and
with the arguments for strict keep it at 1, for !pretend 0 and otherwise
3.

So, which case do you think is handled incorrectly with that?

	Jakub