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 <dmalcolm@redhat.com>

1 files changed, 34 insertions, 1 deletions

diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc
index 212e9c2..9aee810 100644
--- a/gcc/analyzer/sm-malloc.cc
+++ b/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 @@ public:
 
   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 @@ public:
 	&& 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 @@ public:
 
   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 @@ public:
 
 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.  */