From 82f305a5a8279168a5d1f56db56a9d10474020e0 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Fri, 19 Jul 2019 15:52:27 -0400
Subject: [PATCH 107/108] FIXME: fix merging of sm_state_map

---
 gcc/analyzer/impl-4.cc                            | 97 +++++++++++++----------
 gcc/testsuite/gcc.dg/analyzer/malloc-vs-local-2.c | 45 +++++++++--
 gcc/testsuite/gcc.dg/analyzer/malloc-vs-local-3.c |  3 +-
 gcc/testsuite/gcc.dg/analyzer/malloc-vs-local.c   | 16 ++--
 gcc/testsuite/gcc.dg/analyzer/paths-3.c           |  8 +-
 gcc/testsuite/gcc.dg/analyzer/paths-7.c           |  8 +-
 6 files changed, 109 insertions(+), 68 deletions(-)

diff --git a/gcc/analyzer/impl-4.cc b/gcc/analyzer/impl-4.cc
index cf5223f..e10f23e 100644
--- a/gcc/analyzer/impl-4.cc
+++ b/gcc/analyzer/impl-4.cc
@@ -721,8 +721,8 @@ public:
 
   sm_state_map *clone () const;
 
-  sm_state_map *maybe_clone_with_remapping
-    (const svalue_id_merger_mapping &sid_mapping) const;
+  sm_state_map *
+  clone_with_remapping (const one_way_svalue_id_map &id_map) const;
 
   void print (const state_machine &sm, pretty_printer *pp) const;
   void dump (const state_machine &sm) const;
@@ -2533,7 +2533,7 @@ program_point::cmp_within_supernode (const program_point &point_a,
 
 ////////////////////////////////////////////////////////////////////////////
 
-/* FIXME.  */
+/* Clone the sm_state_map.  */
 
 sm_state_map *
 sm_state_map::clone () const
@@ -2541,20 +2541,10 @@ sm_state_map::clone () const
   return new sm_state_map (*this);
 }
 
-/* Part of program_state merging with equal sm_state_map, after their two
-   region_models have been merged.
-   Attempt to create a clone of this sm_state_map for the merged
-   program_state, applying SID_MAPPING to the svalue_ids in this map.
-   Check that each sm-state's svalue_id mapped in the same way from A/B to
-   the merged region and remap accordingly; otherwise, the program_states
-   aren't mergable.
-
-   Return a new sm_state_map if successful, or NULL if the program_states
-   should not be mergeable.  */
+/* Clone this sm_state_map, remapping all svalue_ids within it with ID_MAP.  */
 
 sm_state_map *
-sm_state_map::maybe_clone_with_remapping
-  (const svalue_id_merger_mapping &sid_mapping) const
+sm_state_map::clone_with_remapping (const one_way_svalue_id_map &id_map) const
 {
   sm_state_map *result = new sm_state_map ();
   for (typename map_t::iterator iter = m_map.begin ();
@@ -2562,24 +2552,9 @@ sm_state_map::maybe_clone_with_remapping
        ++iter)
     {
       svalue_id sid = (*iter).first;
-      svalue_id a_to_m_sid
-	= sid_mapping.m_map_from_a_to_m.get_dst_for_src (sid);
-      svalue_id b_to_m_sid
-	= sid_mapping.m_map_from_b_to_m.get_dst_for_src (sid);
-      /* Check that the input svalue_id in A and B mapped to the same
-	 non-null thing in the merged region_model.  */
-      if (a_to_m_sid == b_to_m_sid
-	  && !a_to_m_sid.null_p ())
-	{
-	  entry_t e = (*iter).second;
-	  result->m_map.put (a_to_m_sid, e);
-	}
-      else
-	{
-	  /* Otherwise we can't merge the two program_states.  */
-	  delete result;
-	  return NULL;
-	}
+      entry_t e = (*iter).second;
+      id_map.update (&e.m_origin);
+      result->m_map.put (id_map.get_dst_for_src (sid), e);
     }
   return result;
 }
