PR middle-end/24306 revisited: va_arg and zero-sized objects

[Richard Sandiford <rdsandiford at googlemail dot com>]

[ Nick, you might remember mentioning this bug a few months back.
  I think I've finally got a proper fix, rather than the failed
  attempt I originally sent. ]

2005-12-20  Richard Guenther  <rguenther@suse.de>

	PR middle-end/24306
	* builtins.c (std_gimplify_va_arg_expr): Do not align
	va frame for zero sized types.
	* config/i386/i386.c (ix86_gimplify_va_arg): Likewise.

made va_arg ignore function_arg_boundary for zero-sized types.
I think this was due to the x86_64 definition of function_arg_advance:

static void
function_arg_advance_64 (CUMULATIVE_ARGS *cum, enum machine_mode mode,
			 const_tree type, HOST_WIDE_INT words, bool named)
{
  int int_nregs, sse_nregs;

  /* Unnamed 256bit vector mode parameters are passed on stack.  */
  if (!named && VALID_AVX256_REG_MODE (mode))
    return;

  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;
      cum->regno += int_nregs;
      cum->sse_regno += sse_nregs;
    }
  else
    {
      int align = ix86_function_arg_boundary (mode, type) / BITS_PER_WORD;
      cum->words = (cum->words + align - 1) & ~(align - 1);
      cum->words += words;
    }
}

which does indeed ignore ix86_function_arg_boundary for zero-sized
objects within the regparm range.  But the target-independent handling
of stack arguments doesn't have a similar check, so things go wrong
with 6+ arguments.  See the testcase below, which fails at count == 6
on x86_64-linux-gnu.

I've just realised this is also the cause of a MIPS bug that Nick pointed
me at a while ago, and also the cause of the mips-sde-elf struct-layout-1
failures.  The MIPS ABIs effectively treat the register parameters
as a memory image, and alignment is always honoured:

