analyzer: stop exploring the path after certain diagnostics [PR108830]

PR analyzer/108830 reports a situation in which there are lots of
followup -Wanalyzer-null-dereference warnings after the first access of
a NULL pointer, leading to very noisy output from -fanalyzer.

The analyzer's logic for stopping emitting multiple warnings from a
state machine doesn't quite work for NULL pointers: it attempts to
transition the malloc state machine's NULL pointer to the "stop" state,
which doesn't seem to make much sense in retrospect, and seems to get
confused over types.

Similarly, poisoned_value_diagnostic can be very noisy for uninit
variables, emitting a warning for every access to an uninitialized
variable.  In theory, region_model::check_for_poison makes some attempts
to suppress followups, but only for the symbolic value itself; if the
user's code keeps accessing the same region, we would get a warning on
each one.  For example, this showed up in Doom's s_sound.c where there
were 7 followup uninit warnings after the first uninit warning in
"S_ChangeMusic".

This patch adds an extra mechanism, giving pending diagnostics the
option of stopping the analysis of an execution path if they're saved
for emission on it, and turning this on for these warnings:
  -Wanalyzer-null-dereference
  -Wanalyzer-null-argument
  -Wanalyzer-use-after-free
  -Wanalyzer-use-of-pointer-in-stale-stack-frame
  -Wanalyzer-use-of-uninitialized-value

Doing so should hopefully reduce the cascades of diagnostics that
-fanalyzer can sometimes emit.

I added a -fno-analyzer-suppress-followups for the cases where you
really want the followup warnings (e.g. in some DejaGnu tests, and
for microbenchmarks of UB detection, such as PR analyzer/104224).

Integration testing shows this patch reduces the number of probable
false positives reported by 94, and finds one more true positive:

Comparison: 9.34% -> 10.91%
  GOOD:  66 ->  67  (+1)
   BAD: 641 -> 547 (-94)

where the affected warnings/projects are:

  -Wanalyzer-null-dereference: 0.00% GOOD: 0 BAD: 269 -> 239 (-30)
     Unclassified: 257 -> 228 (-29)
                  apr-1.7.0:  12 ->   5  (-7)
                       doom:   1 ->   0  (-1)
              haproxy-2.7.1:  47 ->  41  (-6)
       ImageMagick-7.1.0-57:  13 ->   9  (-4)
                 qemu-7.2.0: 165 -> 154 (-11)

      Known false: 7 -> 6 (-1)
                   xz-5.4.0:   4 ->   3  (-1)

  -Wanalyzer-use-of-uninitialized-value: 0.00% GOOD: 0 BAD: 143 -> 80 (-63)
      Known false: 47 -> 16 (-31)
                       doom: 42 -> 11 (-31)

     Unclassified: 96 -> 64 (-32)
              coreutils-9.1: 14 -> 10  (-4)
              haproxy-2.7.1: 29 -> 23  (-6)
                 qemu-7.2.0: 48 -> 26 (-22)

  -Wanalyzer-null-argument: 0.00% -> 2.33% GOOD: 0 -> 1 (+1) BAD: 43 -> 42 (-1)
     Unclassified: 39 -> 38 (-1)
      due to coreutils-9.1: 9 -> 8 (-1)

    True positive: 0 -> 1 (+1)
      (in haproxy-2.7.1)

gcc/analyzer/ChangeLog:
	PR analyzer/108830
	* analyzer.opt (fanalyzer-suppress-followups): New option.
	* engine.cc (impl_region_model_context::warn): Terminate the path
	if the diagnostic's terminate_path_p vfunc returns true and
	-fanalyzer-suppress-followups is true (the default).
	(impl_sm_context::warn): Likewise, for both overloads.
	* pending-diagnostic.h (pending_diagnostic::terminate_path_p): New
	vfunc.
	* program-state.cc (program_state::on_edge): Terminate the path if
	the ctxt requests it during updating the edge.
	* region-model.cc (poisoned_value_diagnostic::terminate_path_p):
	New vfunc.
	* sm-malloc.cc (null_deref::terminate_path_p): New vfunc.
	(null_arg::terminate_path_p): New vfunc.

gcc/ChangeLog:
	PR analyzer/108830
	* doc/invoke.texi: Document -fno-analyzer-suppress-followups.

