analyzer: use dominator info in -Wanalyzer-deref-before-check [PR108455]

My integration testing [1] of -fanalyzer in GCC 13 is showing a lot of
diagnostics from the new -Wanalyzer-deref-before-check warning on
real-world C projects, and most of these seem to be false positives.

This patch updates the warning to make it much less likely to fire:
- only intraprocedural cases are now reported
- reject cases in which there are control flow paths to the check
  that didn't come through the dereference, by looking at BB dominator
  information.  This fixes a false positive seen in git-2.39.0's
  pack-revindex.c: load_revindex_from_disk (PR analyzer/108455), in
  which a shared "cleanup:" section checks "data" for NULL, and
  depending on how much of the function is executed "data" might or
  might not have already been dereferenced.

The counts of -Wanalyzer-deref-before-check diagnostics in [1]
before/after this patch show this improvement:
  Known false positives:    6 ->  0  (-6)
  Known true positives:     1 ->  1
  Unclassified positives: 123 -> 63 (-60)

[1] https://github.com/davidmalcolm/gcc-analyzer-integration-tests

gcc/analyzer/ChangeLog:
	PR analyzer/108455
	* analyzer.h (class checker_event): New forward decl.
	(class state_change_event): Indent.
	(class warning_event): New forward decl.
	* checker-event.cc (state_change_event::state_change_event): Add
	"enode" param.
	(warning_event::get_desc): Update for new param of
	evdesc::final_event ctor.
	* checker-event.h (state_change_event::state_change_event): Add
	"enode" param.
	(state_change_event::get_exploded_node): New accessor.
	(state_change_event::m_enode): New field.
	(warning_event::warning_event): New "enode" param.
	(warning_event::get_exploded_node): New accessor.
	(warning_event::m_enode): New field.
	* diagnostic-manager.cc
	(state_change_event_creator::on_global_state_change): Pass
	src_node to state_change_event ctor.
	(state_change_event_creator::on_state_change): Likewise.
	(null_assignment_sm_context::set_next_state): Pass NULL for
	new param of state_change_event ctor.
	* infinite-recursion.cc
	(infinite_recursion_diagnostic::add_final_event): Update for new
	param of warning_event ctor.
	* pending-diagnostic.cc (pending_diagnostic::add_final_event):
	Pass enode to warning_event ctor.
	* pending-diagnostic.h (evdesc::final_event): Add reference to
	warning_event.
	* sm-malloc.cc: Include "analyzer/checker-event.h" and
	"analyzer/exploded-graph.h".
	(deref_before_check::deref_before_check): Initialize new fields.
	(deref_before_check::emit): Reject warnings in which we were
	unable to determine the enodes of the dereference and the check.
	Reject warnings interprocedural warnings. Reject warnings in which
	the dereference doesn't dominate the check.
	(deref_before_check::describe_state_change): Set m_deref_enode.
	(deref_before_check::describe_final_event): Set m_check_enode.
	(deref_before_check::m_deref_enode): New field.
	(deref_before_check::m_check_enode): New field.

gcc/testsuite/ChangeLog:
	PR analyzer/108455
	* gcc.dg/analyzer/deref-before-check-1.c: Add test coverage
	involving dominance.
	* gcc.dg/analyzer/deref-before-check-pr108455-1.c: New test.
	* gcc.dg/analyzer/deref-before-check-pr108455-git-pack-revindex.c:
	New test.

Signed-off-by: David Malcolm <[email protected]>

Author: davidmalcolm

gcc/analyzer/analyzer.h

@@ -93,7 +93,9 @@ class bounded_ranges_manager;
 class pending_diagnostic;
 class pending_note;
 struct event_loc_info;
-class state_change_event;
+class checker_event;
+  class state_change_event;
+  class warning_event;
 class checker_path;
 class extrinsic_state;
 class sm_state_map;

gcc/analyzer/checker-event.cc

@@ -410,15 +410,17 @@ state_change_event::state_change_event (const supernode *node,
 					state_machine::state_t from,
 					state_machine::state_t to,
 					const svalue *origin,
-					const program_state &dst_state)
+					const program_state &dst_state,
+					const exploded_node *enode)
 : checker_event (EK_STATE_CHANGE,
 		 event_loc_info (stmt->location,
 				 node->m_fun->decl,
 				 stack_depth)),
   m_node (node), m_stmt (stmt), m_sm (sm),
   m_sval (sval), m_from (from), m_to (to),
   m_origin (origin),
-  m_dst_state (dst_state)
+  m_dst_state (dst_state),
+  m_enode (enode)
 {
 }
 
@@ -1159,7 +1161,7 @@ warning_event::get_desc (bool can_colorize) const
       tree var = fixup_tree_for_diagnostic (m_var);
       label_text ev_desc
 	= m_pending_diagnostic->describe_final_event
