From 2d80dbdfc80832217fb4158c3eb0628e360d9c71 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Wed, 24 Jun 2020 13:02:43 -0400
Subject: [PATCH 247/251] FIXME: fixes to leak-detection

---
 gcc/analyzer/engine.cc                   | 42 ++++++++----
 gcc/analyzer/program-state.cc            | 40 +++++++++--
 gcc/analyzer/program-state.h             | 16 +++++
 gcc/analyzer/region-model2.cc            | 84 +++++++++++++++++++-----
 gcc/analyzer/region-model2.h             | 15 +++--
 gcc/testsuite/gcc.dg/analyzer/malloc-1.c |  6 +-
 6 files changed, 159 insertions(+), 44 deletions(-)

diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
index d4c0a58c700..8259c9f022c 100644
--- a/gcc/analyzer/engine.cc
+++ b/gcc/analyzer/engine.cc
@@ -2138,22 +2138,40 @@ exploded_node::detect_leaks (exploded_graph &eg) const
   gcc_assert (get_point ().get_stack_depth () == 1);
 
   const program_state &old_state = get_state ();
-  if (!old_state.m_region_model)
-    return;
 
   /* Work with a temporary copy of the state: pop the frame, and see
      what leaks (via purge_unused_svalues).  */
   program_state new_state (old_state);
 
-  gcc_assert (new_state.m_region_model);
-
-  purge_stats stats;
-  impl_region_model_context ctxt (eg, this,
-				  &old_state, &new_state,
-				  NULL,
-				  get_stmt ());
-  new_state.m_region_model->pop_frame (region_id::null (),
-				       true, &stats, &ctxt);
+  if (!old_state.m_region_model)
+    {
+      gcc_assert (new_state.m_region_model2);
+
+#if 1
+      purge_stats2 stats;
+      impl_region_model2_context ctxt (eg, this,
+				       &old_state, &new_state,
+				       NULL,
+				       get_stmt ());
+      const svalue2 *result = NULL;
+      new_state.m_region_model2->pop_frame (NULL, true, &stats, &result,
+					    &ctxt);
+      program_state::detect_leaks2 (old_state, new_state, result,
+				    eg.get_ext_state (), &ctxt);
+#endif
+    }
+  else
+    {
+      gcc_assert (new_state.m_region_model);
+
+      purge_stats stats;
+      impl_region_model_context ctxt (eg, this,
+				      &old_state, &new_state,
+				      NULL,
+				      get_stmt ());
+      new_state.m_region_model->pop_frame (region_id::null (),
+					   true, &stats, &ctxt);
+    }
 }
 
 /* Dump the successors and predecessors of this enode to OUTF.  */
@@ -3384,7 +3402,7 @@ exploded_graph::process_node (exploded_node *node)
 		impl_region_model2_context ctxt (*this, node,
 						 &old_state, &next_state, &change,
 						 stmt);
-		program_state::detect_leaks2 (old_state, next_state,
+		program_state::detect_leaks2 (old_state, next_state, NULL,
 					      get_ext_state (), &ctxt);
 	      }
 
diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc
index 742afc21417..e8bb20eb185 100644
--- a/gcc/analyzer/program-state.cc
+++ b/gcc/analyzer/program-state.cc
@@ -1679,7 +1679,30 @@ program_state::prune_for_point2 (exploded_graph &eg,
 	    = pm->get_data_for_ssa_name (ssa_name);
 	  if (!per_ssa.needed_at_point_p (point.get_function_point ()))
 	    {
-	      // TODO: see logic below on sm-state
+	      /* Don't purge bindings of SSA names to svalues
+		 that have unpurgable sm-state, so that leaks are
+		 reported at the end of the function, rather than
+		 at the last place that such an SSA name is referred to.
+
+		 But do purge them for temporaries (when SSA_NAME_VAR is
+		 NULL), so that we report for cases where a leak happens when
+		 a variable is overwritten with another value, so that the leak
+		 is reported at the point of overwrite, rather than having
+		 temporaries keep the value reachable until the frame is
+		 popped.  */
+	      const svalue2 *sval
+		= new_state.m_region_model2->get_store_value (reg);
+	      if (!new_state.can_purge_p2 (eg.get_ext_state (), sval)
+		  && SSA_NAME_VAR (ssa_name))
+		{
+		  /* (currently only state maps can keep things
+		     alive).  */
+		  if (logger)
+		    logger->log ("not purging binding for %qE"
+				 " (used by state map)", ssa_name);
+		  continue;
+		}
+
 	      new_state.m_region_model2->purge_region (reg);
 	      num_ssas_purged++;
 	      // TODO: update stats
@@ -1771,7 +1794,7 @@ program_state::prune_for_point2 (exploded_graph &eg,
 					   &new_state,
 					   change,
 					   point.get_stmt ());
-	  detect_leaks2 (*this, new_state, eg.get_ext_state (), &ctxt);
+	  detect_leaks2 (*this, new_state, NULL, eg.get_ext_state (), &ctxt);
 	}
     }
 
