From f81ea4ec303508bc064ad6ce44ecee42ad8421f6 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Mon, 15 Jul 2019 12:15:42 -0400
Subject: [PATCH 088/108] FIXME: rework canonicalization to handle constants
 only reached via constraints

---
 gcc/analyzer/constraint-manager.cc                |  10 +-
 gcc/analyzer/region-model.cc                      | 131 ++++++++++++++++++----
 gcc/testsuite/gcc.dg/analyzer/malloc-vs-local-2.c |  14 +--
 gcc/testsuite/gcc.dg/analyzer/malloc-vs-local-3.c |   2 +-
 gcc/testsuite/gcc.dg/analyzer/malloc-vs-local.c   |   4 +-
 gcc/testsuite/gcc.dg/analyzer/paths-3.c           |   4 +-
 gcc/testsuite/gcc.dg/analyzer/paths-7.c           |   4 +-
 7 files changed, 129 insertions(+), 40 deletions(-)

diff --git a/gcc/analyzer/constraint-manager.cc b/gcc/analyzer/constraint-manager.cc
index 27e8f14..b1618e2 100644
--- a/gcc/analyzer/constraint-manager.cc
+++ b/gcc/analyzer/constraint-manager.cc
@@ -336,9 +336,9 @@ equiv_class::del (svalue_id sid)
 svalue_id
 equiv_class::get_representative () const
 {
-#if 0
-  if (m_constant)
-    return path_var (m_constant, 0);
+#if 1
+  if (!m_cst_sid.null_p ())
+    return m_cst_sid;
   else
 #endif
     {
@@ -362,7 +362,7 @@ equiv_class::remap_svalue_ids (const svalue_id_map &map)
 /* Comparator for use by equiv_class::canonicalize.  */
 
 static int
-svalue_id_cmp (const void *p1, const void *p2)
+svalue_id_cmp_by_id (const void *p1, const void *p2)
 {
   const svalue_id *sid1 = (const svalue_id *)p1;
   const svalue_id *sid2 = (const svalue_id *)p2;
@@ -374,7 +374,7 @@ svalue_id_cmp (const void *p1, const void *p2)
 void
 equiv_class::canonicalize ()
 {
-  m_vars.qsort (svalue_id_cmp);
+  m_vars.qsort (svalue_id_cmp_by_id);
 }
 
 ////////////////////////////////////////////////////////////////////////////
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index bd24aaa..7a64249 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -1444,6 +1444,23 @@ tree_cmp (const_tree t1, const_tree t2)
     case INTEGER_CST:
       return tree_int_cst_compare (t1, t2);
 
+    case REAL_CST:
+      {
+	real_value *rv1 = TREE_REAL_CST_PTR (t1);
+	real_value *rv2 = TREE_REAL_CST_PTR (t2);
+	if (real_compare (LT_EXPR, rv1, rv2))
+	  return -1;
+	if (real_compare (LT_EXPR, rv2, rv1))
+	  return 1;
+	return 0;
+      }
+
+    case STRING_CST:
+      // FIXME: does this assume that they're nil-terminated?
+      // FIXME: are they?
+      return strcmp (TREE_STRING_POINTER (t1),
+		     TREE_STRING_POINTER (t2));
+
     default:
       gcc_unreachable ();
       break;
@@ -2562,6 +2579,49 @@ region_model::validate () const
     }
 }
 
+/* Global data for use by svalue_id_cmp_by_svalue.  */
+
+static region_model *svalue_id_cmp_by_svalue_model = NULL;
+
+/* Comparator for use by region_model::canonicalize.  */
+
+static int
+svalue_id_cmp_by_svalue (const void *p1, const void *p2)
+{
+  const svalue_id *sid1 = (const svalue_id *)p1;
+  const svalue_id *sid2 = (const svalue_id *)p2;
+  gcc_assert (!sid1->null_p ());
+  gcc_assert (!sid2->null_p ());
+  gcc_assert (svalue_id_cmp_by_svalue_model);
+  const svalue &sval1 = *svalue_id_cmp_by_svalue_model->get_svalue (*sid1);
+  const svalue &sval2 = *svalue_id_cmp_by_svalue_model->get_svalue (*sid2);
+  if (sval1.get_kind () != sval2.get_kind ())
+    return sval1.get_kind () - sval2.get_kind ();
+  switch (sval1.get_kind ())
+    {
+    default:
+      gcc_unreachable ();
+    case SK_CONSTANT:
+      {
+	tree cst1 = ((const constant_svalue &)sval1).get_constant ();
+	tree cst2 = ((const constant_svalue &)sval2).get_constant ();
+	return tree_cmp (cst1, cst2);
+      }
+      break;
+    case SK_POISONED:
+      {
+	const poisoned_svalue &sval1_sub
+	  = (const poisoned_svalue &)sval1;
+	const poisoned_svalue &sval2_sub
+	  = (const poisoned_svalue &)sval2;
+	return sval1_sub.get_poison_kind () - sval2_sub.get_poison_kind ();
+      }
+      break;
+    }
+  // TODO
+}
+
+
 /* Reorder the regions and svalues into a deterministic "canonical" order,
    to maximize the chance of equality.
    If non-NULL, notify CTXT about the svalue id remapping.  */
@@ -2574,29 +2634,57 @@ region_model::canonicalize (region_model_context *ctxt)
   return;
 #endif
 
-  /* First pass: walk all regions in a deterministic order, generating
-     a rid and sid map.  */
+  /* Walk all regions and values in a deterministic order, visiting
+     rids and sids, generating a rid and sid map.  */
   canonicalization c (*this);
+
+  /* (1): Walk all svalues, putting poisoned values first, then
+     constants, sorting the constants (thus imposing an ordering on any
+     constants that are purely referenced by constraints).
+     Ignore other svalues for now.  */
+  {
+    unsigned i;
+    auto_vec<svalue_id> sids;
+    svalue *sval;
+    FOR_EACH_VEC_ELT (m_svalues, i, sval)
+      {
+	if (sval->get_kind () == SK_POISONED
+	    || sval->get_kind () == SK_CONSTANT)
+	  sids.safe_push (svalue_id::from_int (i));
+      }
+    svalue_id_cmp_by_svalue_model = this;
+    sids.qsort (svalue_id_cmp_by_svalue);
+    svalue_id_cmp_by_svalue_model = NULL;
+    svalue_id *sid;
+    FOR_EACH_VEC_ELT (sids, i, sid)
+      c.walk_sid (*sid);
+  }
+
+  /* (2): Walk all regions in a deterministic order.  */
   c.walk_rid (m_root_rid);
 
-  /* Second pass: ensure we've visited everything, as we don't want to purge
-     at this stage.  */
-  unsigned i;
-  region *region;
-  FOR_EACH_VEC_ELT (m_regions, i, region)
-    c.walk_rid (region_id::from_int (i));
-  svalue *sval;
-  FOR_EACH_VEC_ELT (m_svalues, i, sval)
-    c.walk_sid (svalue_id::from_int (i));
+  /* (3): Ensure we've visited everything, as we don't want to purge
+     at this stage.  Anything we visit for the first time here has
+     arbitrary order.  */
+  {
+    unsigned i;
+    region *region;
+    FOR_EACH_VEC_ELT (m_regions, i, region)
+      c.walk_rid (region_id::from_int (i));
+    svalue *sval;
+    FOR_EACH_VEC_ELT (m_svalues, i, sval)
+      c.walk_sid (svalue_id::from_int (i));
+  }
 
-  /* Third pass: reorder the regions and values.  */
+  /* (4): Apply the reordering to the regions and values.  */
   remap_svalue_ids (c.m_sid_map);
   remap_region_ids (c.m_rid_map);
   if (ctxt)
     ctxt->remap_svalue_ids (c.m_sid_map);
 
-  /* Canonicalize the constraint_manager (it has already had its
-     svalue_ids remapped above).  */
+  /* (5): Canonicalize the constraint_manager (it has already had its
+     svalue_ids remapped above).  This makes use of the new svalue_id
+     values, and so must happen last.  */
   m_constraints->canonicalize (get_num_svalues ());
 
   validate ();
@@ -5147,25 +5235,26 @@ test_canonicalization_2 ()
 }
 
 /* Verify that constraints for
-     x > 0 && y > 0
+     x > 3 && y > 42
    and
-     y > 0 && x > 0
+     y > 42 && x > 3
    are equal after canonicalization.  */
 
 static void
 test_canonicalization_3 ()
 {
-  tree int_0 = build_int_cst (integer_type_node, 0);
+  tree int_3 = build_int_cst (integer_type_node, 3);
+  tree int_42 = build_int_cst (integer_type_node, 42);
   tree x = build_global_decl ("x", integer_type_node);
   tree y = build_global_decl ("y", integer_type_node);
 
   region_model model0;
-  model0.add_constraint (x, GT_EXPR, int_0, NULL);
-  model0.add_constraint (y, GT_EXPR, int_0, NULL);
+  model0.add_constraint (x, GT_EXPR, int_3, NULL);
+  model0.add_constraint (y, GT_EXPR, int_42, NULL);
 
   region_model model1;
-  model1.add_constraint (y, GT_EXPR, int_0, NULL);
-  model1.add_constraint (x, GT_EXPR, int_0, NULL);
+  model1.add_constraint (y, GT_EXPR, int_42, NULL);
+  model1.add_constraint (x, GT_EXPR, int_3, NULL);
 
   model0.canonicalize (NULL);
   model1.canonicalize (NULL);
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local-2.c b/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local-2.c
index e72bac0..216a5bc 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local-2.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local-2.c
@@ -39,9 +39,9 @@ int test_repeated_predicate_1 (int n)
     result = sum;
   }
 
