public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] i386: Fix setup of incoming varargs for (...) functions which return large aggregates [PR114175]
@ 2024-03-16  7:23 Jakub Jelinek
  2024-03-16 13:58 ` Richard Biener
  0 siblings, 1 reply; 2+ messages in thread
From: Jakub Jelinek @ 2024-03-16  7:23 UTC (permalink / raw)
  To: Joseph S. Myers, Richard Biener, Uros Bizjak; +Cc: gcc-patches

Hi!

The c23-stdarg-6.c testcase I've added recently apparently works fine with
-O0 but aborts with -O1 and higher on x86_64-linux.
The problem is in setup of incoming varargs.

Like function.cc before r14-9249 even ix86_setup_incoming_varargs assumes
that TYPE_NO_NAMED_ARGS_STDARG_P don't have any named arguments and there
is nothing to advance, but that is not the case for (...) functions
returning by hidden reference which have one such artificial argument.
If the setup_incoming_varargs hook is called from the
  if (TYPE_NO_NAMED_ARGS_STDARG_P (TREE_TYPE (fndecl))
      && fnargs.is_empty ())
    {
      struct assign_parm_data_one data = {};
      assign_parms_setup_varargs (&all, &data, false);
    }
spot, i.e. where there is no hidden return argument passed, arg.type
is always NULL, while when it is called in the
      if (cfun->stdarg && !DECL_CHAIN (parm))
        assign_parms_setup_varargs (&all, &data, false);
spot, even when it is TYPE_NO_NAMED_ARGS_STDARG_P arg.type will be non-NULL.
The tree-stdarg.cc pass in f in c23-stdarg-6.cc at -O1 or higher determines
that va_arg is used on integral types at most twice (loads 2 words),
and because ix86_setup_incoming_varargs doesn't advance, the code saves
just the %rdi and %rsi registers to the save area.  But that isn't correct,
it should save %rsi and %rdx because %rdi is the hidden return argument.
With -O0 tree-stdarg.cc doesn't attempt to optimize and we save all the
registers, so it works fine in that case.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux,
ok for trunk?

Now, I think we'll need the same fix also on
aarch64, alpha, arc, csky, ia64, loongarch, mips, mmix, nios2, riscv, visium
which have pretty much the similarly looking snippet in their hooks
changed by the r13-3549 commit.
Then arm, epiphany, fr30, frv, ft32, m32r, mcore, nds32, rs6000, sh
have different changes but most likely need something similar too.
I don't have access to most of those, could test aarch64 and rs6000 I guess.

2024-03-16  Jakub Jelinek  <jakub@redhat.com>

	PR target/114175
	* config/i386/i386.cc (ix86_setup_incoming_varargs): Only skip
	ix86_function_arg_advance for TYPE_NO_NAMED_ARGS_STDARG_P functions
	if arg.type is NULL.

	* gcc.dg/c23-stdarg-7.c: New test.
	* gcc.dg/c23-stdarg-8.c: New test.

--- gcc/config/i386/i386.cc.jj	2024-03-05 10:26:19.424029862 +0100
+++ gcc/config/i386/i386.cc	2024-03-15 23:50:08.821823213 +0100
@@ -4643,7 +4643,8 @@ ix86_setup_incoming_varargs (cumulative_
   /* For varargs, we do not want to skip the dummy va_dcl argument.
      For stdargs, we do want to skip the last named argument.  */
   next_cum = *cum;
-  if (!TYPE_NO_NAMED_ARGS_STDARG_P (TREE_TYPE (current_function_decl))
+  if ((!TYPE_NO_NAMED_ARGS_STDARG_P (TREE_TYPE (current_function_decl))
+       || arg.type != NULL_TREE)
       && stdarg_p (fntype))
     ix86_function_arg_advance (pack_cumulative_args (&next_cum), arg);
 
--- gcc/testsuite/gcc.dg/c23-stdarg-7.c.jj	2024-03-15 23:50:56.857185518 +0100
+++ gcc/testsuite/gcc.dg/c23-stdarg-7.c	2024-03-15 23:50:45.611334813 +0100
@@ -0,0 +1,6 @@
+/* Test C23 variadic functions with no named parameters, or last named
+   parameter with a declaration not allowed in C17.  Execution tests.  */
+/* { dg-do run } */
+/* { dg-options "-O2 -std=c23 -pedantic-errors" } */
+
+#include "c23-stdarg-4.c"
--- gcc/testsuite/gcc.dg/c23-stdarg-8.c.jj	2024-03-15 23:51:20.814867467 +0100
+++ gcc/testsuite/gcc.dg/c23-stdarg-8.c	2024-03-15 23:51:06.973051224 +0100
@@ -0,0 +1,6 @@
+/* Test C23 variadic functions with no named parameters, or last named
+   parameter with a declaration not allowed in C17.  Execution tests.  */
+/* { dg-do run } */
+/* { dg-options "-O2 -std=c23 -pedantic-errors" } */
+
+#include "c23-stdarg-6.c"

	Jakub


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] i386: Fix setup of incoming varargs for (...) functions which return large aggregates [PR114175]
  2024-03-16  7:23 [PATCH] i386: Fix setup of incoming varargs for (...) functions which return large aggregates [PR114175] Jakub Jelinek
@ 2024-03-16 13:58 ` Richard Biener
  0 siblings, 0 replies; 2+ messages in thread