-	    (evdesc::final_event (can_colorize, var, m_state));
+	    (evdesc::final_event (can_colorize, var, m_state, *this));
       if (ev_desc.get ())
 	{
 	  if (m_sm && flag_analyzer_verbose_state_changes)

gcc/analyzer/checker-event.h

@@ -357,7 +357,8 @@ class state_change_event : public checker_event
 		      state_machine::state_t from,
 		      state_machine::state_t to,
 		      const svalue *origin,
-		      const program_state &dst_state);
+		      const program_state &dst_state,
+		      const exploded_node *enode);
 
   label_text get_desc (bool can_colorize) const final override;
   meaning get_meaning () const override;
@@ -367,6 +368,8 @@ class state_change_event : public checker_event
     return m_dst_state.get_current_function ();
   }
 
+  const exploded_node *get_exploded_node () const { return m_enode; }
+
   const supernode *m_node;
   const gimple *m_stmt;
   const state_machine &m_sm;
@@ -375,6 +378,7 @@ class state_change_event : public checker_event
   state_machine::state_t m_to;
   const svalue *m_origin;
   program_state m_dst_state;
+  const exploded_node *m_enode;
 };
 
 /* Subclass of checker_event; parent class for subclasses that relate to
@@ -668,17 +672,22 @@ class warning_event : public checker_event
 {
 public:
   warning_event (const event_loc_info &loc_info,
+		 const exploded_node *enode,
 		 const state_machine *sm,
 		 tree var, state_machine::state_t state)
   : checker_event (EK_WARNING, loc_info),
+    m_enode (enode),
     m_sm (sm), m_var (var), m_state (state)
   {
   }
 
   label_text get_desc (bool can_colorize) const final override;
   meaning get_meaning () const override;
 
+  const exploded_node *get_exploded_node () const { return m_enode; }
+
 private:
+  const exploded_node *m_enode;
   const state_machine *m_sm;
   tree m_var;
   state_machine::state_t m_state;

gcc/analyzer/diagnostic-manager.cc

@@ -1572,7 +1572,8 @@ class state_change_event_creator : public state_change_visitor
 					src_sm_val,
 					dst_sm_val,
 					NULL,
-					dst_state));
+					dst_state,
+					src_node));
     return false;
   }
 
@@ -1616,7 +1617,8 @@ class state_change_event_creator : public state_change_visitor
 					src_sm_val,
 					dst_sm_val,
 					dst_origin_sval,
-					dst_state));
+					dst_state,
+					src_node));
     return false;
   }
 
@@ -1760,7 +1762,8 @@ struct null_assignment_sm_context : public sm_context
 					var_new_sval,
 					from, to,
 					NULL,
-					*m_new_state));
+					*m_new_state,
+					NULL));
   }
 
   void set_next_state (const gimple *stmt,
@@ -1785,7 +1788,8 @@ struct null_assignment_sm_context : public sm_context
 					sval,
 					from, to,
 					NULL,
-					*m_new_state));
+					*m_new_state,
+					NULL));
   }
 
   void warn (const supernode *, const gimple *,

gcc/analyzer/infinite-recursion.cc

@@ -182,7 +182,7 @@ class infinite_recursion_diagnostic
   /* Customize the location where the warning_event appears, putting
      it at the topmost entrypoint to the function.  */
   void add_final_event (const state_machine *,
-			const exploded_node *,
+			const exploded_node *enode,
 			const gimple *,
 			tree,
 			state_machine::state_t,
@@ -195,6 +195,7 @@ class infinite_recursion_diagnostic
 			  ()->get_start_location (),
 			m_callee_fndecl,
 			m_new_entry_enode->get_stack_depth ()),
+	enode,
 	NULL, NULL, NULL));
   }
 

gcc/analyzer/pending-diagnostic.cc

@@ -232,6 +232,7 @@ pending_diagnostic::add_final_event (const state_machine *sm,
      (event_loc_info (get_stmt_location (stmt, enode->get_function ()),
 		      enode->get_function ()->decl,
 		      enode->get_stack_depth ()),
+      enode,
       sm, var, state));
 }
 

gcc/analyzer/pending-diagnostic.h

@@ -131,13 +131,15 @@ struct return_of_state : public event_desc
 struct final_event : public event_desc
 {
   final_event (bool colorize,
-	       tree expr, state_machine::state_t state)
+	       tree expr, state_machine::state_t state,
+	       const warning_event &event)
   : event_desc (colorize),
-    m_expr (expr), m_state (state)
+    m_expr (expr), m_state (state), m_event (event)
   {}
 
   tree m_expr;
   state_machine::state_t m_state;
+  const warning_event &m_event;
 };
 
 } /* end of namespace evdesc */

gcc/analyzer/sm-malloc.cc

@@ -45,6 +45,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "attribs.h"
 #include "analyzer/function-set.h"
 #include "analyzer/program-state.h"