@@ -2006,6 +2029,7 @@ log_set_of_svalue2s (logger *logger, const char *name,
 void
 program_state::detect_leaks2 (const program_state &src_state,
 			      const program_state &dest_state,
+			      const svalue2 *extra_sval,
 			      const extrinsic_state &ext_state,
 			      region_model2_context *ctxt)
 {
@@ -2022,13 +2046,21 @@ program_state::detect_leaks2 (const program_state &src_state,
       pp_string (pp, "dest_state: ");
       dest_state.dump_to_pp (ext_state, true, false, pp);
       logger->end_log_line ();
+      if (extra_sval)
+	{
+	  logger->start_log_line ();
+	  pp_string (pp, "extra_sval: ");
+	  extra_sval->dump_to_pp (pp, true);
+	  logger->end_log_line ();
+	}
     }
 
   /* Get svalues reachable from each of src_state and dst_state.  */
   hash_set<const svalue2 *> src_svalues;
   hash_set<const svalue2 *> dest_svalues;
-  src_state.m_region_model2->get_reachable_svalues (&src_svalues);
-  dest_state.m_region_model2->get_reachable_svalues (&dest_svalues);
+  src_state.m_region_model2->get_reachable_svalues (&src_svalues, NULL);
+  dest_state.m_region_model2->get_reachable_svalues (&dest_svalues,
+						     extra_sval);
 
   if (logger)
     {
diff --git a/gcc/analyzer/program-state.h b/gcc/analyzer/program-state.h
index fa854c1afcb..8eff3856ae3 100644
--- a/gcc/analyzer/program-state.h
+++ b/gcc/analyzer/program-state.h
@@ -403,6 +403,21 @@ public:
       }
     return true;
   }
+  bool can_purge_p2 (const extrinsic_state &ext_state,
+		     const svalue2 *sval)
+  {
+    /* Don't purge vars that have non-purgeable sm state, to avoid
+       generating false "leak" complaints.  */
+    int i;
+    sm_state_map2 *smap;
+    FOR_EACH_VEC_ELT (m_checker_states2, i, smap)
+      {
+	const state_machine &sm = ext_state.get_sm (i);
+	if (!sm.can_purge_p (smap->get_state (sval)))
+	  return false;
+      }
+    return true;
+  }
 
   bool can_merge_with_p (const program_state &other,
 			 const extrinsic_state &ext_state,
@@ -417,6 +432,7 @@ public:
 
   static void detect_leaks2 (const program_state &src_state,
 			     const program_state &dest_state,
+			     const svalue2 *extra_sval,
 			     const extrinsic_state &ext_state,
 			     region_model2_context *ctxt);
 
diff --git a/gcc/analyzer/region-model2.cc b/gcc/analyzer/region-model2.cc
index 7665e0029db..a24780feca2 100644
--- a/gcc/analyzer/region-model2.cc
+++ b/gcc/analyzer/region-model2.cc
@@ -5445,10 +5445,15 @@ add_svalues_to_set_cb (const svalue2 *sval, hash_set<const svalue2 *> *out)
    Traverse the region2s in this model, determining what region2s are
    reachable from the store.
 
+   If EXTRA_SVAL is non-NULL, treat it as an additional "root"
+   for reachability (for handling return values from functions when
+   analyzing return of the only function on the stack).
+
    Find svalues that haven't leaked.    */
 
 void
-region_model2::get_reachable_svalues (hash_set<const svalue2 *> *out)
+region_model2::get_reachable_svalues (hash_set<const svalue2 *> *out,
+				      const svalue2 *extra_sval)
 {
   reachable_region2s reachable_regs (&m_store, m_mgr);
 
@@ -5457,6 +5462,9 @@ region_model2::get_reachable_svalues (hash_set<const svalue2 *> *out)
   m_store.for_each_cluster (reachable_region2s::init_cluster_cb,
 			    &reachable_regs);
 
+  if (extra_sval)
+    reachable_regs.handle_sval (extra_sval);
+
 #if 0
   /* Mark all locals in all frames (and what's reachable from them).  */
   for (const frame_region2 *iter_frame = get_current_frame ();
@@ -5498,6 +5506,15 @@ region_model2::get_reachable_svalues (hash_set<const svalue2 *> *out)
      still have their initial values (but not for uninit values).  */
 #endif
 
+#if 1
+  /* Populate *OUT based on the values that were reachable.  */
+  for (hash_set<const svalue2 *>::iterator iter
+	 = reachable_regs.begin_reachable_svals ();
+       iter != reachable_regs.end_reachable_svals (); ++iter)
+    {
+      out->add (*iter);
+    }
+#else
   /* Populate *OUT based on the clusters that were reachable.  */
   for (hash_set<const region2 *>::iterator iter = reachable_regs.begin ();
        iter != reachable_regs.end (); ++iter)
@@ -5514,6 +5531,7 @@ region_model2::get_reachable_svalues (hash_set<const svalue2 *> *out)
 	  out->add (init);
 	}
     }
+#endif
 }
 
 /* Update this model for the RETURN_STMT, using CTXT to report any
@@ -5587,7 +5605,7 @@ region_model2::on_longjmp (const gcall *longjmp_call, const gcall *setjmp_call,
   while (get_stack_depth () > setjmp_stack_depth)
     {
       /* Don't purge unused svalue2s yet, as we're using fake_retval_sval.  */
-      pop_frame (NULL, false, NULL, ctxt);
+      pop_frame (NULL, false, NULL, NULL, ctxt);
     }
 
   gcc_assert (get_stack_depth () == setjmp_stack_depth);
