[committed] analyzer: fix false leak involving params [PR98969]

Message ID 20210217154647.1730244-1-dmalcolm@redhat.com
State New
Headers show
Series
  • [committed] analyzer: fix false leak involving params [PR98969]
Related show

Commit Message

Kewen.Lin via Gcc-patches Feb. 17, 2021, 3:46 p.m.
This patch updates the svalue liveness code so that the initial value
of parameters at top-level functions to the analysis are treated as
live (since the values are presumably still live within the
outside-of-the-analysis calling code).

This fixes the false leak in PR analyzer/98969 seen on:

void
test (long int i)
{
  struct foo *f = (struct foo *)i;
  f->expr = __builtin_malloc (1024);
}

since the calling code can presumably still access the allocated
buffer via:
  ((struct foo *)i)->expr

The patch also removes the expected leak warnings from
g++.dg/analyzer/pr99064.C and gcc.dg/analyzer/pr96841.c, which now
appear to me to be false positives.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to trunk as r11-7270-ge0139b2a912585496f23c352f0e2c56895f78fbf.

gcc/analyzer/ChangeLog:
	PR analyzer/98969
	* constraint-manager.cc (dead_svalue_purger::should_purge_p):
	Update for change to svalue::live_p.
	* program-state.cc (sm_state_map::on_liveness_change): Likewise.
	(program_state::detect_leaks): Likewise.
	* region-model-reachability.cc (reachable_regions::init_cluster):
	When dealing with a symbolic region, if the underlying pointer is
	implicitly live, add the region to the reachable regions.
	* region-model.cc (region_model::compare_initial_and_pointer):
	Move logic for detecting initial values of params to
	initial_svalue::initial_value_of_param_p.
	* svalue.cc (svalue::live_p): Convert "live_svalues" from a
	reference to a pointer; support it being NULL.
	(svalue::implicitly_live_p): Convert first param from a
	refererence to a pointer.
	(region_svalue::implicitly_live_p): Likewise.
	(constant_svalue::implicitly_live_p): Likewise.
	(initial_svalue::implicitly_live_p): Likewise.  Treat the initial
	values of params for the top level frame as still live.
	(initial_svalue::initial_value_of_param_p): New function, taken
	from a test in region_model::compare_initial_and_pointer.
	(unaryop_svalue::implicitly_live_p): Convert first param from a
	refererence to a pointer.
	(binop_svalue::implicitly_live_p): Likewise.
	(sub_svalue::implicitly_live_p): Likewise.
	(unmergeable_svalue::implicitly_live_p): Likewise.
	* svalue.h (svalue::live_p): Likewise.
	(svalue::implicitly_live_p): Likewise.
	(region_svalue::implicitly_live_p): Likewise.
	(constant_svalue::implicitly_live_p): Likewise.
	(initial_svalue::implicitly_live_p): Likewise.
	(initial_svalue::initial_value_of_param_p): New decl.
	(unaryop_svalue::implicitly_live_p): Convert first param from a
	refererence to a pointer.
	(binop_svalue::implicitly_live_p): Likewise.
	(sub_svalue::implicitly_live_p): Likewise.
	(unmergeable_svalue::implicitly_live_p): Likewise.

gcc/testsuite/ChangeLog:
	PR analyzer/98969
	* g++.dg/analyzer/pr99064.C: Convert dg-bogus to dg-warning.
	* gcc.dg/analyzer/pr96841.c: Add -Wno-analyzer-too-complex to
	options.  Remove false leak directive.
	* gcc.dg/analyzer/pr98969.c (test_1): Remove xfail from leak
	false positive.
	(test_3): New.