From: Richard Biener @ 2024-03-16 13:58 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Joseph S. Myers, Uros Bizjak, gcc-patches



> Am 16.03.2024 um 08:24 schrieb Jakub Jelinek <jakub@redhat.com>:
> 
> Hi!
> 
> The c23-stdarg-6.c testcase I've added recently apparently works fine with
> -O0 but aborts with -O1 and higher on x86_64-linux.
> The problem is in setup of incoming varargs.
> 
> Like function.cc before r14-9249 even ix86_setup_incoming_varargs assumes
> that TYPE_NO_NAMED_ARGS_STDARG_P don't have any named arguments and there
> is nothing to advance, but that is not the case for (...) functions
> returning by hidden reference which have one such artificial argument.
> If the setup_incoming_varargs hook is called from the
>  if (TYPE_NO_NAMED_ARGS_STDARG_P (TREE_TYPE (fndecl))
>      && fnargs.is_empty ())
>    {
>      struct assign_parm_data_one data = {};
>      assign_parms_setup_varargs (&all, &data, false);
>    }
> spot, i.e. where there is no hidden return argument passed, arg.type
> is always NULL, while when it is called in the
>      if (cfun->stdarg && !DECL_CHAIN (parm))
>        assign_parms_setup_varargs (&all, &data, false);
> spot, even when it is TYPE_NO_NAMED_ARGS_STDARG_P arg.type will be non-NULL.
> The tree-stdarg.cc pass in f in c23-stdarg-6.cc at -O1 or higher determines
> that va_arg is used on integral types at most twice (loads 2 words),
> and because ix86_setup_incoming_varargs doesn't advance, the code saves
> just the %rdi and %rsi registers to the save area.  But that isn't correct,
> it should save %rsi and %rdx because %rdi is the hidden return argument.
> With -O0 tree-stdarg.cc doesn't attempt to optimize and we save all the
> registers, so it works fine in that case.
> 
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux,
> ok for trunk?

Ok

> Now, I think we'll need the same fix also on
> aarch64, alpha, arc, csky, ia64, loongarch, mips, mmix, nios2, riscv, visium
> which have pretty much the similarly looking snippet in their hooks
> changed by the r13-3549 commit.
> Then arm, epiphany, fr30, frv, ft32, m32r, mcore, nds32, rs6000, sh
> have different changes but most likely need something similar too.
> I don't have access to most of those, could test aarch64 and rs6000 I guess.

If we have testsuite coverage after this I’d say go ahead for unmaintained ports.

Richard 

> 2024-03-16  Jakub Jelinek  <jakub@redhat.com>
> 
>    PR target/114175
>    * config/i386/i386.cc (ix86_setup_incoming_varargs): Only skip
>    ix86_function_arg_advance for TYPE_NO_NAMED_ARGS_STDARG_P functions
>    if arg.type is NULL.
> 
>    * gcc.dg/c23-stdarg-7.c: New test.
>    * gcc.dg/c23-stdarg-8.c: New test.
> 
> --- gcc/config/i386/i386.cc.jj    2024-03-05 10:26:19.424029862 +0100
> +++ gcc/config/i386/i386.cc    2024-03-15 23:50:08.821823213 +0100
> @@ -4643,7 +4643,8 @@ ix86_setup_incoming_varargs (cumulative_
>   /* For varargs, we do not want to skip the dummy va_dcl argument.
>      For stdargs, we do want to skip the last named argument.  */
>   next_cum = *cum;
> -  if (!TYPE_NO_NAMED_ARGS_STDARG_P (TREE_TYPE (current_function_decl))
> +  if ((!TYPE_NO_NAMED_ARGS_STDARG_P (TREE_TYPE (current_function_decl))
> +       || arg.type != NULL_TREE)
>       && stdarg_p (fntype))
>     ix86_function_arg_advance (pack_cumulative_args (&next_cum), arg);
> 
> --- gcc/testsuite/gcc.dg/c23-stdarg-7.c.jj    2024-03-15 23:50:56.857185518 +0100
> +++ gcc/testsuite/gcc.dg/c23-stdarg-7.c    2024-03-15 23:50:45.611334813 +0100
> @@ -0,0 +1,6 @@
> +/* Test C23 variadic functions with no named parameters, or last named
> +   parameter with a declaration not allowed in C17.  Execution tests.  */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -std=c23 -pedantic-errors" } */
> +
> +#include "c23-stdarg-4.c"
> --- gcc/testsuite/gcc.dg/c23-stdarg-8.c.jj    2024-03-15 23:51:20.814867467 +0100
> +++ gcc/testsuite/gcc.dg/c23-stdarg-8.c    2024-03-15 23:51:06.973051224 +0100
> @@ -0,0 +1,6 @@
> +/* Test C23 variadic functions with no named parameters, or last named
> +   parameter with a declaration not allowed in C17.  Execution tests.  */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -std=c23 -pedantic-errors" } */
> +
> +#include "c23-stdarg-6.c"
> 
>    Jakub
> 

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2024-03-16 13:58 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-16  7:23 [PATCH] i386: Fix setup of incoming varargs for (...) functions which return large aggregates [PR114175] Jakub Jelinek
2024-03-16 13:58 ` Richard Biener

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).