static void
mips_get_arg_info (struct mips_arg_info *info, const CUMULATIVE_ARGS *cum,
		   enum machine_mode mode, const_tree type, bool named)
{
  [...]
  /* See whether the argument has doubleword alignment.  */
  doubleword_aligned_p = (mips_function_arg_boundary (mode, type)
			  > BITS_PER_WORD);

So MIPS wants the original behaviour of always aligning.

[ The reason the struct-layout-1 tests didn't fail for mips*-linux-gnu
  is that the tests were passing a long double after the 16-byte aligned
  type.  n32 and n64 long doubles are usually 16 bytes and usually have
  16-byte alignment, so that alignment was saving us.  But mips-sde-elf
  uses a variation of n32 in which long doubles are only 8 bytes. ]

The easiest way of fixing this for MIPS without affecting other targets
seemed to be to define a std_gimplify_va_arg_expr_always_align function
to go alongside std_gimplify_va_arg.  This avoids duplicating the whole
function in the MIPS backend.  The PR trail suggests that this might be
a problem for PowerPC too, so maybe other targets would want to use it.

Tested on mips64-linux-gnu, mips-sde-elf and x86_64-linux-gnu.
OK to install?

What about x86_64?  The test fails before and after the patch,
so should I XFAIL it there for now?  I certainly don't know enough
about the x86_64 ABI to fix it myself.

Thanks,
Richard


gcc/
	PR middle-end/24306
	* tree.h (std_gimplify_va_arg_expr_always_align): Declare.
	* builtins.c (std_gimplify_va_arg_expr_1): New function,
	split out from...
	(std_gimplify_va_arg_expr): ...here.
	(std_gimplify_va_arg_expr_always_align): New function.
	* config/mips/mips.c (mips_gimplify_va_arg_expr): Call it
	instead of std_gimplify_va_arg_expr.

gcc/testsuite/
	PR middle-end/24306
	* gcc.dg/va-arg-6.c: New test.

Index: gcc/tree.h
===================================================================
--- gcc/tree.h	2012-01-29 11:15:28.000000000 +0000
+++ gcc/tree.h	2012-01-29 11:27:25.000000000 +0000
@@ -5442,6 +5442,8 @@ extern tree build_call_expr (tree, int,
 extern tree mathfn_built_in (tree, enum built_in_function fn);
 extern tree c_strlen (tree, int);
 extern tree std_gimplify_va_arg_expr (tree, tree, gimple_seq *, gimple_seq *);
+extern tree std_gimplify_va_arg_expr_always_align (tree, tree, gimple_seq *,
+						   gimple_seq *);
 extern tree build_va_arg_indirect_ref (tree);
 extern tree build_string_literal (int, const char *);
 extern bool validate_arglist (const_tree, ...);
Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	2012-01-29 11:15:28.000000000 +0000
+++ gcc/builtins.c	2012-01-29 11:27:25.000000000 +0000
@@ -4229,12 +4229,13 @@ expand_builtin_va_start (tree exp)
   return const0_rtx;
 }
 
-/* The "standard" implementation of va_arg: read the value from the
-   current (padded) address and increment by the (padded) size.  */
-
-tree
-std_gimplify_va_arg_expr (tree valist, tree type, gimple_seq *pre_p,
-			  gimple_seq *post_p)
+/* A subroutine of std_gimplify_va_arg_expr and
+   std_gimplify_va_arg_expr_always_align, with ALWAYS_ALIGN_P
+   distinguishing between them.  */
+
+static tree
+std_gimplify_va_arg_expr_1 (tree valist, tree type, gimple_seq *pre_p,
+			    gimple_seq *post_p, bool always_align_p)
 {
   tree addr, t, type_size, rounded_size, valist_tmp;
   unsigned HOST_WIDE_INT align, boundary;
@@ -4269,7 +4270,7 @@ std_gimplify_va_arg_expr (tree valist, t
   /* va_list pointer is aligned to PARM_BOUNDARY.  If argument actually
      requires greater alignment, we must perform dynamic alignment.  */
   if (boundary > align
-      && !integer_zerop (TYPE_SIZE (type)))
+      && (always_align_p || !integer_zerop (TYPE_SIZE (type))))
     {
       t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist_tmp,
 		  fold_build_pointer_plus_hwi (valist_tmp, boundary - 1));
@@ -4326,6 +4327,26 @@ std_gimplify_va_arg_expr (tree valist, t
   return build_va_arg_indirect_ref (addr);
 }
 
+/* The "standard" implementation of va_arg: read the value from the
+   current (padded) address and increment by the (padded) size.  */
+
+tree
+std_gimplify_va_arg_expr (tree valist, tree type, gimple_seq *pre_p,
+			  gimple_seq *post_p)
+{
+  return std_gimplify_va_arg_expr_1 (valist, type, pre_p, post_p, false);
+}
+
+/* Like std_gimplify_va_arg_expr, but honor function_arg_boundary
+   even for zero-sized containers.  */
+
+tree
+std_gimplify_va_arg_expr_always_align (tree valist, tree type,
+				       gimple_seq *pre_p, gimple_seq *post_p)
+{
+  return std_gimplify_va_arg_expr_1 (valist, type, pre_p, post_p, true);
+}
+
 /* Build an indirect-ref expression over the given TREE, which represents a
    piece of a va_arg() expansion.  */
 tree
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	2012-01-29 11:15:28.000000000 +0000
+++ gcc/config/mips/mips.c	2012-01-29 11:27:25.000000000 +0000
@@ -5590,7 +5590,7 @@ mips_gimplify_va_arg_expr (tree valist,
     type = build_pointer_type (type);
 
   if (!EABI_FLOAT_VARARGS_P)
-    addr = std_gimplify_va_arg_expr (valist, type, pre_p, post_p);
+    addr = std_gimplify_va_arg_expr_always_align (valist, type, pre_p, post_p);
   else
     {
       tree f_ovfl, f_gtop, f_ftop, f_goff, f_foff;
Index: gcc/testsuite/gcc.dg/va-arg-6.c
===================================================================
--- /dev/null	2012-01-29 09:41:21.178691970 +0000
+++ gcc/testsuite/gcc.dg/va-arg-6.c	2012-01-29 11:36:30.000000000 +0000
@@ -0,0 +1,48 @@
+/* { dg-options "" } */
+/* { dg-do run } */
+
+#include <stdarg.h>
+
+extern void abort (void);
+
+struct __attribute__((aligned(16))) empty {};
+
+static void __attribute__((noinline))
+check_args (int count, ...)
+{
+  va_list va;
+  int i;
+
+  va_start (va, count);
+  for (i = 0; i < count; i++)
+    if (va_arg (va, int) != 1000 + i)
+      abort ();
+
+  va_arg (va, struct empty);
+  if (va_arg (va, int) != 2000 + count)
+    abort ();
+
+  va_end (va);
+}
+
+int
+main (void)
+{
+  struct empty e;
+
+  check_args (1, 1000, e, 2001);
+  check_args (2, 1000, 1001, e, 2002);
+  check_args (3, 1000, 1001, 1002, e, 2003);
+  check_args (4, 1000, 1001, 1002, 1003, e, 2004);
+  check_args (5, 1000, 1001, 1002, 1003, 1004, e, 2005);
+  check_args (6, 1000, 1001, 1002, 1003, 1004, 1005, e, 2006);
+  check_args (7, 1000, 1001, 1002, 1003, 1004, 1005, 1006, e, 2007);
+  check_args (8, 1000, 1001, 1002, 1003, 1004, 1005, 1006, 1007, e, 2008);
+  check_args (9, 1000, 1001, 1002, 1003, 1004, 1005, 1006, 1007,
+	      1008, e, 2009);
+  check_args (10, 1000, 1001, 1002, 1003, 1004, 1005, 1006, 1007,
+	      1008, 1009, e, 2010);
+  check_args (11, 1000, 1001, 1002, 1003, 1004, 1005, 1006, 1007,
+	      1008, 1009, 1010, e, 2011);
+  return 0;
+}