---
 gcc/analyzer/constraint-manager.cc        |  2 +-
 gcc/analyzer/program-state.cc             |  4 +-
 gcc/analyzer/region-model-reachability.cc |  2 +
 gcc/analyzer/region-model.cc              | 14 +-----
 gcc/analyzer/svalue.cc                    | 52 +++++++++++++++++------
 gcc/analyzer/svalue.h                     | 20 +++++----
 gcc/testsuite/g++.dg/analyzer/pr99064.C   |  2 +-
 gcc/testsuite/gcc.dg/analyzer/pr96841.c   |  4 +-
 gcc/testsuite/gcc.dg/analyzer/pr98969.c   |  9 +++-
 9 files changed, 69 insertions(+), 40 deletions(-)

-- 
2.26.2

Patch

diff --git a/gcc/analyzer/constraint-manager.cc b/gcc/analyzer/constraint-manager.cc
index 23421080fba..4dadd200bee 100644
--- a/gcc/analyzer/constraint-manager.cc
+++ b/gcc/analyzer/constraint-manager.cc
@@ -1643,7 +1643,7 @@  public:
 
   bool should_purge_p (const svalue *sval) const
   {
-    return !sval->live_p (m_live_svalues, m_model);
+    return !sval->live_p (&m_live_svalues, m_model);
   }
 
 private:
diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc
index 60fed56cd65..e427fff59d6 100644
--- a/gcc/analyzer/program-state.cc
+++ b/gcc/analyzer/program-state.cc
@@ -523,7 +523,7 @@  sm_state_map::on_liveness_change (const svalue_set &live_svalues,
        ++iter)
     {
       const svalue *iter_sval = (*iter).first;
-      if (!iter_sval->live_p (live_svalues, model))
+      if (!iter_sval->live_p (&live_svalues, model))
 	{
 	  svals_to_unset.add (iter_sval);
 	  entry_t e = (*iter).second;
@@ -1201,7 +1201,7 @@  program_state::detect_leaks (const program_state &src_state,
 	 live in DEST_STATE: either explicitly reachable, or implicitly
 	 live based on the set of explicitly reachable svalues.
 	 Record those that have ceased to be live.  */
-      if (!sval->live_p (dest_svalues, dest_state.m_region_model))
+      if (!sval->live_p (&dest_svalues, dest_state.m_region_model))
 	dead_svals.quick_push (sval);
     }
 
diff --git a/gcc/analyzer/region-model-reachability.cc b/gcc/analyzer/region-model-reachability.cc
index a988ffc1439..087185b4e45 100644
--- a/gcc/analyzer/region-model-reachability.cc
+++ b/gcc/analyzer/region-model-reachability.cc
@@ -91,6 +91,8 @@  reachable_regions::init_cluster (const region *base_reg)
   if (const symbolic_region *sym_reg = base_reg->dyn_cast_symbolic_region ())
     {
       const svalue *ptr = sym_reg->get_pointer ();
+      if (ptr->implicitly_live_p (NULL, m_model))
+	add (base_reg, true);
       switch (ptr->get_kind ())
 	{
 	default:
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 2fc07c49649..159b0f18654 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -1980,18 +1980,8 @@  region_model::compare_initial_and_pointer (const initial_svalue *init,
   /* If we have a pointer to something within a stack frame, it can't be the
      initial value of a param.  */
   if (pointee->maybe_get_frame_region ())
-    {
-      const region *reg = init->get_region ();
-      if (tree reg_decl = reg->maybe_get_decl ())
-	if (TREE_CODE (reg_decl) == SSA_NAME)
-	  {
-	    tree ssa_name = reg_decl;
-	    if (SSA_NAME_IS_DEFAULT_DEF (ssa_name)
-		&& SSA_NAME_VAR (ssa_name)
-		&& TREE_CODE (SSA_NAME_VAR (ssa_name)) == PARM_DECL)
-	      return tristate::TS_FALSE;
-	  }
-    }
+    if (init->initial_value_of_param_p ())
+      return tristate::TS_FALSE;
 
   return tristate::TS_UNKNOWN;
 }
diff --git a/gcc/analyzer/svalue.cc b/gcc/analyzer/svalue.cc
index 5bbd05e8327..d6305a37b9a 100644
--- a/gcc/analyzer/svalue.cc
+++ b/gcc/analyzer/svalue.cc
@@ -246,15 +246,18 @@  svalue::can_merge_p (const svalue *other,
 }
 
 /* Determine if this svalue is either within LIVE_SVALUES, or is implicitly
-   live with respect to LIVE_SVALUES and MODEL.  */
+   live with respect to LIVE_SVALUES and MODEL.
+   LIVE_SVALUES can be NULL, in which case determine if this svalue is
+   intrinsically live.  */
 
 bool
-svalue::live_p (const svalue_set &live_svalues,
+svalue::live_p (const svalue_set *live_svalues,
 		const region_model *model) const
 {
   /* Determine if SVAL is explicitly live.  */
-  if (const_cast<svalue_set &> (live_svalues).contains (this))
-    return true;
+  if (live_svalues)
+    if (const_cast<svalue_set *> (live_svalues)->contains (this))
+      return true;
 
   /* Otherwise, determine if SVAL is implicitly live due to being made of
      other live svalues.  */
@@ -264,7 +267,7 @@  svalue::live_p (const svalue_set &live_svalues,
 /* Base implementation of svalue::implicitly_live_p.  */
 
 bool
-svalue::implicitly_live_p (const svalue_set &, const region_model *) const
+svalue::implicitly_live_p (const svalue_set *, const region_model *) const
 {
   return false;
 }
@@ -512,7 +515,7 @@  region_svalue::accept (visitor *v) const
 /* Implementation of svalue::implicitly_live_p vfunc for region_svalue.  */
 
 bool
-region_svalue::implicitly_live_p (const svalue_set &,
+region_svalue::implicitly_live_p (const svalue_set *,
 				  const region_model *model) const
 {
   /* Pointers into clusters that have escaped should be treated as live.  */
@@ -609,7 +612,7 @@  constant_svalue::accept (visitor *v) const
    Constants are implicitly live.  */
 
 bool
-constant_svalue::implicitly_live_p (const svalue_set &,
+constant_svalue::implicitly_live_p (const svalue_set *,
 				    const region_model *) const
 {
   return true;
@@ -749,7 +752,7 @@  initial_svalue::accept (visitor *v) const
 /* Implementation of svalue::implicitly_live_p vfunc for initial_svalue.  */
 
 bool
-initial_svalue::implicitly_live_p (const svalue_set &,
+initial_svalue::implicitly_live_p (const svalue_set *,
 				   const region_model *model) const
 {
   /* This svalue may be implicitly live if the region still implicitly
@@ -765,6 +768,31 @@  initial_svalue::implicitly_live_p (const svalue_set &,
 	return true;
     }
 
+  /* Assume that the initial values of params for the top level frame
+     are still live, because (presumably) they're still
+     live in the external caller.  */
+  if (initial_value_of_param_p ())
+    if (const frame_region *frame_reg = m_reg->maybe_get_frame_region ())
+      if (frame_reg->get_calling_frame () == NULL)
+	return true;
+
+  return false;
+}
+
+/* Return true if this is the initial value of a function parameter.  */
+
+bool
+initial_svalue::initial_value_of_param_p () const
+{
+  if (tree reg_decl = m_reg->maybe_get_decl ())
+    if (TREE_CODE (reg_decl) == SSA_NAME)
+      {
+	tree ssa_name = reg_decl;
+	if (SSA_NAME_IS_DEFAULT_DEF (ssa_name)
+	    && SSA_NAME_VAR (ssa_name)
+	    && TREE_CODE (SSA_NAME_VAR (ssa_name)) == PARM_DECL)
+	  return true;
+      }
   return false;
 }
 
@@ -816,7 +844,7 @@  unaryop_svalue::accept (visitor *v) const
 /* Implementation of svalue::implicitly_live_p vfunc for unaryop_svalue.  */
 
 bool
-unaryop_svalue::implicitly_live_p (const svalue_set &live_svalues,
+unaryop_svalue::implicitly_live_p (const svalue_set *live_svalues,
 				   const region_model *model) const
 {
   return get_arg ()->live_p (live_svalues, model);
@@ -862,7 +890,7 @@  binop_svalue::accept (visitor *v) const
 /* Implementation of svalue::implicitly_live_p vfunc for binop_svalue.  */
 
 bool
-binop_svalue::implicitly_live_p (const svalue_set &live_svalues,
+binop_svalue::implicitly_live_p (const svalue_set *live_svalues,
 				 const region_model *model) const
 {
   return (get_arg0 ()->live_p (live_svalues, model)
@@ -919,7 +947,7 @@  sub_svalue::accept (visitor *v) const
 /* Implementation of svalue::implicitly_live_p vfunc for sub_svalue.  */
 
 bool
-sub_svalue::implicitly_live_p (const svalue_set &live_svalues,
+sub_svalue::implicitly_live_p (const svalue_set *live_svalues,
 			       const region_model *model) const
 {
   return get_parent ()->live_p (live_svalues, model);
@@ -1136,7 +1164,7 @@  unmergeable_svalue::accept (visitor *v) const
 /* Implementation of svalue::implicitly_live_p vfunc for unmergeable_svalue.  */
 
 bool
-unmergeable_svalue::implicitly_live_p (const svalue_set &live_svalues,
+unmergeable_svalue::implicitly_live_p (const svalue_set *live_svalues,
 				       const region_model *model) const
 {
   return get_arg ()->live_p (live_svalues, model);
diff --git a/gcc/analyzer/svalue.h b/gcc/analyzer/svalue.h
index 0703cac8bb3..672a89cccff 100644
--- a/gcc/analyzer/svalue.h
+++ b/gcc/analyzer/svalue.h
@@ -128,9 +128,9 @@  public:
 
   virtual void accept (visitor *v) const  = 0;
 
-  bool live_p (const svalue_set &live_svalues,
+  bool live_p (const svalue_set *live_svalues,
 	       const region_model *model) const;
-  virtual bool implicitly_live_p (const svalue_set &live_svalues,
+  virtual bool implicitly_live_p (const svalue_set *live_svalues,
 				  const region_model *model) const;
 
   static int cmp_ptr (const svalue *, const svalue *);
@@ -194,7 +194,7 @@  public:
 
   void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE;
   void accept (visitor *v) const FINAL OVERRIDE;
-  bool implicitly_live_p (const svalue_set &,
+  bool implicitly_live_p (const svalue_set *,
 			  const region_model *) const FINAL OVERRIDE;
 
   const region * get_pointee () const { return m_reg; }
@@ -243,7 +243,7 @@  public:
 
   void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE;
   void accept (visitor *v) const FINAL OVERRIDE;
-  bool implicitly_live_p (const svalue_set &,
+  bool implicitly_live_p (const svalue_set *,
 			  const region_model *) const FINAL OVERRIDE;
 
   tree get_constant () const { return m_cst_expr; }
@@ -493,9 +493,11 @@  public:
 
   void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE;
   void accept (visitor *v) const FINAL OVERRIDE;
-  bool implicitly_live_p (const svalue_set &,
+  bool implicitly_live_p (const svalue_set *,
 			  const region_model *) const FINAL OVERRIDE;
 
+  bool initial_value_of_param_p () const;
+
   const region *get_region () const { return m_reg; }
 
  private:
@@ -564,7 +566,7 @@  public:
 
   void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE;
   void accept (visitor *v) const FINAL OVERRIDE;
-  bool implicitly_live_p (const svalue_set &,
+  bool implicitly_live_p (const svalue_set *,
 			  const region_model *) const FINAL OVERRIDE;
 
   enum tree_code get_op () const { return m_op; }
@@ -653,7 +655,7 @@  public:
 
   void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE;
   void accept (visitor *v) const FINAL OVERRIDE;
-  bool implicitly_live_p (const svalue_set &,
+  bool implicitly_live_p (const svalue_set *,
 			  const region_model *) const FINAL OVERRIDE;
 
   enum tree_code get_op () const { return m_op; }
@@ -734,7 +736,7 @@  public:
 
   void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE;
   void accept (visitor *v) const FINAL OVERRIDE;
-  bool implicitly_live_p (const svalue_set &,
+  bool implicitly_live_p (const svalue_set *,
 			  const region_model *) const FINAL OVERRIDE;
 
   const svalue *get_parent () const { return m_parent_svalue; }
@@ -788,7 +790,7 @@  public:
 
   void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE;
   void accept (visitor *v) const FINAL OVERRIDE;
-  bool implicitly_live_p (const svalue_set &,
+  bool implicitly_live_p (const svalue_set *,
 			  const region_model *) const FINAL OVERRIDE;
 
   const svalue *get_arg () const { return m_arg; }
diff --git a/gcc/testsuite/g++.dg/analyzer/pr99064.C b/gcc/testsuite/g++.dg/analyzer/pr99064.C
index a002219bf58..9fa54f57814 100644
--- a/gcc/testsuite/g++.dg/analyzer/pr99064.C
+++ b/gcc/testsuite/g++.dg/analyzer/pr99064.C
@@ -34,6 +34,6 @@  struct TPkcs11Token {
 void list_tokens() {
   for (__normal_iterator base = list_tokens_token_list.begin();;) {
     int *add_info = new int;
-    (*base).add_info = add_info; // { dg-bogus "leak" "PR analyzer/98969" { xfail *-*-* } }
+    (*base).add_info = add_info; // { dg-warning "leak" }
   }
 }
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr96841.c b/gcc/testsuite/gcc.dg/analyzer/pr96841.c
index d9d35f3dce8..85466617054 100644
--- a/gcc/testsuite/gcc.dg/analyzer/pr96841.c
+++ b/gcc/testsuite/gcc.dg/analyzer/pr96841.c
@@ -1,4 +1,4 @@ 
-/* { dg-additional-options "-O1 -Wno-builtin-declaration-mismatch" } */
+/* { dg-additional-options "-Wno-analyzer-too-complex -O1 -Wno-builtin-declaration-mismatch" } */
 
 int
 l8 (void);
@@ -18,6 +18,6 @@  bv (__SIZE_TYPE__ ny)
     {
       *mf = 0;
       (*mf)[ny] = (int *) malloc (sizeof (int));
-      th ((*mf)[ny]); /* { dg-warning "leak" } */
+      th ((*mf)[ny]);
     }
 }
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr98969.c b/gcc/testsuite/gcc.dg/analyzer/pr98969.c
index 8298f263898..7e1587d7094 100644
--- a/gcc/testsuite/gcc.dg/analyzer/pr98969.c
+++ b/gcc/testsuite/gcc.dg/analyzer/pr98969.c
@@ -8,7 +8,7 @@  test_1 (long int i)
 {
   struct foo *f = (struct foo *)i;
   f->expr = __builtin_malloc (1024);
-} /* { dg-bogus "leak" "PR analyzer/98969" { xfail *-*-* } } */
+} /* { dg-bogus "leak" } */
 
 void
 test_2 (long int i)
@@ -16,3 +16,10 @@  test_2 (long int i)
   __builtin_free (((struct foo *)i)->expr);
   __builtin_free (((struct foo *)i)->expr); /* { dg-warning "double-'free' of '\\*\\(\\(struct foo \\*\\)i\\)\\.expr'" } */
 }
+
+void
+test_3 (void *p)
+{
+  void **q = (void **)p;
+  *q = __builtin_malloc (1024);  
+}