This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] Fix va_start (PR tree-optimization/18828)


Hi!

GCC 4.0 if optimizing issues warnings about va_start e.g. on the 3
cases in the testcase below, while I don't see anything in the standard
that would preclude modifying the value of the parameter passed as
va_start's second argument, one of the examples in the standard even
does void foo (int i, ...) { va_list ap; if (i > MAXI) i = MAXI;
va_start (ap, i); va_end (ap); }.
The problem is that if we keep the parameter as second argument to
__builtin_va_start after checking that it is the correct argument
and possibly warning if it is not, the tree optimizers optimize
even that argument, so it is no longer the actual parameter but e.g.
a constant or a PLUS_EXPR of the parameter and constant etc.
and we issue a warning.

For 4.1, I believe you are planning to gimplify va_arg right away,
so this problem will go away, so below is just a temporary solution
(well, most of it is a cleanup and only a few lines of an ugly hack).

IMHO we want for 4.0 to make sure we check the arguments just once,
before gimplification or during it, and after that make sure the
parameter is not among __builtin_va_start arguments any longer
(so that e.g. tree optimizers don't think it is needed when the
builtin actually doesn't need its value at all).
I'm not sure if gimplify_call_expr can't be called multiple times
on the builtin, so I chose a magic __builtin_va_start (ap, 0, 0)
that tells fold_builtin_next_arg the arguments have been already
verified.  I avoided using __builtin_va_start (ap) for this, because
the code used to warn about missing arguments before.

On the bright side, this patch removes duplicated code from
expand_builtin_next_arg, fold_builtin_next_arg is called always before
it anyway and does the same checking.

Ok or is this too ugly even as a temporary solution?

2005-01-05  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/18828
	* builtins.c (expand_builtin_next_arg): Remove argument and all
	the argument checking.
	(expand_builtin): Adjust caller.
	(expand_builtin_va_start): Likewise.  Remove error for too many
	arguments.
	(fold_builtin_next_arg): Issue error for too many arguments.
	After checking arguments, replace them with magic arguments that
	prevent further checking of the args.

	* gcc.dg/20050105-2.c: New test.

--- gcc/builtins.c.jj	2004-12-14 19:01:11.000000000 +0100
+++ gcc/builtins.c	2005-01-05 20:09:33.951698568 +0100
@@ -99,7 +99,7 @@ static rtx expand_builtin_mathfn (tree, 
 static rtx expand_builtin_mathfn_2 (tree, rtx, rtx);
 static rtx expand_builtin_mathfn_3 (tree, rtx, rtx);
 static rtx expand_builtin_args_info (tree);
-static rtx expand_builtin_next_arg (tree);
+static rtx expand_builtin_next_arg (void);
 static rtx expand_builtin_va_start (tree);
 static rtx expand_builtin_va_end (tree);
 static rtx expand_builtin_va_copy (tree);
@@ -3743,43 +3743,13 @@ expand_builtin_args_info (tree arglist)
   return const0_rtx;
 }
 
-/* Expand ARGLIST, from a call to __builtin_next_arg.  */
+/* Expand a call to __builtin_next_arg.  */
 
 static rtx
-expand_builtin_next_arg (tree arglist)
+expand_builtin_next_arg (void)
 {
-  tree fntype = TREE_TYPE (current_function_decl);
-
-  if (TYPE_ARG_TYPES (fntype) == 0
-      || (TREE_VALUE (tree_last (TYPE_ARG_TYPES (fntype)))
-	  == void_type_node))
-    {
-      error ("%<va_start%> used in function with fixed args");
-      return const0_rtx;
-    }
-
-  if (arglist)
-    {
-      tree last_parm = tree_last (DECL_ARGUMENTS (current_function_decl));
-      tree arg = TREE_VALUE (arglist);
-
-      /* Strip off all nops for the sake of the comparison.  This
-	 is not quite the same as STRIP_NOPS.  It does more.
-	 We must also strip off INDIRECT_EXPR for C++ reference
-	 parameters.  */
-      while (TREE_CODE (arg) == NOP_EXPR
-	     || TREE_CODE (arg) == CONVERT_EXPR
-	     || TREE_CODE (arg) == NON_LVALUE_EXPR
-	     || TREE_CODE (arg) == INDIRECT_REF)
-	arg = TREE_OPERAND (arg, 0);
-      if (arg != last_parm)
-	warning ("second parameter of %<va_start%> not last named argument");
-    }
-  else
-    /* Evidently an out of date version of <stdarg.h>; can't validate
-       va_start's second argument, but can still work as intended.  */
-    warning ("%<__builtin_next_arg%> called without an argument");
-
+  /* Checking arguments is already done in fold_builtin_next_arg
+     that must be called before this function.  */
   return expand_binop (Pmode, add_optab,
 		       current_function_internal_arg_pointer,
 		       current_function_arg_offset_rtx,
@@ -3867,15 +3837,11 @@ expand_builtin_va_start (tree arglist)
       error ("too few arguments to function %<va_start%>");
       return const0_rtx;
     }
-  if (TREE_CHAIN (chain))
-    error ("too many arguments to function %<va_start%>");
 
   if (fold_builtin_next_arg (chain))
-    {
-      return const0_rtx;
-    }
+    return const0_rtx;
 
-  nextarg = expand_builtin_next_arg (chain);
+  nextarg = expand_builtin_next_arg ();
   valist = stabilize_va_list (TREE_VALUE (arglist), 1);
 
 #ifdef EXPAND_BUILTIN_VA_START
@@ -5256,7 +5222,7 @@ expand_builtin (tree exp, rtx target, rt
     case BUILT_IN_NEXT_ARG:
       if (fold_builtin_next_arg (arglist))
         return const0_rtx;
-      return expand_builtin_next_arg (arglist);
+      return expand_builtin_next_arg ();
 
     case BUILT_IN_CLASSIFY_TYPE:
       return expand_builtin_classify_type (arglist);
@@ -8671,11 +8637,29 @@ fold_builtin_next_arg (tree arglist)
       error ("%<va_start%> used in function with fixed args");
       return true;
     }
-  else if (arglist)
+  else if (!arglist)
+    {
+      /* Evidently an out of date version of <stdarg.h>; can't validate
+	 va_start's second argument, but can still work as intended.  */
+      warning ("%<__builtin_next_arg%> called without an argument");
+      return true;
+    }
+  /* We use __builtin_va_start (ap, 0, 0) or __builtin_next_arg (0, 0)
+     when we checked the arguments and if needed issued a warning.  */
+  else if (!TREE_CHAIN (arglist)
+           || !integer_zerop (TREE_VALUE (arglist))
+           || !integer_zerop (TREE_VALUE (TREE_CHAIN (arglist)))
+           || TREE_CHAIN (TREE_CHAIN (arglist)))
     {
       tree last_parm = tree_last (DECL_ARGUMENTS (current_function_decl));
       tree arg = TREE_VALUE (arglist);
 
+      if (TREE_CHAIN (arglist))
+        {
+          error ("%<va_start%> used with too many arguments");
+          return true;
+        }
+
       /* Strip off all nops for the sake of the comparison.  This
 	 is not quite the same as STRIP_NOPS.  It does more.
 	 We must also strip off INDIRECT_EXPR for C++ reference
@@ -8692,17 +8676,15 @@ fold_builtin_next_arg (tree arglist)
 	     argument.  We just warn and set the arg to be the last
 	     argument so that we will get wrong-code because of
 	     it.  */
-	  arg = last_parm;
 	  warning ("second parameter of %<va_start%> not last named argument");
 	}
-      TREE_VALUE (arglist) = arg;
-    }
-  else
-    {
-      /* Evidently an out of date version of <stdarg.h>; can't validate
-	 va_start's second argument, but can still work as intended.  */
-      warning ("%<__builtin_next_arg%> called without an argument");
-      return true;
+      /* We want to verify the second parameter just once before the tree
+         optimizers are run and then avoid keeping it in the tree,
+         as otherwise we could warn even for correct code like:
+         void foo (int i, ...)
+         { va_list ap; i++; va_start (ap, i); va_end (ap); }  */
+      TREE_VALUE (arglist) = integer_zero_node;
+      TREE_CHAIN (arglist) = build_tree_list (NULL, integer_zero_node);
     }
   return false;
 }
--- gcc/testsuite/gcc.dg/20050105-2.c.jj	2005-01-05 20:14:13.813967889 +0100
+++ gcc/testsuite/gcc.dg/20050105-2.c	2005-01-05 20:15:03.112206548 +0100
@@ -0,0 +1,32 @@
+/* PR tree-optimization/18828 */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#include <stdarg.h>
+
+extern void abort (void);
+
+void foo (int x, ...)
+{
+  va_list ap;
+  if (x != 21)
+    abort ();
+  va_start (ap, x);
+  va_end (ap);
+}
+
+void bar (int x, ...)
+{
+  va_list ap;
+  x++;
+  va_start (ap, x);
+  va_end (ap);
+}
+
+void baz (int x, ...)
+{
+  va_list ap;
+  x = 0;
+  va_start (ap, x);
+  va_end (ap);
+}

	Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]