Improved sequence point warnings

From-SVN: r36901

gcc/ChangeLog

+2000-10-17  Bernd Schmidt  <bernds@redhat.co.uk>
+
+	* c-tree.h (warn_sequence_point): Move declaration to...
+	* c-common.h (warn_sequence_point): ... here.
+	* c-decl.c (warn_sequence_point): Move definition to...
+	* c-common.c (warn_sequence_point): ... here.
+	(struct reverse_tree): New.
+	(reverse_list, reverse_max_depth): New static variables.
+	(build_reverse_tree, common_ancestor, modify_ok
+	verify_sequence_points): New functions.
+	(c_expand_expr_stmt): Call verify_sequence_points if -Wsequence-point.
+	* c-typeck.c (check_modify_expr): Delete.
+	(build_modify_expr): Don't call it.
+
 2000-10-17  Joseph S. Myers  <jsm28@cam.ac.uk>
 
 	* c-common.h (warn_missing_format_attribute): New variable.

gcc/c-common.c

@@ -140,6 +140,10 @@ cpp_reader  parse_in;
 
 tree c_global_trees[CTI_MAX];
 
+/* Nonzero means warn about possible violations of sequence point rules.  */
+
+int warn_sequence_point;
+
 /* The elements of `ridpointers' are identifier nodes for the reserved
    type names and storage classes.  It is indexed by a RID_... value.  */
 tree *ridpointers;
@@ -3346,6 +3350,263 @@ convert_and_check (type, expr)
   return t;
 }
 
