Re: [PATCH] PR64979: S/390: Fix setup of overflow area pointer in va_start

[Jakub Jelinek <jakub at redhat dot com>]

On Mon, Feb 09, 2015 at 12:40:05PM +0100, Andreas Krebbel wrote:
> On 02/09/2015 12:29 PM, Jakub Jelinek wrote:
> > On Mon, Feb 09, 2015 at 10:50:34AM +0100, Andreas Krebbel wrote:
> >> Hi,
> >>
> >> the attached patch fixes a critical problem in the va_start expansion
> >> code in the S/390 backend. The problem exists since GCC 4.0.
> >>
> >> Ok to commit to 4.9 branch and mainline?
> > 
> > No.  This isn't a backend problem, but a bug in the tree-stdarg.c pass,
> > so should be fixed there, for all targets, rather than just worked around by
> > pessimizing unnecessarily one target.
> 
> I think for the overflow area pointer we would need a different flag indicating whether a pointer to
> the va_list structure escapes or not. To my understanding it is not correct to only use the
> va_list_gpr_size/va_list_fpr_size fields for that purpose. These only refer to the va_arg expansions
> in the current function.

No.  The flag for the escapes is
  if (va_list_escapes)
    {
      fun->va_list_gpr_size = VA_LIST_MAX_GPR_SIZE;
      fun->va_list_fpr_size = VA_LIST_MAX_FPR_SIZE;
    }
and has been that way since forever.  Escape case is like no stdarg pass run
at all (-O0), it is a case where you don't know anything about it.

What I mean as a fix is something like untested following patch, though that
only fixes the case on the !va_list_simple_ptr targets like x86_64 or s390*,
but not say on i?86.  Even on i?86 it should say that it escapes, because it
does, thus something is still wrong in the stdarg pass.

As you can see, the updated testcase fails even on x86_64-linux.

--- gcc/tree-stdarg.c.jj	2015-01-09 21:59:44.000000000 +0100
+++ gcc/tree-stdarg.c	2015-02-09 12:47:12.020789728 +0100
@@ -856,22 +856,23 @@ pass_stdarg::execute (function *fun)
       /* For va_list_simple_ptr, we have to check PHI nodes too.  We treat
 	 them as assignments for the purpose of escape analysis.  This is
 	 not needed for non-simple va_list because virtual phis don't perform
-	 any real data movement.  */
-      if (va_list_simple_ptr)
-	{
-	  tree lhs, rhs;
-	  use_operand_p uop;
-	  ssa_op_iter soi;
+	 any real data movement.  For !va_list_simple_ptr, check PHI nodes for
+	 taking address of the va_list vars.  */
+      tree lhs, rhs;
+      use_operand_p uop;
+      ssa_op_iter soi;
 
-	  for (gphi_iterator i = gsi_start_phis (bb); !gsi_end_p (i);
-	       gsi_next (&i))
-	    {
-	      gphi *phi = i.phi ();
-	      lhs = PHI_RESULT (phi);
+      for (gphi_iterator i = gsi_start_phis (bb); !gsi_end_p (i);
+	   gsi_next (&i))
+	{
+	  gphi *phi = i.phi ();
+	  lhs = PHI_RESULT (phi);
 
-	      if (virtual_operand_p (lhs))
-		continue;
+	  if (virtual_operand_p (lhs))
+	    continue;
 
+	  if (va_list_simple_ptr)
+	    {
 	      FOR_EACH_PHI_ARG (uop, phi, soi, SSA_OP_USE)
 		{
 		  rhs = USE_FROM_PTR (uop);
@@ -894,6 +895,22 @@ pass_stdarg::execute (function *fun)
 		    }
 		}
 	    }
+	  else
+	    {
+	      for (unsigned j = 0; !va_list_escapes
+				   && j < gimple_phi_num_args (phi); ++j)
+		if (walk_tree (gimple_phi_arg_def_ptr (phi, j),
+			       find_va_list_reference, &wi, NULL))
+		  {
+		    if (dump_file && (dump_flags & TDF_DETAILS))
+		      {
+			fputs ("va_list escapes in ", dump_file);
+			print_gimple_stmt (dump_file, phi, 0, dump_flags);
+			fputc ('\n', dump_file);
+		      }
+		    va_list_escapes = true;
+		  }
+	    }
 	}
 
       for (gimple_stmt_iterator i = gsi_start_bb (bb);
@@ -916,8 +933,8 @@ pass_stdarg::execute (function *fun)
 
 	  if (is_gimple_assign (stmt))
 	    {
-	      tree lhs = gimple_assign_lhs (stmt);
-	      tree rhs = gimple_assign_rhs1 (stmt);
+	      lhs = gimple_assign_lhs (stmt);
+	      rhs = gimple_assign_rhs1 (stmt);
 
 	      if (va_list_simple_ptr)
 		{
--- gcc/testsuite/gcc.dg/tree-ssa/stdarg-7.c.jj	2015-02-09 12:54:44.222284401 +0100
+++ gcc/testsuite/gcc.dg/tree-ssa/stdarg-7.c	2015-02-09 12:58:10.406875647 +0100
@@ -0,0 +1,22 @@
+/* PR target/64979 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-stdarg" } */
+
+#include <stdarg.h>
+
+void bar (int x, va_list *ap);
+
+void
+foo (int x, ...)
+{
+  va_list ap;
+  int n;
+
+  va_start (ap, x);
+  n = va_arg (ap, int);
+  bar (x, (va_list *) ((n == 0) ? ((void *) 0) : &ap));
+  va_end (ap);
+}
+
+/* { dg-final { scan-tree-dump "foo: va_list escapes 1, needs to save all GPR units and all FPR units" "stdarg" } } */
+/* { dg-final { cleanup-tree-dump "stdarg" } } */
--- gcc/testsuite/gcc.c-torture/execute/pr64979.c.jj	2015-02-09 12:54:01.867984625 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr64979.c	2015-02-09 12:55:12.998808652 +0100
@@ -0,0 +1,36 @@
+/* PR target/64979 */
+
+#include <stdarg.h>
+
+void __attribute__((noinline, noclone))
+bar (int x, va_list *ap)
+{
+  if (ap)
+    {
+      int i;
+      for (i = 0; i < 10; i++)
+	if (i != va_arg (*ap, int))
+	  __builtin_abort ();
+      if (va_arg (*ap, double) != 0.5)
+	__builtin_abort ();
+    }
+}
+
+void __attribute__((noinline, noclone))
+foo (int x, ...)
+{
+  va_list ap;
+  int n;
+
+  va_start (ap, x);
+  n = va_arg (ap, int);
+  bar (x, (va_list *) ((n == 0) ? ((void *) 0) : &ap));
+  va_end (ap);
+}
+
+int
+main ()
+{
+  foo (100, 1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0.5);
+  return 0;
+}


	Jakub