-  __analyzer_dump_exploded_nodes (0); /* { dg-warning "5 exploded nodes" } */
-  __analyzer_dump_exploded_nodes (0); /* { dg-warning "5 exploded nodes" } */
-  // FIXME: why 5 here?
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "7 exploded nodes" } */
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "6 exploded nodes" } */
+  // FIXME: why 7/6 here?
 
   if (n > 10)
     free (ptr); /* { dg-bogus "not on the heap" "" { xfail *-*-* } } */
@@ -108,8 +108,8 @@ int test_explicit_flag (int n)
     result = sum;
   }
 
-  __analyzer_dump_exploded_nodes (0); /* { dg-warning "5 exploded nodes" } */
-  // FIXME: why 5 here?
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "7 exploded nodes" } */
+  // FIXME: why 7 here?
 
   if (need_to_free)
     free (ptr); /* { dg-bogus "not on the heap" } */
@@ -143,8 +143,8 @@ int test_pointer_comparison (int n)
     result = sum;
   }
 
-  __analyzer_dump_exploded_nodes (0); /* { dg-warning "5 exploded nodes" } */
-  // FIXME: why 5 here?
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "7 exploded nodes" } */
+  // FIXME: why 7 here?
 
   if (ptr != buf)
     free (ptr); /* { dg-bogus "not on the heap" } */
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local-3.c b/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local-3.c
index d3bff3f..f70a91b 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local-3.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local-3.c
@@ -39,7 +39,7 @@ int test_1 (int n)
   }
 
   __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 exploded nodes" "" { xfail *-*-* } } */