+/* Describe a reversed version of a normal tree, so that we can get to the
+   parent of each node.  */
+struct reverse_tree
+{
+  /* All reverse_tree structures for a given tree are chained through this
+     field.  */
+  struct reverse_tree *next;
+  /* The parent of this node.  */
+  struct reverse_tree *parent;
+  /* The actual tree node.  */
+  tree x;
+  /* The operand number this node corresponds to in the parent.  */
+  int operandno;
+  /* Describe whether this expression is written to or read.  */
+  char read, write;
+};
+
+/* A list of all reverse_tree structures for a given expression, built by
+   build_reverse_tree.  */
+static struct reverse_tree *reverse_list;
+/* The maximum depth of a tree, computed by build_reverse_tree.  */
+static int reverse_max_depth;
+
+static void build_reverse_tree PARAMS ((tree, struct reverse_tree *, int, int,
+					int, int));
+static struct reverse_tree *common_ancestor PARAMS ((struct reverse_tree *,
+						     struct reverse_tree *,
+						     struct reverse_tree **,
+						     struct reverse_tree **));
+static int modify_ok PARAMS ((struct reverse_tree *, struct reverse_tree *));
+static void verify_sequence_points PARAMS ((tree));
+
+/* Recursively process an expression, X, building a reverse tree while
+   descending and recording OPERANDNO, READ, and WRITE in the created
+   structures.  DEPTH is used to compute reverse_max_depth.
+   FIXME: if walk_tree gets moved out of the C++ front end, this should
+   probably use walk_tree.  */
+
+static void
+build_reverse_tree (x, parent, operandno, read, write, depth)
+     tree x;
+     struct reverse_tree *parent;
+     int operandno, read, write, depth;
+{
+  struct reverse_tree *node;
+
+  if (x == 0 || x == error_mark_node)
+    return;
+
+  node = (struct reverse_tree *) xmalloc (sizeof (struct reverse_tree));
+
+  node->parent = parent;
+  node->x = x;
+  node->read = read;
+  node->write = write;
+  node->operandno = operandno;
+  node->next = reverse_list;
+  reverse_list = node;
+  if (depth > reverse_max_depth)
+    reverse_max_depth = depth;
+
+  switch (TREE_CODE (x))
+    {
+    case PREDECREMENT_EXPR:
+    case PREINCREMENT_EXPR:
+    case POSTDECREMENT_EXPR:
+    case POSTINCREMENT_EXPR:
+      build_reverse_tree (TREE_OPERAND (x, 0), node, 0, 1, 1, depth + 1);
+      break;
+
+    case CALL_EXPR:
+      build_reverse_tree (TREE_OPERAND (x, 0), node, 0, 1, 0, depth + 1);
+      x = TREE_OPERAND (x, 1);
+      while (x)
+	{
+	  build_reverse_tree (TREE_VALUE (x), node, 1, 1, 0, depth + 1);
+	  x = TREE_CHAIN (x);
+	}
+      break;
+
+    case TREE_LIST:
+      /* Scan all the list, e.g. indices of multi dimensional array.  */
+      while (x)
+	{
+	  build_reverse_tree (TREE_VALUE (x), node, 0, 1, 0, depth + 1);
+	  x = TREE_CHAIN (x);
+	}
+      break;
+
+    case MODIFY_EXPR:
+      build_reverse_tree (TREE_OPERAND (x, 0), node, 0, 0, 1, depth + 1);
+      build_reverse_tree (TREE_OPERAND (x, 1), node, 1, 1, 0, depth + 1);
+      break;
+
+    default:
+      switch (TREE_CODE_CLASS (TREE_CODE (x)))
+	{
+	case 'r':
+	case '<':
+	case '2':
+	case 'b':
+	case '1':
+	case 'e':
+	case 's':
+	case 'x':
+	  {
+	    int lp;
+	    int max = first_rtl_op (TREE_CODE (x));
+	    for (lp = 0; lp < max; lp++)
+	      build_reverse_tree (TREE_OPERAND (x, lp), node, lp, 1, 0,
+				  depth + 1);
+	    break;
+	  }
+	default:
+	  break;
+	}
+      break;
+    }
+}
+
+/* Given nodes P1 and P2 as well as enough scratch space pointed to by TMP1
+   and TMP2, find the common ancestor of P1 and P2.  */
+
+static struct reverse_tree *
+common_ancestor (p1, p2, tmp1, tmp2)
+     struct reverse_tree *p1, *p2;
+     struct reverse_tree **tmp1, **tmp2;
+{
+  struct reverse_tree *t1 = p1;
+  struct reverse_tree *t2 = p2;
+  int i, j;
+
+  /* First, check if we're actually looking at the same expression twice,
+     which can happen if it's wrapped in a SAVE_EXPR - in this case there's
+     no chance of conflict.  */
+  while (t1 && t2 && t1->x == t2->x)
+    {
+      if (TREE_CODE (t1->x) == SAVE_EXPR)
+	return 0;
+      t1 = t1->parent;
+      t2 = t2->parent;
+    }
+
+  for (i = 0; p1; i++, p1 = p1->parent)
+    tmp1[i] = p1;
+  for (j = 0; p2; j++, p2 = p2->parent)
+    tmp2[j] = p2;
+  while (tmp1[i - 1] == tmp2[j - 1])
+    i--, j--;
+
+  return tmp1[i];
+}
+
+/* Subroutine of verify_sequence_points to check whether a node T corresponding
+   to a MODIFY_EXPR invokes undefined behaviour.  OTHER occurs somewhere in the
+   RHS, and an identical expression is the LHS of T.
+   For MODIFY_EXPRs, some special cases apply when testing for undefined
+   behaviour if one of the expressions we found is the LHS of the MODIFY_EXPR.
+   If the other expression is just a use, then there's no undefined behaviour.
+   Likewise, if the other expression is wrapped inside another expression that
+   will force a sequence point, then there's no undefined behaviour either.  */
+
+static int
+modify_ok (t, other)
+     struct reverse_tree *t, *other;
+{
+  struct reverse_tree *p;
+
+  if (! other->write)
+    return 1;
+
+  /* See if there's an intervening sequence point.  */
+  for (p = other; p->parent != t; p = p->parent)
+    {
+      if ((TREE_CODE (p->parent->x) == COMPOUND_EXPR
+	   || TREE_CODE (p->parent->x) == TRUTH_ANDIF_EXPR
+	   || TREE_CODE (p->parent->x) == TRUTH_ORIF_EXPR
+	   || TREE_CODE (p->parent->x) == COND_EXPR)
+	  && p->operandno == 0)
+	return 1;
+      if (TREE_CODE (p->parent->x) == SAVE_EXPR)
+	return 1;
+      if (TREE_CODE (p->parent->x) == CALL_EXPR
+	  && p->operandno != 0)
+	return 1;
+    }
+  return 0;
+}
+
+/* Try to warn for undefined behaviour in EXPR due to missing sequence
+   points.  */
+
+static void
+verify_sequence_points (expr)
+     tree expr;
+{
+  struct reverse_tree **tmp1, **tmp2;
+  struct reverse_tree *p;
+
+  reverse_list = 0;
+  reverse_max_depth = 0;
+  build_reverse_tree (expr, NULL, 0, 1, 0, 1);
+
+  tmp1 = (struct reverse_tree **) xmalloc (sizeof (struct reverse_tree *)
+					   * reverse_max_depth);
+  tmp2 = (struct reverse_tree **) xmalloc (sizeof (struct reverse_tree *)
+					   * reverse_max_depth);
+
+  /* Search for multiple occurrences of the same variable, where either both
+     occurrences are writes, or one is a read and a write.  If we can't prove
+     that these are ordered by a sequence point, warn that the expression is
+     undefined.  */
+  for (p = reverse_list; p; p = p->next)
+    {
+      struct reverse_tree *p2;
+      if (TREE_CODE (p->x) != VAR_DECL && TREE_CODE (p->x) != PARM_DECL)
+	continue;
+      for (p2 = p->next; p2; p2 = p2->next)
+	{
+	  if ((TREE_CODE (p2->x) == VAR_DECL || TREE_CODE (p2->x) == PARM_DECL)
+	      && DECL_NAME (p->x) == DECL_NAME (p2->x)
+	      && (p->write || p2->write))
+	    {
+	      struct reverse_tree *t = common_ancestor (p, p2, tmp1, tmp2);
+
+	      if (t == 0
+		  || TREE_CODE (t->x) == COMPOUND_EXPR
+		  || TREE_CODE (t->x) == TRUTH_ANDIF_EXPR
+		  || TREE_CODE (t->x) == TRUTH_ORIF_EXPR
+		  || TREE_CODE (t->x) == COND_EXPR)
+		continue;
+	      if (TREE_CODE (t->x) == MODIFY_EXPR
+		  && p->parent == t
+		  && modify_ok (t, p2))
+		continue;
+	      if (TREE_CODE (t->x) == MODIFY_EXPR
+		  && p2->parent == t
+		  && modify_ok (t, p))
+		continue;
+
+	      warning ("operation on `%s' may be undefined",
+		       IDENTIFIER_POINTER (DECL_NAME (p->x)));
+	      break;
+	    }
+	}
+    }
+
+  while (reverse_list)
+    {
+      struct reverse_tree *p = reverse_list;
+      reverse_list = p->next;
+      free (p);
+    }
+  free (tmp1);
+  free (tmp2);
+}
+
 void
 c_expand_expr_stmt (expr)
      tree expr;