@@ -7157,7 +7175,7 @@ region_model2::update_for_return_superedge (const return_superedge &return_edge,
     }
 
   purge_stats2 stats;
-  pop_frame (result_dst_reg, true, &stats, ctxt);
+  pop_frame (result_dst_reg, true, &stats, NULL, ctxt);
 
   // TODO: do something with the stats?
 
@@ -7374,11 +7392,27 @@ region_model2::get_current_function () const
 }
 
 /* Pop the topmost frame_region2 from this region_model2's stack;
-   see the comment for stack_region2::pop_frame.  */
+
+   FIXME:
+   If RESULT_DST_RID is non-null, copy any return value from the frame
+   into RESULT_DST_RID's region.
+
+   FIXME:
+   Purge the frame region and all its descendent regions.
+   Convert any pointers that point into such regions into
+   POISON_KIND_POPPED_STACK svalues.
+
+   FIXME:
+   If PURGE, then purge all unused svalues, with the exception of any
+   returned values.
+
+   FIXME:
+   Accumulate stats on purged entities into STATS.  */
 
 void
 region_model2::pop_frame (const region2 *result_dst_reg,
 			  bool purge, purge_stats2 *out,
+			  const svalue2 **out_result,
 			  region_model2_context *ctxt)
 {
   gcc_assert (m_current_frame);
@@ -7399,6 +7433,8 @@ region_model2::pop_frame (const region2 *result_dst_reg,
 		       get_lvalue (result, ctxt),
 		       ctxt);
 	}
