diff mbox

Fix va_start on x86_64 after any named argument passed on the stack that needed padding (PR target/44942)

Message ID 20100715160545.GQ20208@tyan-ft48-01.lab.bos.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek July 15, 2010, 4:05 p.m. UTC 
Hi!

When say one integer argument is passed on the stack (because all regs are
already used by earlier arguments) and then long double is passed, it is
padded on the stack to 16 byte boundary on x86-64 (similarly for a bunch of
other argument types).  Unfortunately cum->words isn't adjusted for this
padding and thus ap.overflow_arg_area is set to a wrong address.

It seems cum->words is only ever used by ix86_va_start (for -m64 only
and not for MS ABI), so this patch shouldn't affect anything but
__builtin_va_start expansion.

Bootstrapped/regtested on x86_64-linux and i686-linux, tested with
compat.exp and struct-layout-1.exp also against older gcc in
ALT_CC_UNDER_TEST/ALT_CXX_UNDER_TEST.  Ok for trunk?

What about release branches after a while on the trunk? 
While this probably isn't a regression, it is quite bad wrong code bug.

2010-07-15  Jakub Jelinek  <jakub@redhat.com>

	PR target/44942
	* config/i386/i386-protos.h (ix86_function_arg_boundary): Change second
	argument to const_tree.
	* config/i386/i386.c (function_arg_advance): If padding needs to be
	inserted before argument, increment cum->words by number of padding
	words as well.
	(contains_aligned_value_p): Change argument to const_tree.
	(ix86_function_arg_boundary): Change second argument to const_tree.

	* gcc.c-torture/execute/pr44942.c: New test.
	* gcc.target/i386/pr44942.c: New test.


	Jakub

Comments

Uros Bizjak July 16, 2010, 8:46 a.m. UTC | #1 
On Thu, Jul 15, 2010 at 6:05 PM, Jakub Jelinek <jakub@redhat.com> wrote:

> When say one integer argument is passed on the stack (because all regs are
> already used by earlier arguments) and then long double is passed, it is
> padded on the stack to 16 byte boundary on x86-64 (similarly for a bunch of
> other argument types).  Unfortunately cum->words isn't adjusted for this
> padding and thus ap.overflow_arg_area is set to a wrong address.
>
> It seems cum->words is only ever used by ix86_va_start (for -m64 only
> and not for MS ABI), so this patch shouldn't affect anything but
> __builtin_va_start expansion.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, tested with
> compat.exp and struct-layout-1.exp also against older gcc in
> ALT_CC_UNDER_TEST/ALT_CXX_UNDER_TEST.  Ok for trunk?
>
> What about release branches after a while on the trunk?
> While this probably isn't a regression, it is quite bad wrong code bug.
>
> 2010-07-15  Jakub Jelinek  <jakub@redhat.com>
>
>        PR target/44942
>        * config/i386/i386-protos.h (ix86_function_arg_boundary): Change second
>        argument to const_tree.
>        * config/i386/i386.c (function_arg_advance): If padding needs to be
>        inserted before argument, increment cum->words by number of padding
>        words as well.
>        (contains_aligned_value_p): Change argument to const_tree.
>        (ix86_function_arg_boundary): Change second argument to const_tree.
>
>        * gcc.c-torture/execute/pr44942.c: New test.
>        * gcc.target/i386/pr44942.c: New test.

This is OK for mainline and branches (after a couple of days,
perhaps). Even if it is not a regression, the change is well isolated
and without impact on other compiler functionality, so I guess that
wrong-code fix outweights relatively small risk.

Thanks,
Uros.
Jan Hubicka July 16, 2010, 8:57 a.m. UTC | #2 
> This is OK for mainline and branches (after a couple of days,
> perhaps). Even if it is not a regression, the change is well isolated
> and without impact on other compiler functionality, so I guess that
> wrong-code fix outweights relatively small risk.

Agreed, we always tried to backport API breakages back to branches as  
soon as possible.
Thanks!
Honza
>
> Thanks,
> Uros.
>
diff mbox

Patch

--- gcc/config/i386/i386-protos.h.jj	2010-07-13 15:56:31.000000000 +0200
+++ gcc/config/i386/i386-protos.h	2010-07-15 12:45:01.000000000 +0200
@@ -140,8 +140,8 @@  extern enum machine_mode ix86_fp_compare
 extern rtx ix86_libcall_value (enum machine_mode);
 extern bool ix86_function_arg_regno_p (int);
 extern void ix86_asm_output_function_label (FILE *, const char *, tree);
-extern int ix86_function_arg_boundary (enum machine_mode, tree);
-extern bool ix86_solaris_return_in_memory (const_tree,const_tree);
+extern int ix86_function_arg_boundary (enum machine_mode, const_tree);
+extern bool ix86_solaris_return_in_memory (const_tree, const_tree);
 extern rtx ix86_force_to_memory (enum machine_mode, rtx);
 extern void ix86_free_from_memory (enum machine_mode);
 extern enum calling_abi ix86_cfun_abi (void);