@@ -3356,6 +3617,9 @@ c_expand_expr_stmt (expr)
       || TREE_CODE (TREE_TYPE (expr)) == FUNCTION_TYPE)
     expr = default_conversion (expr);
 
+  if (warn_sequence_point)
+    verify_sequence_points (expr);
+
   if (TREE_TYPE (expr) != error_mark_node
       && !COMPLETE_OR_VOID_TYPE_P (TREE_TYPE (expr))
       && TREE_CODE (TREE_TYPE (expr)) != ARRAY_TYPE)

gcc/c-common.h

@@ -337,6 +337,10 @@ extern int flag_const_strings;
 
 extern int warn_format;
 
+/* Warn about possible violations of sequence point rules.  */
+
+extern int warn_sequence_point;
+
 /* Warn about functions which might be candidates for format attributes.  */
 
 extern int warn_missing_format_attribute;

gcc/c-decl.c

@@ -498,10 +498,6 @@ int warn_float_equal = 0;
 
 int warn_multichar = 1;
 
-/* Nonzero means warn about possible violations of sequence point rules.  */
-
-int warn_sequence_point;
-
 /* The variant of the C language being processed.  */
 
 c_language_kind c_language = clk_c;

gcc/c-tree.h

@@ -366,10 +366,6 @@ extern int warn_missing_braces;
 
 extern int warn_sign_compare;
 
-/* Warn about possible violations of sequence point rules.  */
-
-extern int warn_sequence_point;
-
 /* Warn about testing equality of floating point numbers. */
 
 extern int warn_float_equal;

gcc/c-typeck.c

@@ -61,7 +61,6 @@ static tree pointer_diff		PARAMS ((tree, tree));
 static tree unary_complex_lvalue	PARAMS ((enum tree_code, tree));
 static void pedantic_lvalue_warning	PARAMS ((enum tree_code));
 static tree internal_build_compound_expr PARAMS ((tree, int));