gcc/testsuite/ChangeLog:
	PR analyzer/108830
	* gcc.dg/analyzer/attribute-nonnull.c: Update for
	-Wanalyzer-use-of-uninitialized-value terminating analysis along
	a path.
	* gcc.dg/analyzer/call-summaries-2.c: Likewise.
	* gcc.dg/analyzer/data-model-1.c: Likewise.
	* gcc.dg/analyzer/data-model-5.c: Likewise.
	* gcc.dg/analyzer/doom-s_sound-pr108867.c: New test.
	* gcc.dg/analyzer/memset-CVE-2017-18549-1.c: Add
	-fno-analyzer-suppress-followups.
	* gcc.dg/analyzer/null-deref-pr108830.c: New test.
	* gcc.dg/analyzer/pipe-1.c: Add -fno-analyzer-suppress-followups.
	* gcc.dg/analyzer/pipe-void-return.c: Likewise.
	* gcc.dg/analyzer/pipe2-1.c: Likewise.
	* gcc.dg/analyzer/pr101547.c: Update for
	-Wanalyzer-use-of-uninitialized-value terminating analysis along
	a path.
	* gcc.dg/analyzer/pr101875.c: Likewise.
	* gcc.dg/analyzer/pr104224-split.c: New test, based on...
	* gcc.dg/analyzer/pr104224.c: Add
	-fno-analyzer-suppress-followups.
	* gcc.dg/analyzer/realloc-2.c: Add
	-fno-analyzer-suppress-followups.
	* gcc.dg/analyzer/realloc-3.c: Likewise.
	* gcc.dg/analyzer/realloc-5.c: Likewise.
	* gcc.dg/analyzer/stdarg-1-ms_abi.c: Likewise.
	* gcc.dg/analyzer/stdarg-1-sysv_abi.c: Likewise.
	* gcc.dg/analyzer/stdarg-1.c: Likewise.
	* gcc.dg/analyzer/symbolic-1.c: Likewise.
	* gcc.dg/analyzer/symbolic-7.c: Update for
	-Wanalyzer-use-of-uninitialized-value terminating analysis along a
	path.
	* gcc.dg/analyzer/uninit-4.c: Likewise.
	* gcc.dg/analyzer/uninit-8.c: New test.
	* gcc.dg/analyzer/uninit-pr94713.c: Update for
	-Wanalyzer-use-of-uninitialized-value terminating analysis along a
	path.
	* gcc.dg/analyzer/zlib-6a.c: Add -fno-analyzer-suppress-followups.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>

6 files changed, 64 insertions, 7 deletions

diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt
index b4dcdb8..9d1a937 100644
--- a/gcc/analyzer/analyzer.opt
+++ b/gcc/analyzer/analyzer.opt
@@ -262,6 +262,10 @@ fanalyzer-state-merge
 Common Var(flag_analyzer_state_merge) Init(1)
 Merge similar-enough states during analysis.
 
+fanalyzer-suppress-followups
+Common Var(flag_analyzer_suppress_followups) Init(1)
+Stop exploring an execution path after certain diagnostics.
+
 fanalyzer-transitivity
 Common Var(flag_analyzer_transitivity) Init(0)
 Enable transitivity of constraints during analysis.
diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
index 24ded26..a5965c2 100644
--- a/gcc/analyzer/engine.cc
+++ b/gcc/analyzer/engine.cc
@@ -125,11 +125,20 @@ impl_region_model_context::warn (std::unique_ptr<pending_diagnostic> d)
       return false;
     }
   if (m_eg)
-    return m_eg->get_diagnostic_manager ().add_diagnostic
-      (m_enode_for_diag, m_enode_for_diag->get_supernode (),
-       m_stmt, m_stmt_finder, std::move (d));
-  else
-    return false;
+    {
+      bool terminate_path = d->terminate_path_p ();
+      if (m_eg->get_diagnostic_manager ().add_diagnostic
+	  (m_enode_for_diag, m_enode_for_diag->get_supernode (),
+	   m_stmt, m_stmt_finder, std::move (d)))
+	{
+	  if (m_path_ctxt
+	      && terminate_path
+	      && flag_analyzer_suppress_followups)
+	    m_path_ctxt->terminate_path ();
+	  return true;
+	}
+    }
+  return false;
 }
 
 void
@@ -378,9 +387,14 @@ public:
       = (var
 	 ? m_old_smap->get_state (var_old_sval, m_eg.get_ext_state ())
 	 : m_old_smap->get_global_state ());
+    bool terminate_path = d->terminate_path_p ();
     m_eg.get_diagnostic_manager ().add_diagnostic
       (&m_sm, m_enode_for_diag, snode, stmt, m_stmt_finder,
        var, var_old_sval, current, std::move (d));
