[gcc r12-9358] analyzer: fix feasibility false +ve on jumps through function ptrs [PR107582]

[gcc r12-9358] analyzer: fix feasibility false +ve on jumps through function ptrs [PR107582]

[David Malcolm]

https://gcc.gnu.org/g:e7f7483d50069fede8450091449714d122c58fca

commit r12-9358-ge7f7483d50069fede8450091449714d122c58fca
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Wed Mar 29 14:16:47 2023 -0400

    analyzer: fix feasibility false +ve on jumps through function ptrs [PR107582]
    
    PR analyzer/107582 reports a false +ve from
    -Wanalyzer-use-of-uninitialized-value where
    the analyzer's feasibility checker erroneously decides
    that point (B) in the code below is reachable, with "x" being
    uninitialized there:
    
        pthread_cleanup_push(func, NULL);
    
        while (ret != ETIMEDOUT)
            ret = rand() % 1000;
    
        /* (A): after the while loop  */
    
        if (ret != ETIMEDOUT)
          x = &z;
    
        pthread_cleanup_pop(1);
    
        if (ret == ETIMEDOUT)
          return 0;
    
        /* (B): after not bailing out  */
    
    due to these contradictionary conditions somehow both holding:
      * (ret == ETIMEDOUT), at (A) (skipping the initialization of x), and
      * (ret != ETIMEDOUT), at (B)
    
    The root cause is that after the while loop, state merger puts ret in
    the exploded graph in an UNKNOWN state, and saves the diagnostic at (B).
    
    Later, as we explore the feasibilty of reaching the enode for (B),
    dynamic_call_info_t::update_model is called to push/pop the
    frames for handling the call to "func" in pthread_cleanup_pop.
    The "ret" at these nodes in the feasible_graph has a conjured_svalue for
    "ret", and a constraint on it being either == *or* != ETIMEDOUT.
    
    However dynamic_call_info_t::update_model blithely clobbers the
    model with a copy from the exploded_graph, in which "ret" is UNKNOWN.
    
    This patch fixes dynamic_call_info_t::update_model so that it
    simulates pushing/popping a frame on the model we're working with,
    preserving knowledge of the constraint on "ret", and enabling the
    analyzer to "know" that the bail-out must happen.
    
    Doing so fixes the false positive.
    
    Cherrypicked from r13-4158-ga7aef0a5a2b7e2.
    
    gcc/analyzer/ChangeLog:
            PR analyzer/107582
            * engine.cc (dynamic_call_info_t::update_model): Update the model
            by pushing or pop a frame, rather than by clobbering it with the
            model from the exploded_node's state.
    
    gcc/testsuite/ChangeLog:
            PR analyzer/107582
            * gcc.dg/analyzer/feasibility-4.c: New test.
            * gcc.dg/analyzer/feasibility-pr107582-1.c: New test.
            * gcc.dg/analyzer/feasibility-pr107582-2.c: New test.
    
    Signed-off-by: David Malcolm <dmalcolm@redhat.com>

Diff:
---
 gcc/analyzer/engine.cc                             | 14 +++++--
 gcc/testsuite/gcc.dg/analyzer/feasibility-4.c      | 42 +++++++++++++++++++++
 .../gcc.dg/analyzer/feasibility-pr107582-1.c       | 43 ++++++++++++++++++++++
 .../gcc.dg/analyzer/feasibility-pr107582-2.c       | 34 +++++++++++++++++
 4 files changed, 129 insertions(+), 4 deletions(-)

diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
index a7818c46d95..9a116d94527 100644
--- a/gcc/analyzer/engine.cc
+++ b/gcc/analyzer/engine.cc
@@ -1803,16 +1803,22 @@ exploded_node::dump_succs_and_preds (FILE *outf) const
 /* Implementation of custom_edge_info::update_model vfunc
    for dynamic_call_info_t.
 
-   Update state for the dynamically discorverd calls */
+   Update state for a dynamically discovered call (or return), by pushing
+   or popping the a frame for the appropriate function.  */
 
 bool
 dynamic_call_info_t::update_model (region_model *model,
 				   const exploded_edge *eedge,
-				   region_model_context *) const
+				   region_model_context *ctxt) const
 {
   gcc_assert (eedge);
-  const program_state &dest_state = eedge->m_dest->get_state ();
-  *model = *dest_state.m_region_model;
+  if (m_is_returning_call)
+    model->update_for_return_gcall (m_dynamic_call, ctxt);
+  else
+    {
+      function *callee = eedge->m_dest->get_function ();
+      model->update_for_gcall (m_dynamic_call, ctxt, callee);
+    }
   return true;
 }
 
diff --git a/gcc/testsuite/gcc.dg/analyzer/feasibility-4.c b/gcc/testsuite/gcc.dg/analyzer/feasibility-4.c
new file mode 100644
index 00000000000..1a1128089fb
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/feasibility-4.c
@@ -0,0 +1,42 @@
+#include "analyzer-decls.h"
+
+extern int rand (void);
+
+void test_1 (void)
+{
+  int   ret = 0;
+  while (ret != 42)
+    ret = rand() % 1000;
+
+  if (ret != 42)
+    __analyzer_dump_path (); /* { dg-bogus "path" } */
+}
+
+static void empty_local_fn (void) {}
+extern void external_fn (void);
+
+void test_2 (void)
+{
+  void (*callback) () = empty_local_fn;
+  int   ret = 0;
+  while (ret != 42)
+    ret = rand() % 1000;
+
+  (*callback) ();
+
+  if (ret != 42)
+    __analyzer_dump_path (); /* { dg-bogus "path" } */
+}
+
+void test_3 (void)
+{
+  void (*callback) () = external_fn;
+  int   ret = 0;
+  while (ret != 42)
+    ret = rand() % 1000;
+
+  (*callback) ();
+
+  if (ret != 42)
+    __analyzer_dump_path (); /* { dg-bogus "path" } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/feasibility-pr107582-1.c b/gcc/testsuite/gcc.dg/analyzer/feasibility-pr107582-1.c
new file mode 100644
index 00000000000..15799d02845
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/feasibility-pr107582-1.c
@@ -0,0 +1,43 @@
+/* { dg-require-effective-target pthread }  */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <pthread.h>
+#include <errno.h>
+#include <unistd.h>
+
+#include "analyzer-decls.h"
+
+int z;
+
+static void func(void * o)
+{
+  (void) o;
+}
+
+int main(int    argc,
+         int ** argv)
+{
+  struct timespec now;
+
+  int * x;
+  int   ret = 0;
+
+  pthread_cleanup_push(func, NULL);
+
+  while (ret != ETIMEDOUT)
+    ret = rand() % 1000;
+
+  if (ret != ETIMEDOUT)
+    x = &z;
+
+  pthread_cleanup_pop(1);
+
+  if (ret == ETIMEDOUT)
+    return 0;
+
+  __analyzer_dump_path (); /* { dg-bogus "path" } */
+  printf("x = %d\n", *x); /* { dg-bogus "use of uninitialized value 'x'" } */
+
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/feasibility-pr107582-2.c b/gcc/testsuite/gcc.dg/analyzer/feasibility-pr107582-2.c
new file mode 100644
index 00000000000..b86ffd86d5c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/feasibility-pr107582-2.c
@@ -0,0 +1,34 @@
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "analyzer-decls.h"
+
+int z;
+
+static void func(void)
+{
+}
+
+int main(int    argc,
+         int ** argv)
+{
+  int * x;
+  int   ret = 0;
+  void (*callback) () = func;
+
+  while (ret != 110)
+    ret = rand() % 1000;
+
+  if (ret != 110)
+    x = &z;
+
+  (*callback) ();
+
+  if (ret == 110)
+    return 0;
+
+  __analyzer_dump_path (); /* { dg-bogus "path" } */
+  printf("x = %d\n", *x); /* { dg-bogus "use of uninitialized value 'x'" } */
+
+  return 0;
+}