-static void check_modify_expr		PARAMS ((tree, tree));
 static tree convert_for_assignment	PARAMS ((tree, tree, const char *,
 						 tree, tree, int));
 static void warn_for_assignment		PARAMS ((const char *, const char *,
@@ -3816,132 +3815,6 @@ build_c_cast (type, expr)
   return value;
 }
 
-/* Recursive check for expressions that break the sequence point rules
-   and so have undefined semantics (e.g. n = n++).  FIXME: if walk_tree
-   gets moved out of the C++ front end, this should probably be moved
-   to code shared between the front ends and use walk_tree.  */
-static void
-check_modify_expr (lhs, rhs)
-     tree lhs, rhs;
-{
-  tree identifier_name;   /* A VAR_DECL name on the LHS that could
-			     be the same as one on the RHS. */
-  identifier_name = NULL_TREE;
-
-  if ((lhs == NULL_TREE) || (rhs == NULL_TREE))
-    return;
-
-  switch (TREE_CODE (rhs))
-    {
-    case ERROR_MARK:
-      return;
-    case VAR_DECL:
-    case PARM_DECL:
-      identifier_name = DECL_NAME (rhs);
-      break;
-    case PREDECREMENT_EXPR:
-    case PREINCREMENT_EXPR:
-    case POSTDECREMENT_EXPR:
-    case POSTINCREMENT_EXPR:
-      {
-	tree var_decl = TREE_OPERAND (rhs, 0);
-	if (TREE_CODE (var_decl) == VAR_DECL
-	    || TREE_CODE (var_decl) == PARM_DECL)
-	  identifier_name = DECL_NAME (var_decl);
-      }
-      break;
-    case TREE_LIST:
-      {
-	tree parm = TREE_CHAIN (rhs);
-	/* Now scan all the list, e.g. indices of multi dimensional array.  */
-	while (parm)
-	  {
-	    check_modify_expr (lhs, TREE_VALUE (parm));
-	    parm = TREE_CHAIN (parm);
-	  }
-      }
-      return;
-    case NOP_EXPR:
-    case CONVERT_EXPR:
-    case NON_LVALUE_EXPR:
-      check_modify_expr (lhs, TREE_OPERAND (rhs, 0));
-      return;
-    case MODIFY_EXPR:
-      /* First check for form a = b = a++ by checking RHS.  */
-      check_modify_expr (lhs, TREE_OPERAND (rhs, 1));
-      /* Then check for a = (a = 1) + 2 and a = b[a++] = c.  */
-      if (TREE_CODE (TREE_OPERAND (rhs, 0)) == VAR_DECL
-	  || TREE_CODE (TREE_OPERAND (rhs, 0)) == PARM_DECL)
-	{
-	  identifier_name = DECL_NAME (TREE_OPERAND (rhs, 0));
-	  break;
-	}
-      else
-	{
-	  check_modify_expr (lhs, TREE_OPERAND (rhs, 0));
-	  return;
-	}
-    default:
-      /* We don't know what to do... pray check_modify_expr removes
-	 loops in the tree.  */
-      switch (TREE_CODE_CLASS (TREE_CODE (rhs)))
-	{
-	case 'r':
-	case '<':
-	case '2':
-	case 'b':
-	case '1':
-	case 'e':
-	case 's':
-	case 'x':
-	  {
-	    int lp;
-	    int max = first_rtl_op (TREE_CODE (rhs));
-	    for (lp = 0; lp < max; lp++)
-	      check_modify_expr (lhs, TREE_OPERAND (rhs, lp));
-	    return;
-	  }
-	default:
-	  return;
-	}
-      break;
-    }
-  if (identifier_name != NULL_TREE)
-    {
-      switch (TREE_CODE (lhs))
-	{
-	case ERROR_MARK:
-	  return;
-	  /* Perhaps this variable was incremented on the RHS.  */
-	case VAR_DECL:
-	case PARM_DECL:
-	  if (TREE_CODE (rhs) != VAR_DECL && TREE_CODE (rhs) != PARM_DECL)
-	    if (DECL_NAME (lhs) == identifier_name)
-	      warning ("operation on `%s' may be undefined",
-		       IDENTIFIER_POINTER (DECL_NAME (lhs)));
-	  break;
-	case PREDECREMENT_EXPR:
-	case PREINCREMENT_EXPR:
-	case POSTDECREMENT_EXPR:
-	case POSTINCREMENT_EXPR:
-	  {
-	    tree var_decl = TREE_OPERAND (lhs, 0);
-	    if (TREE_CODE (var_decl) == VAR_DECL
-		|| TREE_CODE (var_decl) == PARM_DECL)
-	      if (identifier_name == DECL_NAME (var_decl))
-		warning ("operation on `%s' may be undefined",
-			 IDENTIFIER_POINTER (DECL_NAME (var_decl)));
-	  }
-	  break;
-	default:
-	  /* To save duplicating tree traversal code swap args, and recurse.  */
-	  check_modify_expr (rhs, lhs);
-	  break;
-	}
-    }
-}
-
-
 /* Build an assignment expression of lvalue LHS from value RHS.
    MODIFYCODE is the code for a binary operator that we use
    to combine the old value of LHS with RHS to get the new value.
@@ -4096,9 +3969,6 @@ build_modify_expr (lhs, modifycode, rhs)
   if (TREE_CODE (newrhs) == ERROR_MARK)
     return error_mark_node;
 
-  if (warn_sequence_point)
-    check_modify_expr (lhs, rhs);
-
   /* Scan operands */
 
   result = build (MODIFY_EXPR, lhstype, lhs, newrhs);

gcc/testsuite/ChangeLog

+2000-10-17  Bernd Schmidt  <bernds@redhat.co.uk>
+
+	* gcc.dg/sequence-pt-1.c: Several new tests; remove xfail from some old tests.
+
 2000-10-17  Joseph S. Myers  <jsm28@cam.ac.uk>
 
 	* gcc.dg/format-miss-1.c: New test.

gcc/testsuite/gcc.dg/sequence-pt-1.c

@@ -11,6 +11,8 @@ struct s
 };
 
 extern int fn (int);