+      if (out_result)
+	*out_result = get_rvalue (result, ctxt);
       if (purge)
 	{
 #if 0
@@ -7423,7 +7459,7 @@ region_model2::pop_frame (const region2 *result_dst_reg,
   /* Pop the frame.  */
   m_current_frame = m_current_frame->get_calling_frame ();
 
-#if 0
+#if 1
   unbind_region2_and_descendents (frame_reg,
 				  POISON_KIND_POPPED_STACK,
 				  out,
@@ -7742,28 +7778,40 @@ region_model2::get_descendents (region2 *reg, region2_id_set *out,
     }
 }
 #endif
+
+// FIXME
 /* Delete RID and all descendent region2s.
    Find any pointers to such region2s; convert them to
    poisoned values of kind PKIND.
    Accumulate stats on purged entities into STATS.  */
-#if 0
+
 void
-region_model2::delete_region2_and_descendents (region2 *reg,
-					     enum poison_kind pkind,
-					     purge_stats *stats,
-					     logger *logger)
+region_model2::unbind_region2_and_descendents (const region2 *reg,
+					       enum poison_kind pkind,
+					       purge_stats2 *stats,
+					       logger *logger)
 {
-  /* Find all child and descendent region2s.  */
-  region2_id_set descendents (this);
-  get_descendents (rid, &descendents, region2_id::null ());
+  /* Gather a set of base regions to be unbound.  */
+  hash_set<const region2 *> base_regs;
+  for (store2::cluster_map_t::iterator iter = m_store.begin ();
+       iter != m_store.end (); ++iter)
+    {
+      const region2 *iter_base_reg = (*iter).first;
+      if (iter_base_reg->descendent_of_p (reg))
+	base_regs.add (iter_base_reg);
+    }
+  for (hash_set<const region2 *>::iterator iter = base_regs.begin ();
+       iter != base_regs.end (); ++iter)
+    m_store.purge_region (m_mgr->get_store2_manager (), (*iter));
 
+  // TODO: unit test for descendent_of_p
+  // TODO: poison pointers to regions that are descendents of REG.
+#if 0
   /* Find any pointers to such region2s; convert to poisoned.  */
   poison_any_pointers_to_bad_region2s (descendents, pkind);
-
-  /* Delete all such region2s.  */
-  purge_region2s (descendents, stats, logger);
-}
 #endif
+}
+
 /* Find any pointers to region2s within BAD_REGION2S; convert them to
    poisoned values of kind PKIND.  */
 #if 0
@@ -9395,7 +9443,7 @@ test_stack_frames ()
 
   /* Pop the "child_fn" frame from the stack.  */
   purge_stats2 purged;
-  model.pop_frame (NULL, true, &purged, &ctxt);
+  model.pop_frame (NULL, true, &purged, NULL, &ctxt);
 
 #if 0
   /* We should have purged the unknown values for x and y. */
diff --git a/gcc/analyzer/region-model2.h b/gcc/analyzer/region-model2.h
index 97ac2bced52..70f98cae282 100644
--- a/gcc/analyzer/region-model2.h
+++ b/gcc/analyzer/region-model2.h
@@ -2175,7 +2175,8 @@ class region_model2
 #endif
   void handle_unrecognized_call (const gcall *call,
 				 region_model2_context *ctxt);
-  void get_reachable_svalues (hash_set<const svalue2 *> *out);
+  void get_reachable_svalues (hash_set<const svalue2 *> *out,
+			      const svalue2 *extra_sval);
   bool on_greturn (const greturn *stmt,
 		   region_model2_context *ctxt);
   void on_setjmp (const gcall *stmt, const exploded_node *enode,
@@ -2201,6 +2202,7 @@ class region_model2
   function * get_current_function () const;
   void pop_frame (const region2 *result_dst,
 		  bool purge, purge_stats2 *stats,
+		  const svalue2 **out_result,
 		  region_model2_context *ctxt);
   int get_stack_depth () const;
   const frame_region2 *get_frame_at_index (int index) const;
@@ -2291,12 +2293,13 @@ class region_model2
 #if 0
   void get_descendents (region2_id rid, region2_id_set *out,
 			region2_id exclude_rid) const;
-
-  void delete_region2_and_descendents (region2_id rid,
-				      enum poison_kind pkind,
-				      purge_stats *stats,
-				      logger *logger);
 #endif
+
+  void unbind_region2_and_descendents (const region2 *reg,
+				       enum poison_kind pkind,
+				       purge_stats2 *stats,
+				       logger *logger);
+
   bool can_merge_with_p (const region_model2 &other_model,
 			 const program_point &point,
 			 region_model2 *out_model) const;
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-1.c b/gcc/testsuite/gcc.dg/analyzer/malloc-1.c
index 3024e546137..fe4e55d24c6 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-1.c
@@ -529,10 +529,8 @@ struct link global_link;
 void test_43 (void)
 {
   global_link.m_ptr = malloc (sizeof (struct link)); /* { dg-message "allocated here" } */
-  global_link.m_ptr = NULL;
-} /* { dg-warning "leak of '<unknown>'" } */
-/* TODO: should be more precise than just '<unknown>', and
-   ideally would be at the assigment to NULL.  */
+  global_link.m_ptr = NULL; /* { dg-warning "leak of 'global_link.m_ptr'" } */
+}
 
 struct link *global_ptr;
 
-- 
2.26.2