+    if (m_path_ctxt
+	&& terminate_path
+	&& flag_analyzer_suppress_followups)
+      m_path_ctxt->terminate_path ();
   }
 
   void warn (const supernode *snode, const gimple *stmt,
@@ -393,9 +407,14 @@ public:
       = (sval
 	 ? m_old_smap->get_state (sval, m_eg.get_ext_state ())
 	 : m_old_smap->get_global_state ());
+    bool terminate_path = d->terminate_path_p ();
     m_eg.get_diagnostic_manager ().add_diagnostic
       (&m_sm, m_enode_for_diag, snode, stmt, m_stmt_finder,
        NULL_TREE, sval, current, std::move (d));
+    if (m_path_ctxt
+	&& terminate_path
+	&& flag_analyzer_suppress_followups)
+      m_path_ctxt->terminate_path ();
   }
 
   /* Hook for picking more readable trees for SSA names of temporaries,
diff --git a/gcc/analyzer/pending-diagnostic.h b/gcc/analyzer/pending-diagnostic.h
index d9e9e7f..6423c8b 100644
--- a/gcc/analyzer/pending-diagnostic.h
+++ b/gcc/analyzer/pending-diagnostic.h
@@ -173,6 +173,10 @@ class pending_diagnostic
      having to generate feasible execution paths for them).  */
   virtual int get_controlling_option () const = 0;
 
+  /* Vfunc to give the diagnostic the chance to terminate the execution
+     path being explored.  By default, don't terminate the path.  */
+  virtual bool terminate_path_p () const { return false; }
+
   /* Vfunc for emitting the diagnostic.  The rich_location will have been
      populated with a diagnostic_path.
      Return true if a diagnostic is actually emitted.  */
diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc
index 9a1a8cd..8dade4b 100644
--- a/gcc/analyzer/program-state.cc
+++ b/gcc/analyzer/program-state.cc
@@ -1105,6 +1105,27 @@ program_state::on_edge (exploded_graph &eg,
 			const superedge *succ,
 			uncertainty_t *uncertainty)
 {
+  class my_path_context : public path_context
+  {
+  public:
+    my_path_context (bool &terminated) : m_terminated (terminated) {}
+    void bifurcate (std::unique_ptr<custom_edge_info>) final override
+    {
+      gcc_unreachable ();
+    }
+
+    void terminate_path () final override
+    {
+      m_terminated = true;
+    }
+
+    bool terminate_path_p () const final override
+    {
+      return m_terminated;
+    }
+    bool &m_terminated;
+  };
+
   /* Update state.  */
   const program_point &point = enode->get_point ();
   const gimple *last_stmt = point.get_supernode ()->get_last_stmt ();
@@ -1117,11 +1138,12 @@ program_state::on_edge (exploded_graph &eg,
      Adding the relevant conditions for the edge could also trigger
      sm-state transitions (e.g. transitions due to ptrs becoming known
      to be NULL or non-NULL) */
-
+  bool terminated = false;
+  my_path_context path_ctxt (terminated);
   impl_region_model_context ctxt (eg, enode,
 				  &enode->get_state (),
 				  this,
-				  uncertainty, NULL,
+				  uncertainty, &path_ctxt,
 				  last_stmt);
   if (!m_region_model->maybe_update_for_edge (*succ,
 					      last_stmt,
@@ -1134,6 +1156,8 @@ program_state::on_edge (exploded_graph &eg,
 		     succ->m_dest->m_index);
       return false;
     }
+  if (terminated)
+    return false;
 
   program_state::detect_leaks (enode->get_state (), *this,
 			       NULL, eg.get_ext_state (),
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index e3de74b..f844b51 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -505,6 +505,8 @@ public:
       }
   }
 
+  bool terminate_path_p () const final override { return true; }
+
   bool emit (rich_location *rich_loc) final override
   {
     switch (m_pkind)
diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc
index c24fe73..1ea9b30 100644
--- a/gcc/analyzer/sm-malloc.cc
+++ b/gcc/analyzer/sm-malloc.cc
@@ -1150,6 +1150,8 @@ public:
     return OPT_Wanalyzer_null_dereference;
   }
 
+  bool terminate_path_p () const final override { return true; }
+
   bool emit (rich_location *rich_loc) final override
   {
     /* CWE-476: NULL Pointer Dereference.  */
@@ -1203,6 +1205,8 @@ public:
     return OPT_Wanalyzer_null_argument;
   }
 
+  bool terminate_path_p () const final override { return true; }
+
   bool emit (rich_location *rich_loc) final override
   {
     /* CWE-476: NULL Pointer Dereference.  */