diff mbox series

calls: Further fixes for TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453]

Message ID ZeCpAhVThf3C6SKl@tucnak
State New
Headers show
Series calls: Further fixes for TYPE_NO_NAMED_ARGS_STDARG_P handling [PR107453] | expand

Commit Message

Jakub Jelinek Feb. 29, 2024, 3:55 p.m. UTC 
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?

2024-02-29  Jakub Jelinek  <jakub@redhat.com>

	PR target/107453
	* calls.cc (expand_call): For TYPE_NO_NAMED_ARGS_STDARG_P set
	n_named_args initially before INIT_CUMULATIVE_ARGS to
	structure_value_addr_parm rather than 0, after it don't modify
	it if strict_argument_naming and clear only if
	!pretend_outgoing_varargs_named.


	Jakub

Comments

Richard Earnshaw (lists) Feb. 29, 2024, 5:23 p.m. UTC | #1 
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.

R.

> 
> 2024-02-29  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/107453
> 	* calls.cc (expand_call): For TYPE_NO_NAMED_ARGS_STDARG_P set
> 	n_named_args initially before INIT_CUMULATIVE_ARGS to
> 	structure_value_addr_parm rather than 0, after it don't modify
> 	it if strict_argument_naming and clear only if
> 	!pretend_outgoing_varargs_named.
> 
> --- gcc/calls.cc.jj	2024-01-22 11:48:08.045847508 +0100
> +++ gcc/calls.cc	2024-02-29 16:24:47.799855912 +0100
> @@ -2938,7 +2938,7 @@ expand_call (tree exp, rtx target, int i
>  	 /* 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;
> +    n_named_args = structure_value_addr_parm;
>    else
>      /* If we know nothing, treat all args as named.  */
>      n_named_args = num_actuals;
> @@ -2970,14 +2970,15 @@ expand_call (tree exp, rtx target, int i
>       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.  */
> 
> 	Jakub
>
Jakub Jelinek Feb. 29, 2024, 5:38 p.m. UTC | #2 
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
Richard Earnshaw (lists) Feb. 29, 2024, 5:51 p.m. UTC | #3 
On 29/02/2024 17:38, Jakub Jelinek wrote:
> 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?

The way I was thinking about it (and testing it on Arm) was to look at n_named_args for the cases of a traditional varargs case, then reduce that by one (except it can't ever be negative).

So for 

void f(...);
void g(int, ...);
struct S { int a[32]; };

struct S h (...);
struct S i (int, ...);

void a ()
{
  struct S x;
  f(1, 2, 3, 4);
  g(1, 2, 3, 4);
  x = h (1, 2, 3, 4);
  x = i (1, 2, 3, 4);
}

There are various permutations that could lead to answers of 0, 1, 2, 4 and 5 depending on how those various targets treat each case and how the result pointer address is handled.  My suspicion is that for a target that has strict argument naming and the result pointer passed as a first argument, the answer for the 'h()' call should be 1, not zero.  

Oh, but wait!  Perhaps that now falls into the initial 'if' clause and we never reach the point where you pick zero.  So perhaps I'm worrying about nothing.

R.

> 
> 	Jakub
>
Jakub Jelinek Feb. 29, 2024, 5:56 p.m. UTC | #4 
On Thu, Feb 29, 2024 at 05:51:03PM +0000, Richard Earnshaw (lists) wrote:
> Oh, but wait!  Perhaps that now falls into the initial 'if' clause and we never reach the point where you pick zero.  So perhaps I'm worrying about nothing.

If you are worried about the
+  else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype)
+          && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
     n_named_args = 0;
case in the patch, we know at that point that the initial n_named_args is
equal to structure_value_addr_parm, so either 0, in that case
    --n_named_args;
would yield the undesirable negative value, so we want 0 instead; for that
case we could as well just have ; in there instead of n_named_args = 0;,
or it is 1, in that case --n_named_args; would turn that into 0.

	Jakub
Richard Earnshaw (lists) March 1, 2024, 1:53 p.m. UTC | #5 
On 29/02/2024 17:56, Jakub Jelinek wrote:
> On Thu, Feb 29, 2024 at 05:51:03PM +0000, Richard Earnshaw (lists) wrote:
>> Oh, but wait!  Perhaps that now falls into the initial 'if' clause and we never reach the point where you pick zero.  So perhaps I'm worrying about nothing.
> 
> If you are worried about the
> +  else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype)
> +          && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
>      n_named_args = 0;
> case in the patch, we know at that point that the initial n_named_args is
> equal to structure_value_addr_parm, so either 0, in that case
>     --n_named_args;
> would yield the undesirable negative value, so we want 0 instead; for that
> case we could as well just have ; in there instead of n_named_args = 0;,
> or it is 1, in that case --n_named_args; would turn that into 0.
> 
> 	Jakub
> 

No, I was thinking about the case of strict_argument_naming when the first argument is the artificial return value pointer.  In that case we'd want n_named_args=1.

But I think it's a non-issue as that will be caught by 

  if ((type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
       && targetm.calls.strict_argument_naming (args_so_far))
     ;

R.
Jakub Jelinek March 1, 2024, 2 p.m. UTC | #6 
On Fri, Mar 01, 2024 at 01:53:08PM +0000, Richard Earnshaw (lists) wrote:
> On 29/02/2024 17:56, Jakub Jelinek wrote:
> > On Thu, Feb 29, 2024 at 05:51:03PM +0000, Richard Earnshaw (lists) wrote:
> >> Oh, but wait!  Perhaps that now falls into the initial 'if' clause and we never reach the point where you pick zero.  So perhaps I'm worrying about nothing.
> > 
> > If you are worried about the
> > +  else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype)
> > +          && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far))
> >      n_named_args = 0;
> > case in the patch, we know at that point that the initial n_named_args is
> > equal to structure_value_addr_parm, so either 0, in that case
> >     --n_named_args;
> > would yield the undesirable negative value, so we want 0 instead; for that
> > case we could as well just have ; in there instead of n_named_args = 0;,
> > or it is 1, in that case --n_named_args; would turn that into 0.
> > 
> > 	Jakub
> > 
> 
> No, I was thinking about the case of strict_argument_naming when the first argument is the artificial return value pointer.  In that case we'd want n_named_args=1.
> 
> But I think it's a non-issue as that will be caught by 
> 
>   if ((type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
>        && targetm.calls.strict_argument_naming (args_so_far))
>      ;

Yes, that for strict argument naming and calls to
struct large_struct foo (...);
with the patch we set n_named_args = 1 early:
  else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
    n_named_args = structure_value_addr_parm;
and then
  if ((type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype))
       && targetm.calls.strict_argument_naming (args_so_far))
    ;