@@ -3732,14 +3707,17 @@ program_state::can_merge_with_p (const program_state &other,
 				 const extrinsic_state &ext_state,
 				 program_state *out) const
 {
-  /* Don't attempt to merge if there are sm-differences between the
-     states: these are likely to be interesting to end-users, and
-     worth exploring.  */
+  /* FIXME:  initially I had an early reject here if there
+     are sm-differences between the states.  However, this was
+     falsely rejecting merger opportunities for states where the
+     only difference was in svalue_id ordering.  */
   int i;
   sm_state_map *smap;
+#if 0
   FOR_EACH_VEC_ELT (m_checker_states, i, smap)
     if (!(*smap == *other.m_checker_states[i]))
       return false;
+#endif
 
   program_state result (ext_state);
   svalue_id_merger_mapping sid_mapping (*m_region_model,
@@ -3754,16 +3732,30 @@ program_state::can_merge_with_p (const program_state &other,
   FOR_EACH_VEC_ELT (result.m_checker_states, i, smap)
     delete smap;
   result.m_checker_states.truncate (0);
+
+  /* Remap this and other's m_checker_states using sid_mapping.
+     Only merge states that have equality between the two end-results:
+     sm-state differences are likely to be interesting to end-users, and
+     hence are worth exploring as separate paths in the exploded graph.  */
   FOR_EACH_VEC_ELT (m_checker_states, i, smap)
     {
-      // FIXME: use sid_mapping to do this...
-      /* Each svalue_id has to map to the same non-null thing,
-	 or we must fail.  */
-      sm_state_map *merged_smap
-	= smap->maybe_clone_with_remapping (sid_mapping);
-      if (!merged_smap)
-	return false;
-      result.m_checker_states.safe_push (merged_smap);
+      sm_state_map *other_smap = other.m_checker_states[i];
+      sm_state_map *remapped_a_smap
+	= smap->clone_with_remapping (sid_mapping.m_map_from_a_to_m);
+      sm_state_map *remapped_b_smap
+	= other_smap->clone_with_remapping (sid_mapping.m_map_from_b_to_m);
+      if (*remapped_a_smap == *remapped_b_smap)
+	{
+	  result.m_checker_states.safe_push (remapped_a_smap);
+	  delete remapped_b_smap;
+	}
+      else
+	{
+	  /* Don't merge if there are sm-state differences.  */
+	  delete remapped_a_smap;
+	  delete remapped_b_smap;
+	  return false;
+	}
     }
 
   impl_region_model_context ctxt (&result, ext_state);
@@ -3922,6 +3914,23 @@ exploded_node::dump_dot (graphviz_out *gv, const dump_args_t &) const
   pp_newline (pp);
 #endif
 
+#if 1
+  // FIXME: copied-and-pasted from program_state::dump_dot
+  {
+  int i;
+  sm_state_map *smap;
+  FOR_EACH_VEC_ELT (m_ps.m_state.m_checker_states, i, smap)
+    {
+      if (!smap->is_empty_p ())
+	{
+	  pp_printf (pp, "%s: ", m_ext_state.get_name (i));
+	  smap->print (m_ext_state.get_sm (i), pp);
+	  pp_newline (pp);
+	}
+    }
+}
+#endif
+
   pp_write_text_as_dot_label_to_stream (pp, /*for_record=*/true);
 
   //TODO: dump_bb_for_graph (pp, bb);
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 16f4a5f..d4b6979 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,42 @@ int test_repeated_predicate_1 (int n)
     result = sum;
   }
 
-  __analyzer_dump_exploded_nodes (0); /* { dg-warning "4 exploded nodes" } */
-  __analyzer_dump_exploded_nodes (0); /* { dg-warning "4 exploded nodes" } */
-  // FIXME: why 4 here?
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 exploded nodes" } */
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 exploded nodes" } */
+
+  if (n > 10)
+    free (ptr); /* { dg-bogus "not on the heap" } */
+
+  return result; /* { dg-bogus "leak" } */
+}
+
+/* As above, but with just one loop.  */
+
+int test_repeated_predicate_1a (int n)
+{
+  int buf[10];
+  int *ptr;
+  int result;
+
+  if (n > 10)
+    ptr = (int *)malloc (sizeof (int) * n);
+  else
+    ptr = buf;
+
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 exploded nodes" } */
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 exploded nodes" } */
+
+  {
+    int *p = ptr;
+    int sum = 0;
+    int i;
+    for (i = 0; i < n; i++)
+      p[i] = i;
+    result = sum;
+  }
+
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 exploded nodes" } */
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 exploded nodes" } */
 
   if (n > 10)
     free (ptr); /* { dg-bogus "not on the heap" } */
@@ -104,8 +137,7 @@ int test_explicit_flag (int n)
     result = sum;
   }
 
-  __analyzer_dump_exploded_nodes (0); /* { dg-warning "4 exploded nodes" } */
-  // FIXME: why 4 here?
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 exploded nodes" } */
 
   if (need_to_free)
     free (ptr); /* { dg-bogus "not on the heap" } */
@@ -139,8 +171,7 @@ int test_pointer_comparison (int n)
     result = sum;
   }
 