+#include "analyzer/checker-event.h"
+#include "analyzer/exploded-graph.h"
 
 #if ENABLE_ANALYZER
 
@@ -1490,7 +1492,9 @@ class deref_before_check : public malloc_diagnostic
 {
 public:
   deref_before_check (const malloc_state_machine &sm, tree arg)
-  : malloc_diagnostic (sm, arg)
+  : malloc_diagnostic (sm, arg),
+    m_deref_enode (NULL),
+    m_check_enode (NULL)
   {}
 
   const char *get_kind () const final override { return "deref_before_check"; }
@@ -1502,6 +1506,31 @@ class deref_before_check : public malloc_diagnostic
 
   bool emit (rich_location *rich_loc) final override
   {
+    /* Don't emit the warning if we can't show where the deref
+       and the check occur.  */
+    if (!m_deref_enode)
+      return false;
+    if (!m_check_enode)
+      return false;
+    /* Only emit the warning for intraprocedural cases.  */
+    if (m_deref_enode->get_function () != m_check_enode->get_function ())
+      return false;
+    if (&m_deref_enode->get_point ().get_call_string ()
+	!= &m_check_enode->get_point ().get_call_string ())
+      return false;
+
+    /* Reject the warning if the deref's BB doesn't dominate that
+       of the check, so that we don't warn e.g. for shared cleanup
+       code that checks a pointer for NULL, when that code is sometimes
+       used before a deref and sometimes after.
+       Using the dominance code requires setting cfun.  */
+    auto_cfun sentinel (m_deref_enode->get_function ());
+    calculate_dominance_info (CDI_DOMINATORS);
+    if (!dominated_by_p (CDI_DOMINATORS,
+			 m_check_enode->get_supernode ()->m_bb,
+			 m_deref_enode->get_supernode ()->m_bb))
+      return false;
+
     if (m_arg)
       return warning_at (rich_loc, get_controlling_option (),
 			 "check of %qE for NULL after already"
@@ -1520,6 +1549,7 @@ class deref_before_check : public malloc_diagnostic
 	&& assumed_non_null_p (change.m_new_state))
       {
 	m_first_deref_event = change.m_event_id;
+	m_deref_enode = change.m_event.get_exploded_node ();
 	if (m_arg)
 	  return change.formatted_print ("pointer %qE is dereferenced here",
 					 m_arg);
@@ -1531,6 +1561,7 @@ class deref_before_check : public malloc_diagnostic
 
   label_text describe_final_event (const evdesc::final_event &ev) final override
   {
+    m_check_enode = ev.m_event.get_exploded_node ();
     if (m_first_deref_event.known_p ())
       {
 	if (m_arg)
@@ -1556,6 +1587,8 @@ class deref_before_check : public malloc_diagnostic
 
 private:
   diagnostic_event_id_t m_first_deref_event;
+  const exploded_node *m_deref_enode;
+  const exploded_node *m_check_enode;
 };
 
 /* struct allocation_state : public state_machine::state.  */

gcc/testsuite/gcc.dg/analyzer/deref-before-check-1.c

@@ -167,3 +167,39 @@ int test_checking_ptr_after_calling_deref (int *q)
     return 0;
   return v;
 }
+
+extern void foo ();
+extern void bar ();
+extern void baz ();
+
+int test_cfg_diamond_1 (int *p, int flag)
+{
+  int x;
+  x = *p; /* { dg-message "pointer 'p' is dereferenced here" } */
+  if (flag)
+    foo ();
+  else
+    bar ();
+  if (p) /* { dg-warning "check of 'p' for NULL after already dereferencing it" } */
+    {
+      baz ();
+    }
+  return x;
+}
+
+int test_cfg_diamond_2 (int *p, int flag)
+{
+  int x = 0;
+  if (flag)
+    foo ();
+  else
+    {
+      x = *p;
+      bar ();
+    }
+  if (p) /* { dg-bogus "check of 'p' for NULL after already dereferencing it" } */
+    {
+      baz ();
+    }
+  return x;
+}

gcc/testsuite/gcc.dg/analyzer/deref-before-check-pr108455-1.c

@@ -0,0 +1,36 @@
+extern int could_fail_1 (void);
+extern void *could_fail_2 (int);
+extern void cleanup (void *);
+
+struct header {
+  int signature;
+};
+
+int test_1 (void) {
+  int fd, ret = 0;
+  void *data = ((void *)0);
+  struct header *hdr;
+
+  fd = could_fail_1 ();
+
+  if (fd < 0) {
+    ret = -1;
+    goto cleanup;
+  }
+
+  data = could_fail_2 (fd);
+  hdr = data;
+
+  if (hdr->signature != 42) {
+    ret = -2;
+    goto cleanup;
+  }
+
+cleanup:
+  if (ret) {
+    if (data) /* { dg-bogus "check of 'data' for NULL after already dereferencing it" } */
+      cleanup (data);
+  }
+
+  return ret;
+}