uninit var check enhancement patch

[Xinliang David Li <davidxl at google dot com>]

Hi please review the patch attached.

It is quite common in C++ programs that a var is initialized in
multiple paths in the initializer function and there are clean up code
(for temp objects destruction) before each return path. This is
currently confusing the uninit checker and leads to false warning. The
patch fixes this.  See the test cases in the patch for examples.

Bootstrapped and tested on x86-64. Ok for check in?

Thanks,

David

Index: gcc/tree-ssa-uninit.c
===================================================================
--- gcc/tree-ssa-uninit.c	(revision 165438)
+++ gcc/tree-ssa-uninit.c	(working copy)
@@ -784,6 +784,110 @@ is_use_properly_guarded (gimple use_stmt
                          gimple phi,
                          unsigned uninit_opnds,
                          struct pointer_set_t *visited_phis);
+/* Returns true if all uninitialized opnds are pruned. Returns false
+   otherwise. PHI is the phi node with uninitialized operands,
+   UNINIT_OPNDS is the bitmap of the uninitialize operand positions,
+   FLAG_DEF is the statement defining the flag guarding the use of the
+   PHI output, BOUNDARY_CST is the const value used in the predicate
+   associated with the flag, CMP_CODE is the comparison code used in
+   the predicate, VISITED_PHIS is the pointer set of phis visited, and
+   VISITED_FLAG_PHIS is the pointer to the pointer set of flag definitions
+   that are also phis.  */
+
+static bool
+prune_uninit_phi_opnds_in_unrealizable_paths (
+    gimple phi, unsigned uninit_opnds,
+    gimple flag_def, tree boundary_cst,
+    enum tree_code cmp_code,
+    struct pointer_set_t *visited_phis,
+    struct pointer_set_t **visited_flag_phis)
+{
+  unsigned i;
+
+  for (i = 0; i < 32; i++)
+    {
+      tree flag_arg;
+
+      if (!MASK_TEST_BIT (uninit_opnds, i))
+        continue;
+
+      flag_arg = gimple_phi_arg_def (flag_def, i);
+      if (!is_gimple_constant (flag_arg))
+        {
+          gimple flag_arg_def, phi_arg_def;
+          tree phi_arg;
+          unsigned uninit_opnds_arg_phi;
+
+          if (TREE_CODE (flag_arg) != SSA_NAME)
+            return false;
+          flag_arg_def = SSA_NAME_DEF_STMT (flag_arg);
+          if (gimple_code (flag_arg_def) != GIMPLE_PHI)
+            return false;
+
+          phi_arg = gimple_phi_arg_def (phi, i);
+          if (TREE_CODE (phi_arg) != SSA_NAME)
+            return false;
+
+          phi_arg_def = SSA_NAME_DEF_STMT (phi_arg);
+          if (gimple_code (phi_arg_def) != GIMPLE_PHI)
+            return false;
+
+          if (gimple_bb (phi_arg_def) != gimple_bb (flag_arg_def))
+            return false;
+
+          if (!*visited_flag_phis)
+            *visited_flag_phis = pointer_set_create ();
+
+
+          if (pointer_set_insert (*visited_flag_phis, flag_arg_def))
+            return false;
+
+          /* Now recursively prune the uninitialized phi args.  */
+          uninit_opnds_arg_phi = compute_uninit_opnds_pos (phi_arg_def);
+          if (!prune_uninit_phi_opnds_in_unrealizable_paths (
+                  phi_arg_def, uninit_opnds_arg_phi,
+                  flag_arg_def, boundary_cst, cmp_code,
+                  visited_phis, visited_flag_phis))
+            return false;
+
+          pointer_set_delete (*visited_flag_phis, flag_arg_def);
+          continue;
+        }
+
+      /* Now check if the constant is in the guarded range.  */
+      if (is_value_included_in (flag_arg, boundary_cst, cmp_code))
+        {
+          tree opnd;
+          gimple opnd_def;
+
+          /* Now that we know that this undefined edge is not
+             pruned. If the operand is defined by another phi,
+             we can further prune the incoming edges of that
+             phi by checking the predicates of this operands.  */
+
+          opnd = gimple_phi_arg_def (phi, i);
+          opnd_def = SSA_NAME_DEF_STMT (opnd);
+          if (gimple_code (opnd_def) == GIMPLE_PHI)
+            {
+              edge opnd_edge;
+              unsigned uninit_opnds2
+                  = compute_uninit_opnds_pos (opnd_def);
+              gcc_assert (!MASK_EMPTY (uninit_opnds2));
+              opnd_edge = gimple_phi_arg_edge (phi, i);
+              if (!is_use_properly_guarded (phi,
+                                            opnd_edge->src,
+                                            opnd_def,
+                                            uninit_opnds2,
+                                            visited_phis))
+                  return false;
+            }
+          else
+            return false;
+        }
+    }
+
+  return true;
+}
 
 /* A helper function that determines if the predicate set
    of the use is not overlapping with that of the uninit paths.
@@ -873,6 +977,8 @@ use_pred_not_overlap_with_undef_path_pre
   bool swap_cond = false;
   bool invert = false;
   VEC(use_pred_info_t, heap) *the_pred_chain;
+  struct pointer_set_t *visited_flag_phis = NULL;
+  bool all_pruned = false;
 
   gcc_assert (num_preds > 0);
   /* Find within the common prefix of multiple predicate chains
@@ -935,50 +1041,18 @@ use_pred_not_overlap_with_undef_path_pre
   if (cmp_code == ERROR_MARK)
     return false;
 
-  for (i = 0; i < sizeof (unsigned); i++)
-    {
-      tree flag_arg;
-
-      if (!MASK_TEST_BIT (uninit_opnds, i))
-        continue;
-
-      flag_arg = gimple_phi_arg_def (flag_def, i);
-      if (!is_gimple_constant (flag_arg))
-        return false;
-
-      /* Now check if the constant is in the guarded range.  */
-      if (is_value_included_in (flag_arg, boundary_cst, cmp_code))
-        {
-          tree opnd;
-          gimple opnd_def;
-
-          /* Now that we know that this undefined edge is not
-             pruned. If the operand is defined by another phi,
-             we can further prune the incoming edges of that
-             phi by checking the predicates of this operands.  */
+  all_pruned = prune_uninit_phi_opnds_in_unrealizable_paths (phi,
+                                                             uninit_opnds,
+                                                             flag_def,
+                                                             boundary_cst,
+                                                             cmp_code,
+                                                             visited_phis,
+                                                             &visited_flag_phis);
 
-          opnd = gimple_phi_arg_def (phi, i);
-          opnd_def = SSA_NAME_DEF_STMT (opnd);
-          if (gimple_code (opnd_def) == GIMPLE_PHI)
-            {
-              edge opnd_edge;
-              unsigned uninit_opnds2
-                  = compute_uninit_opnds_pos (opnd_def);
-              gcc_assert (!MASK_EMPTY (uninit_opnds2));
-              opnd_edge = gimple_phi_arg_edge (phi, i);
-              if (!is_use_properly_guarded (phi,
-                                            opnd_edge->src,
-                                            opnd_def,
-                                            uninit_opnds2,
-                                            visited_phis))
-                  return false;
-            }
-          else
-            return false;
-        }
-    }
+  if (visited_flag_phis)
+    pointer_set_destroy (visited_flag_phis);
 