--- gcc/config/i386/i386.c.jj	2010-07-13 15:56:31.000000000 +0200
+++ gcc/config/i386/i386.c	2010-07-15 12:44:31.000000000 +0200
@@ -6157,9 +6157,8 @@  function_arg_advance_64 (CUMULATIVE_ARGS
   if (!named && VALID_AVX256_REG_MODE (mode))
     return;
 
-  if (!examine_argument (mode, type, 0, &int_nregs, &sse_nregs))
-    cum->words += words;
-  else if (sse_nregs <= cum->sse_nregs && int_nregs <= cum->nregs)
+  if (examine_argument (mode, type, 0, &int_nregs, &sse_nregs)
+      && sse_nregs <= cum->sse_nregs && int_nregs <= cum->nregs)
     {
       cum->nregs -= int_nregs;
       cum->sse_nregs -= sse_nregs;
@@ -6167,7 +6166,11 @@  function_arg_advance_64 (CUMULATIVE_ARGS
       cum->sse_regno += sse_nregs;
     }
   else
-    cum->words += words;
+    {
+      int align = ix86_function_arg_boundary (mode, type) / BITS_PER_WORD;
+      cum->words = (cum->words + align - 1) & ~(align - 1);
+      cum->words += words;
+    }
 }
 
 static void
@@ -6508,7 +6511,7 @@  ix86_pass_by_reference (CUMULATIVE_ARGS 
 /* Return true when TYPE should be 128bit aligned for 32bit argument passing
    ABI.  */
 static bool
-contains_aligned_value_p (tree type)
+contains_aligned_value_p (const_tree type)
 {
   enum machine_mode mode = TYPE_MODE (type);
   if (((TARGET_SSE && SSE_REG_MODE_P (mode))
@@ -6558,7 +6561,7 @@  contains_aligned_value_p (tree type)
    specified mode and type.  */
 
 int
-ix86_function_arg_boundary (enum machine_mode mode, tree type)
+ix86_function_arg_boundary (enum machine_mode mode, const_tree type)
 {
   int align;
   if (type)
--- gcc/testsuite/gcc.c-torture/execute/pr44942.c.jj	2010-07-15 13:41:28.000000000 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr44942.c	2010-07-15 13:46:40.000000000 +0200
@@ -0,0 +1,70 @@ 
+/* PR target/44942 */
+
+#include <stdarg.h>
+
+void
+test1 (int a, int b, int c, int d, int e, int f, int g, long double h, ...)
+{
+  int i;
+  va_list ap;
+
+  va_start (ap, h);
+  i = va_arg (ap, int);
+  if (i != 1234)
+    __builtin_abort ();
+  va_end (ap);
+}
+
+void
+test2 (int a, int b, int c, int d, int e, int f, int g, long double h, int i,
+       long double j, int k, long double l, int m, long double n, ...)
+{
+  int o;
+  va_list ap;
+
+  va_start (ap, n);
+  o = va_arg (ap, int);
+  if (o != 1234)
+    __builtin_abort ();
+  va_end (ap);
+}
+
+void
+test3 (double a, double b, double c, double d, double e, double f,
+       double g, long double h, ...)
+{
+  double i;
+  va_list ap;
+
+  va_start (ap, h);
+  i = va_arg (ap, double);
+  if (i != 1234.0)
+    __builtin_abort ();
+  va_end (ap);
+}
+
+void
+test4 (double a, double b, double c, double d, double e, double f, double g,
+       long double h, double i, long double j, double k, long double l,
+       double m, long double n, ...)
+{
+  double o;
+  va_list ap;
+
+  va_start (ap, n);
+  o = va_arg (ap, double);
+  if (o != 1234.0)
+    __builtin_abort ();
+  va_end (ap);
+}
+
+int
+main ()
+{
+  test1 (0, 0, 0, 0, 0, 0, 0, 0.0L, 1234);
+  test2 (0, 0, 0, 0, 0, 0, 0, 0.0L, 0, 0.0L, 0, 0.0L, 0, 0.0L, 1234);
+  test3 (0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0L, 1234.0);
+  test4 (0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0L, 0.0, 0.0L,
+	 0.0, 0.0L, 0.0, 0.0L, 1234.0);
+  return 0;
+}
--- gcc/testsuite/gcc.target/i386/pr44942.c.jj	2010-07-15 13:52:37.000000000 +0200
+++ gcc/testsuite/gcc.target/i386/pr44942.c	2010-07-15 13:53:24.000000000 +0200
@@ -0,0 +1,44 @@ 
+/* PR target/44942 */
+/* { dg-do run { target lp64 } } */
+
+#include <stdarg.h>
+#include <emmintrin.h>
+
+void
+test1 (double a, double b, double c, double d, double e, double f,
+       double g, __m128d h, ...)
+{
+  double i;
+  va_list ap;
+
+  va_start (ap, h);
+  i = va_arg (ap, double);
+  if (i != 1234.0)
+    __builtin_abort ();
+  va_end (ap);
+}
+
+void
+test2 (double a, double b, double c, double d, double e, double f, double g,
+       __m128d h, double i, __m128d j, double k, __m128d l,
+       double m, __m128d n, ...)
+{
+  double o;
+  va_list ap;
+
+  va_start (ap, n);
+  o = va_arg (ap, double);
+  if (o != 1234.0)
+    __builtin_abort ();
+  va_end (ap);
+}
+
+int
+main ()
+{
+  __m128d m = _mm_set1_pd (7.0);
+  test1 (0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, m, 1234.0);
+  test2 (0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, m, 0.0, m,
+	 0.0, m, 0.0, m, 1234.0);
+  return 0;
+}