-  __analyzer_dump_exploded_nodes (0); /* { dg-warning "4 exploded nodes" } */
-  // FIXME: why 4 here?
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 exploded nodes" } */
 
   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 b76f06b..9d1324b 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local-3.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local-3.c
@@ -40,7 +40,8 @@ int test_1 (int n)
 
   __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 exploded nodes" } */
 
-  return result; /* { dg-message "leak of 'ptr'" } */
+  return result; /* { dg-message "leak of 'p'" } */
+  /* FIXME: should this be 'ptr'?  */
 }
 
 /* A simpler version of the above.  */
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local.c b/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local.c
index d1de5cb..d1eb596 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-vs-local.c
@@ -48,10 +48,10 @@ int test_repeated_predicate_1 (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 "6 exploded nodes" } */
-  // FIXME: why 6 here?
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "4 exploded nodes" } */
+  // FIXME: why 4 here?
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "4 exploded nodes" } */
+  // FIXME: why 4 here?
 
   if (n > 10)
     free (ptr); /* { dg-bogus "not on the heap" } */
@@ -105,8 +105,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 "4 exploded nodes" } */
+  // FIXME: why 4 here?
 
   if (need_to_free)
     free (ptr); /* { dg-bogus "not on the heap" } */
@@ -131,8 +131,8 @@ int test_pointer_comparison (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 "4 exploded nodes" } */
+  // FIXME: why 4 here?
 
   if (ptr != buf)
     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 738ce98..efdeb51 100644
--- a/gcc/testsuite/gcc.dg/analyzer/paths-3.c
+++ b/gcc/testsuite/gcc.dg/analyzer/paths-3.c
@@ -12,8 +12,8 @@ int test_1 (int a, int b)
     else
       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 "4 exploded nodes" } */
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 exploded nodes" } */
 
   if (a > 5)
     {
@@ -34,8 +34,8 @@ int test_2 (int a, int b)
     else
       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 "4 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 f841756..e5ac2d9 100644
--- a/gcc/testsuite/gcc.dg/analyzer/paths-7.c
+++ b/gcc/testsuite/gcc.dg/analyzer/paths-7.c
@@ -23,8 +23,7 @@ int test (int flag, void *ptr, int *p, int n)
     sum += foo (p[i]); /* { dg-bogus "uninitialized" } */
   result = sum;
 
-  __analyzer_dump_exploded_nodes (0); /* { dg-warning "3 exploded nodes" } */
-  // FIXME: why 3 here?
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 exploded nodes" } */
 
   if (flag)
     free (ptr); /* { dg-warning "double-free of 'ptr'" } */
@@ -52,8 +51,9 @@ 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 "6 exploded nodes" } */
-  // FIXME: why 6 here?
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "4 exploded nodes" } */
+  __analyzer_dump_exploded_nodes (0); /* { dg-warning "4 exploded nodes" } */
+  // FIXME: why 4 here?
 
   free (ptr); /* { dg-warning "double-free of 'ptr'" } */
   return result;
-- 
1.8.5.3