-  return true;
+  return all_pruned;
 }
 
 /* Returns true if TC is AND or OR */
Index: gcc/pointer-set.c
===================================================================
--- gcc/pointer-set.c	(revision 165438)
+++ gcc/pointer-set.c	(working copy)
@@ -24,9 +24,9 @@ along with GCC; see the file COPYING3.  
 /* A pointer set is represented as a simple open-addressing hash
    table.  Simplifications: The hash code is based on the value of the
    pointer, not what it points to.  The number of buckets is always a
-   power of 2.  Null pointers are a reserved value.  Deletion is not
-   supported (yet).  There is no mechanism for user control of hash
-   function, equality comparison, initial size, or resizing policy.  */
+   power of 2.  Null pointers are a reserved value. There is no mechanism
+   for user control of hash function, equality comparison, initial size,
+   or resizing policy.  */
 
 struct pointer_set_t
 {
@@ -169,6 +169,34 @@ pointer_set_insert (struct pointer_set_t
   return 0;
 }
 
+/* Delete P from PSET if PSET contains P. Returns nonzero if P is in the set.
+   P must be nonnull. */
+int
+pointer_set_delete (struct pointer_set_t *pset, const void *p)
+{
+  size_t n = hash1 (p, pset->n_slots, pset->log_slots);
+
+  while (true)
+    {
+      if (pset->slots[n] == p)
+        {
+          pset->slots[n] = 0;
+          --pset->n_elements;
+          return 1;
+        }
+      else if (pset->slots[n] == 0)
+        {
+          return 0;
+        }
+      else
+        {
+          ++n;
+          if (n == pset->n_slots)
+            n = 0;
+        }
+    }
+}
+
 /* Pass each pointer in PSET to the function in FN, together with the fixed
    parameter DATA.  If FN returns false, the iteration stops.  */
 
Index: gcc/pointer-set.h
===================================================================
--- gcc/pointer-set.h	(revision 165438)
+++ gcc/pointer-set.h	(working copy)
@@ -26,6 +26,7 @@ void pointer_set_destroy (struct pointer
 
 int pointer_set_contains (const struct pointer_set_t *pset, const void *p);
 int pointer_set_insert (struct pointer_set_t *pset, const void *p);
+int pointer_set_delete (struct pointer_set_t *pset, const void *p);
 void pointer_set_traverse (const struct pointer_set_t *,
 			   bool (*) (const void *, void *),
 			   void *);
Index: gcc/testsuite/g++.dg/uninit-pred-3_a.C
===================================================================
--- gcc/testsuite/g++.dg/uninit-pred-3_a.C	(revision 0)
+++ gcc/testsuite/g++.dg/uninit-pred-3_a.C	(revision 0)
@@ -0,0 +1,77 @@
+/* { dg-do compile } */
+/* { dg-options "-Wuninitialized -O2" } */
+
+/* Multiple initialization paths.  */
+
+typedef long long int64;
+void incr ();
+bool is_valid (int);
+int  get_time ();
+
+class A
+{
+public:
+  A ();
+  ~A () {
+    if (I) delete I;
+  }
+
+private:
+  int* I;
+};
+
+bool get_url (A *);
+bool get_url2 (A *);
+
+class M {
+
+ public:
+ __attribute__ ((always_inline))
+ bool GetC (int *c)  {
+
+    A details_str;
+    /* Intialization path 1  */
+    if (get_url (&details_str))
+      {
+        *c = get_time ();
+        return true;
+      }
+
+    /* insert dtor calls (inlined) into following return paths  */
+    A tmp_str;
+
+    /* Intializtion path 2  */
+    if (get_url2 (&details_str))
+      {
+        *c = get_time ();
+        return true;
+      }
+
+    return false;
+  }
+
+  void do_sth();
+  void do_sth2();
+
+  void P (int64 t)
+    {
+      int cc;
+      if (!GetC (&cc)) /* return flag checked properly */
+        return;
+
+      if (cc <= 0)   /* { dg-bogus "uninitialized" "uninitialized variable warning" } */
+        {
+          this->do_sth();
+          return;
+        }
+
+    do_sth2();
+  }
+};
+
+M* m;
+void test(int x)
+{
+  m = new M;
+  m->P(x);
+}
Index: gcc/testsuite/g++.dg/uninit-pred-3_b.C
===================================================================
--- gcc/testsuite/g++.dg/uninit-pred-3_b.C	(revision 0)
+++ gcc/testsuite/g++.dg/uninit-pred-3_b.C	(revision 0)
@@ -0,0 +1,87 @@
+/* { dg-do compile } */
+/* { dg-options "-Wuninitialized -O2" } */
+
+/* Multiple initialization paths.  */
+
+typedef long long int64;
+void incr ();
+bool is_valid (int);
+int  get_time ();
+
+class A
+{
+public:
+  A ();
+  ~A () {
+    if (I) delete I;
+  }
+
+private:
+  int* I;
+};
+
+bool get_url (A *);
+bool get_url2 (A *);
+bool get_url3 (A *);
+
+class M {
+
+ public:
+ __attribute__ ((always_inline))
+ bool GetC (int *c)  {
+
+    A details_str;
+
+    /* Initialization path 1  */
+    if (get_url (&details_str))
+      {
+        *c = get_time ();
+        return true;
+      }
+
+    /* Destructor call before return*/
+    A tmp_str;
+
+    /* Initialization path 2  */
+    if (get_url2 (&details_str))
+      {
+        *c = get_time ();
+        return true;
+      }
+
+    /* Fail to initialize in this path but
+       still returns true  */
+    if (get_url2 (&details_str))
+      {
+        /* Fail to initialize *c  */
+        return true;
+      }
+
+    return false;
+  }
+
+  void do_sth();
+  void do_sth2();
+
+  void P (int64 t)
+    {
+      int cc; /* { dg-excess-errors "note: 'cc' was declared here" } */
+      if (!GetC (&cc))
+        return;
+
+      if (cc <= 0)   /* { dg-warning "uninitialized" "uninitialized variable warning" } */
+        {
+          this->do_sth();
+          return;
+        }
+
+    do_sth2();
+  }
+};
+
+M* m;
+void test(int x)
+{
+  m = new M;
+  m->P(x);
+}

Attachment: cl
Description: Binary data

Attachment: cl.t
Description: Troff document