-  /* { dg-bogus "5 exploded nodes" "" { xfail *-*-* } .-1 } */
+  /* { dg-bogus "6 exploded nodes" "" { xfail *-*-* } .-1 } */
   /* TODO(xfail): should be 2 nodes here, for the malloc-vs-non-malloc case; erroneously getting more.  */
 
   return result; /* { dg-message "leak of 'ptr'" } */
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local.c b/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local.c
index 07b9cca..fea68e7 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local.c
@@ -107,8 +107,8 @@ int test_explicit_flag (int n)
 
   result = do_stuff (ptr, n);
 
-  __analyzer_dump_exploded_nodes (0); /* { dg-warning "6 exploded nodes" } */
-  // FIXME: why 6 here?
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "7 exploded nodes" } */
+  // FIXME: why 7 here?
 
   if (need_to_free)
     free (ptr); /* { dg-bogus "not on the heap" } */
diff --git a/gcc/testsuite/gcc.dg/analyzer/paths-3.c b/gcc/testsuite/gcc.dg/analyzer/paths-3.c
index cea8c83..99de266 100644
--- a/gcc/testsuite/gcc.dg/analyzer/paths-3.c
+++ b/gcc/testsuite/gcc.dg/analyzer/paths-3.c
@@ -13,7 +13,7 @@ int test_1 (int a, int b)
       p = malloc (32);
 
   __analyzer_dump_exploded_nodes (0); /* { dg-warning "3 exploded nodes" } */
-  __analyzer_dump_exploded_nodes (0); /* { dg-warning "3 exploded node" } */
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 exploded nodes" } */
 
   if (a > 5)
     {
@@ -37,7 +37,7 @@ int test_2 (int a, int b)
       p = malloc (32);
 
   __analyzer_dump_exploded_nodes (0); /* { dg-warning "3 exploded nodes" } */
-  __analyzer_dump_exploded_nodes (0); /* { dg-warning "3 exploded nodes" } */
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 exploded nodes" } */
 
   if (a > 6) /* different condition */
     {
diff --git a/gcc/testsuite/gcc.dg/analyzer/paths-7.c b/gcc/testsuite/gcc.dg/analyzer/paths-7.c
index fff4216..def592d 100644
--- a/gcc/testsuite/gcc.dg/analyzer/paths-7.c
+++ b/gcc/testsuite/gcc.dg/analyzer/paths-7.c
@@ -52,8 +52,8 @@ int test_2 (int flag, int *p, int n)
     sum += foo (p[i]); /* { dg-bogus "uninitialized" } */
   result = sum;
 
-  __analyzer_dump_exploded_nodes (0); /* { dg-warning "5 exploded nodes" } */
-  // FIXME: why 5 here?
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "6 exploded nodes" } */
+  // FIXME: why 6 here?
 
   free (ptr); /* { dg-warning "double-free of 'ptr'" } */
   return result;
-- 
1.8.5.3