+extern int fnb (int, int);
+extern int fnc (int *);
 extern int sprintf (char *, const char *, ...);
 
 void
@@ -18,11 +20,11 @@ foo (int a, int b, int n, int p, int *ptr, struct s *sptr,
      int *ap, int *bp, int **cp, char *ans)
 {
   int len;
+    
   a = a++; /* { dg-warning "undefined" "sequence point warning" } */
   a = --a; /* { dg-warning "undefined" "sequence point warning" } */
   a = ++a + b; /* { dg-warning "undefined" "sequence point warning" } */
   a = a-- + b; /* { dg-warning "undefined" "sequence point warning" } */
-  a = (a++ && 4); /* { dg-bogus "undefined" "bogus sequence point warning" { xfail *-*-* } } */
   ap[n] = bp[n++]; /* { dg-warning "undefined" "sequence point warning" } */
   ap[--n] = bp[n]; /* { dg-warning "undefined" "sequence point warning" } */
   ap[++n] = bp[--n]; /* { dg-warning "undefined" "sequence point warning" } */
@@ -31,12 +33,27 @@ foo (int a, int b, int n, int p, int *ptr, struct s *sptr,
   *ptr++ = (int)ptr++; /* { dg-warning "undefined" "sequence point warning" } */
   sptr->a = sptr->a++; /* { dg-warning "undefined" "sequence point warning" { xfail *-*-* } } */
   sptr->a = (int)(sptr++); /* { dg-warning "undefined" "sequence point warning" } */
-  len = sprintf (ans, "%d", len++); /* { dg-bogus "undefined" "bogus sequence point warning" { xfail *-*-* } } */
-  *ptr++ = fn (*ptr); /* { dg-warning "undefined" "sequence point warning" { xfail *-*-* } } */
+  *ptr++ = fn (*ptr); /* { dg-warning "undefined" "sequence point warning" } */
   a = b = a++; /* { dg-warning "undefined" "sequence point warning" } */
   b = a = --b; /* { dg-warning "undefined" "sequence point warning" } */
   a = 1 + (a = 1); /* { dg-warning "undefined" "sequence point warning" } */
   a = (a = b); /* { dg-warning "undefined" "sequence point warning" } */
   a = (a = b) + 1; /* { dg-warning "undefined" "sequence point warning" } */
   a = (bp[a++] = b) + 1; /* { dg-warning "undefined" "sequence point warning" } */
+  a = b++ * b++; /* { dg-warning "undefined" "sequence point warning" } */
+  a = fnb (b++, b++); /* { dg-warning "undefined" "sequence point warning" } */
+  *ap = fnc (ap++); /* { dg-warning "undefined" "sequence point warning" } */
+  (a += b) + (a += n); /* { dg-warning "undefined" "sequence point warning" } */
+  a =  (b, b++) + (b++, b); /* { dg-warning "undefined" "sequence point warning" } */
+
+  a = (a++ && 4); /* { dg-bogus "undefined" "bogus sequence point warning" } */
+  len = sprintf (ans, "%d", len++); /* { dg-bogus "undefined" "bogus sequence point warning" } */
+  a = fn (a++); /* { dg-bogus "undefined" "sequence point warning" } */
+  (a = b++), (a = b++); /* { dg-bogus "undefined" "sequence point warning" } */
+  a = (b++, b++); /* { dg-bogus "undefined" "sequence point warning" } */
+  a = b++ && b++; /* { dg-bogus "undefined" "sequence point warning" } */
+  a = b++ || b++; /* { dg-bogus "undefined" "sequence point warning" } */
+  a = (b++ ? b++ : a); /* { dg-bogus "undefined" "sequence point warning" } */
+  a = (b++ ? a : b++); /* { dg-bogus "undefined" "sequence point warning" } */
+  ap[a++] += bp[b]; /* { dg-bogus "undefined" "sequence point warning" } */
 }