doesn't change it.

	Jakub
Richard Earnshaw (lists) March 1, 2024, 2:16 p.m. UTC | #7 
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?
> 
> 2024-02-29  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/107453

PR 114136

Would be more appropriate for this, I think.

Otherwise, OK.

R.

> 	* calls.cc (expand_call): For TYPE_NO_NAMED_ARGS_STDARG_P set
> 	n_named_args initially before INIT_CUMULATIVE_ARGS to
> 	structure_value_addr_parm rather than 0, after it don't modify
> 	it if strict_argument_naming and clear only if
> 	!pretend_outgoing_varargs_named.
> 
> --- gcc/calls.cc.jj	2024-01-22 11:48:08.045847508 +0100
> +++ gcc/calls.cc	2024-02-29 16:24:47.799855912 +0100
> @@ -2938,7 +2938,7 @@ expand_call (tree exp, rtx target, int i
>  	 /* 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;
> +    n_named_args = structure_value_addr_parm;
>    else
>      /* If we know nothing, treat all args as named.  */
>      n_named_args = num_actuals;
> @@ -2970,14 +2970,15 @@ expand_call (tree exp, rtx target, int i
>       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.  */
> 
> 	Jakub
>
diff mbox series

Patch

--- gcc/calls.cc.jj	2024-01-22 11:48:08.045847508 +0100
+++ gcc/calls.cc	2024-02-29 16:24:47.799855912 +0100
@@ -2938,7 +2938,7 @@  expand_call (tree exp, rtx target, int i
 	 /* 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;
+    n_named_args = structure_value_addr_parm;
   else
     /* If we know nothing, treat all args as named.  */
     n_named_args = num_actuals;
@@ -2970,14 +2970,15 @@  expand_call (tree exp, rtx target, int i
      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.  */