analyzer: directly explore within static functions [PR93355,PR96374]
authorDavid Malcolm <dmalcolm@redhat.com>
Tue, 2 Feb 2021 02:54:11 +0000 (21:54 -0500)
committerDavid Malcolm <dmalcolm@redhat.com>
Tue, 2 Feb 2021 02:54:11 +0000 (21:54 -0500)
PR analyzer/93355 tracks that -fanalyzer fails to report the FILE *
leak in read_alias_file in intl/localealias.c.

One reason for the failure is that read_alias_file is marked as
"static", and the path leading to the single call of
read_alias_file is falsely rejected as infeasible due to
PR analyzer/96374.  I have been attempting to fix that bug, but
don't have a good solution yet.

Previously, -fanalyzer only directly explored "static" functions
if they were needed for call summaries, instead forcing them to
be indirectly explored, but if we have a feasibility bug like
above, we will fail to report any issues in a function that's
only called by such a falsely infeasible path.

It now seems wrong to me to reject directly exploring static
functions: even if there is currently no way to call a function,
it seems reasonable to warn about bugs within them, since
otherwise these latent bugs are a timebomb in the code.

Hence this patch reworks toplevel_function_p to directly explore
almost all functions, working around these feasiblity issues.
It introduces a naming convention that "__analyzer_"-prefixed
function names don't get directly explored, since this is
useful in the analyzer's DejaGnu-based tests.

This workaround gets PR analyzer/93355 closer to working, but
unfortunately there is a second instance of PR analyzer/96374
within read_alias_file itself which means even with this patch
-fanalyzer falsely rejects the path as infeasible.

Still, this ought to help in other cases, and simplifies the
implementation.

gcc/analyzer/ChangeLog:
PR analyzer/93355
PR analyzer/96374
* engine.cc (toplevel_function_p): Simplify so that
we only reject functions with a "__analyzer_" prefix.
(add_any_callbacks): Delete.
(exploded_graph::build_initial_worklist): Update for
dropped param of toplevel_function_p.
(exploded_graph::build_initial_worklist): Don't bother
looking for callbacks that are reachable from global
initializers.

gcc/testsuite/ChangeLog:
PR analyzer/93355
PR analyzer/96374
* gcc.dg/analyzer/conditionals-3.c: Add "__analyzer_"
prefix to support subroutines where necessary.
* gcc.dg/analyzer/data-model-1.c: Likewise.
* gcc.dg/analyzer/feasibility-1.c (called_by_test_6a): New.
(test_6a): New.
* gcc.dg/analyzer/params.c: Add "__analyzer_" prefix to support
subroutines where necessary.
* gcc.dg/analyzer/pr96651-2.c: Likewise.
* gcc.dg/analyzer/signal-4b.c: Likewise.
* gcc.dg/analyzer/single-field.c: Likewise.
* gcc.dg/analyzer/torture/conditionals-2.c: Likewise.

gcc/analyzer/engine.cc
gcc/testsuite/gcc.dg/analyzer/conditionals-3.c
gcc/testsuite/gcc.dg/analyzer/data-model-1.c
gcc/testsuite/gcc.dg/analyzer/feasibility-1.c
gcc/testsuite/gcc.dg/analyzer/params.c
gcc/testsuite/gcc.dg/analyzer/pr96651-2.c
gcc/testsuite/gcc.dg/analyzer/signal-4b.c
gcc/testsuite/gcc.dg/analyzer/single-field.c
gcc/testsuite/gcc.dg/analyzer/torture/conditionals-2.c

index fc81e7523fbaf427f0eb1cd028eaf06129c2c5c6..45aed8f0d377f381cbaf8a6f97d98713d78449db 100644 (file)
@@ -2348,38 +2348,27 @@ exploded_graph::get_per_function_data (function *fun) const
   return NULL;
 }
 
-/* Return true if NODE and FUN should be traversed directly, rather than
+/* Return true if FUN should be traversed directly, rather than only as
    called via other functions.  */
 
 static bool
-toplevel_function_p (cgraph_node *node, function *fun, logger *logger)
+toplevel_function_p (function *fun, logger *logger)
 {
-  /* TODO: better logic here
-     e.g. only if more than one caller, and significantly complicated.
-     Perhaps some whole-callgraph analysis to decide if it's worth summarizing
-     an edge, and if so, we need summaries.  */
-  if (flag_analyzer_call_summaries)
-    {
-      int num_call_sites = 0;
-      for (cgraph_edge *edge = node->callers; edge; edge = edge->next_caller)
-       ++num_call_sites;
-
-      /* For now, if there's more than one in-edge, and we want call
-        summaries, do it at the top level so that there's a chance
-        we'll have a summary when we need one.  */
-      if (num_call_sites > 1)
-       {
-         if (logger)
-           logger->log ("traversing %qE (%i call sites)",
-                        fun->decl, num_call_sites);
-         return true;
-       }
-    }
-
-  if (!TREE_PUBLIC (fun->decl))
+  /* Don't directly traverse into functions that have an "__analyzer_"
+     prefix.  Doing so is useful for the analyzer test suite, allowing
+     us to have functions that are called in traversals, but not directly
+     explored, thus testing how the analyzer handles calls and returns.
+     With this, we can have DejaGnu directives that cover just the case
+     of where a function is called by another function, without generating
+     excess messages from the case of the first function being traversed
+     directly.  */
+#define ANALYZER_PREFIX "__analyzer_"
+  if (!strncmp (IDENTIFIER_POINTER (DECL_NAME (fun->decl)), ANALYZER_PREFIX,
+               strlen (ANALYZER_PREFIX)))
     {
       if (logger)
-       logger->log ("not traversing %qE (static)", fun->decl);
+       logger->log ("not traversing %qE (starts with %qs)",
+                    fun->decl, ANALYZER_PREFIX);
       return false;
     }
 
@@ -2389,18 +2378,6 @@ toplevel_function_p (cgraph_node *node, function *fun, logger *logger)
   return true;
 }
 
-/* Callback for walk_tree for finding callbacks within initializers;
-   ensure they are treated as possible entrypoints to the analysis.  */
-
-static tree
-add_any_callbacks (tree *tp, int *, void *data)
-{
-  exploded_graph *eg = (exploded_graph *)data;
-  if (TREE_CODE (*tp) == FUNCTION_DECL)
-    eg->on_escaped_function (*tp);
-  return NULL_TREE;
-}
-
 /* Add initial nodes to EG, with entrypoints for externally-callable
    functions.  */
 
@@ -2414,7 +2391,7 @@ exploded_graph::build_initial_worklist ()
   FOR_EACH_FUNCTION_WITH_GIMPLE_BODY (node)
   {
     function *fun = node->get_fun ();
-    if (!toplevel_function_p (node, fun, logger))
+    if (!toplevel_function_p (fun, logger))
       continue;
     exploded_node *enode = add_function_entry (fun);
     if (logger)
@@ -2426,19 +2403,6 @@ exploded_graph::build_initial_worklist ()
          logger->log ("did not create enode for %qE entrypoint", fun->decl);
       }
   }
-
-  /* Find callbacks that are reachable from global initializers.  */
-  varpool_node *vpnode;
-  FOR_EACH_VARIABLE (vpnode)
-    {
-      tree decl = vpnode->decl;
-      if (!TREE_PUBLIC (decl))
-       continue;
-      tree init = DECL_INITIAL (decl);
-      if (!init)
-       continue;
-      walk_tree (&init, add_any_callbacks, this, NULL);
-    }
 }
 
 /* The main loop of the analysis.
index 5f29f215dd17b39083fd58b4b2535778266f1476..f1c6c2084056eb859d7b142e8ca1036aaee59405 100644 (file)
@@ -2,12 +2,12 @@
 
 #include "analyzer-decls.h"
 
-static void only_called_when_flag_a_true (int i)
+static void __analyzer_only_called_when_flag_a_true (int i)
 {
   __analyzer_eval (i == 42); /* { dg-warning "TRUE" } */
 }
 
-static void only_called_when_flag_b_true (int i)
+static void __analyzer_only_called_when_flag_b_true (int i)
 {
   __analyzer_eval (i == 17); /* { dg-warning "TRUE" } */
 }
@@ -34,7 +34,7 @@ int test_1 (int flag_a, int flag_b)
       __analyzer_eval (flag_b); /* { dg-warning "UNKNOWN" } */
       __analyzer_eval (i == 42); /* { dg-warning "TRUE" } */
       __analyzer_eval (i == 17); /* { dg-warning "FALSE" } */
-      only_called_when_flag_a_true (i);
+      __analyzer_only_called_when_flag_a_true (i);
     }  
   else
     {
@@ -42,6 +42,6 @@ int test_1 (int flag_a, int flag_b)
       __analyzer_eval (flag_b); /* { dg-warning "UNKNOWN" } */
       __analyzer_eval (i == 42); /* { dg-warning "FALSE" } */
       __analyzer_eval (i == 17); /* { dg-warning "TRUE" } */
-      only_called_when_flag_b_true (i);
+      __analyzer_only_called_when_flag_b_true (i);
     }
 }
index f6681b678af6181319481af33d31796ba158865e..afd155694600477bd883e5473fe12603d46bef8f 100644 (file)
@@ -782,7 +782,7 @@ void test_33 (void)
 }
 
 static int __attribute__((noinline))
-only_called_by_test_34 (int parm)
+__analyzer_only_called_by_test_34 (int parm)
 {
   __analyzer_eval (parm == 42); /* { dg-warning "TRUE" } */
 
@@ -791,7 +791,7 @@ only_called_by_test_34 (int parm)
 
 void test_34 (void)
 {
-  int result = only_called_by_test_34 (42);
+  int result = __analyzer_only_called_by_test_34 (42);
   __analyzer_eval (result == 84); /* { dg-warning "TRUE" } */
 }
 
index f2a8a4cb0be355cfb927fbcba343b5d92f8524f0..c96844467a5183e2b71ee4ca506711c0b4170de5 100644 (file)
@@ -60,3 +60,29 @@ int test_6 (int a, int b)
     }
   return problem;
 }
+
+/* As above, but call a static function.
+   Even if the path to the call of called_by_test_6a is falsely rejected
+   as infeasible, it still makes sense to complain about errors within
+   the called function.  */
+
+static void __attribute__((noinline))
+called_by_test_6a (void *ptr)
+{
+  __builtin_free (ptr);
+  __builtin_free (ptr); /* { dg-message "double-'free'" } */
+}
+
+int test_6a (int a, int b, void *ptr)
+{
+  int problem = 0;
+  if (a)
+    problem = 1;
+  if (b)
+    {
+      if (!problem)
+       problem = 2;
+      called_by_test_6a (ptr);
+    }
+  return problem;
+}
index f8331ddc094b57f55df75758765b32d05a33d0bc..12bba70d6e4d21b4e1be9d8e059993b2736e5bc4 100644 (file)
@@ -1,6 +1,6 @@
 #include "analyzer-decls.h"
 
-static int called_function(int j)
+static int __analyzer_called_function(int j)
 {
   int k;
 
@@ -23,7 +23,7 @@ void test(int i)
 
     __analyzer_eval (i > 4); /* { dg-warning "TRUE" } */
 
-    i = called_function(i);
+    i = __analyzer_called_function(i);
 
     __analyzer_eval (i > 3); /* { dg-warning "TRUE" "desired" { xfail *-*-* } } */
     /* { dg-warning "UNKNOWN" "status quo" { target *-*-* } .-1 } */
index 249a32b8ed9e90fc73a626f77928f23d6da70455..25cda37934e18085ca23fa243cfe7f4f4db7650c 100644 (file)
@@ -26,7 +26,7 @@ void test (void)
 }
 
 static void __attribute__((noinline))
-called_from_main (void)
+__analyzer_called_from_main (void)
 {
   /* When accessed from main, the vars still have their initializer values.  */
   __analyzer_eval (a == 0); /* { dg-warning "TRUE" } */
@@ -53,7 +53,7 @@ int main (void)
      before "main").  */
   __analyzer_eval (stderr == 0); /* { dg-warning "UNKNOWN" } */
 
-  called_from_main ();
+  __analyzer_called_from_main ();
 
   unknown_fn (&a, &c);
 
index cb1e7e475ae37d97c847dba026fad299c357d980..5a2ccb1def48cd7f4cb3f5532a4f2081d1f24689 100644 (file)
@@ -20,14 +20,14 @@ static void int_handler(int signum)
   custom_logger("got signal");
 }
 
-static void register_handler ()
+static void __analyzer_register_handler ()
 {
   signal(SIGINT, int_handler);
 }
 
 void test (void)
 {
-  register_handler ();
+  __analyzer_register_handler ();
   body_of_program();
 }
 
@@ -42,17 +42,17 @@ void test (void)
     |      |      |
     |      |      (1) entry to 'test'
     |   NN | {
-    |   NN |   register_handler ();
-    |      |   ~~~~~~~~~~~~~~~~~~~
+    |   NN |   __analyzer_register_handler ();
+    |      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     |      |   |
-    |      |   (2) calling 'register_handler' from 'test'
+    |      |   (2) calling '__analyzer_register_handler' from 'test'
     |
-    +--> 'register_handler': events 3-4
+    +--> '__analyzer_register_handler': events 3-4
            |
-           |   NN | static void register_handler ()
-           |      |             ^~~~~~~~~~~~~~~~
+           |   NN | static void __analyzer_register_handler ()
+           |      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~
            |      |             |
-           |      |             (3) entry to 'register_handler'
+           |      |             (3) entry to '__analyzer_register_handler'
            |   NN | {
            |   NN |   signal(SIGINT, int_handler);
            |      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~
index d54cfb0c4d1eccb572414a6ae6e3ef49d6c89c9e..31c6feead8fec78c5eb84e606ffc11906082b05d 100644 (file)
@@ -11,14 +11,14 @@ void test_1 (struct foo f)
   __analyzer_describe (0, f.ptr); /* { dg-warning "svalue: 'INIT_VAL\\(f.ptr\\)'" } */
 }
 
-static void called_by_test_2 (struct foo f_inner)
+static void __analyzer_called_by_test_2 (struct foo f_inner)
 {
   free (f_inner.ptr);
   free (f_inner.ptr); /* { dg-warning "double-'free' of 'f_outer.ptr'" } */
 }
 void test_2 (struct foo f_outer)
 {
-  called_by_test_2 (f_outer);
+  __analyzer_called_by_test_2 (f_outer);
 }
 
 struct nested
@@ -26,12 +26,12 @@ struct nested
   struct foo f;
 };
 
-static void called_by_test_3 (struct nested n_inner)
+static void __analyzer_called_by_test_3 (struct nested n_inner)
 {
   free (n_inner.f.ptr);
   free (n_inner.f.ptr); /* { dg-warning "double-'free' of 'n_outer.f.ptr'" } */
 }
 void test_3 (struct nested n_outer)
 {
-  called_by_test_3 (n_outer);
+  __analyzer_called_by_test_3 (n_outer);
 }
index 35b0a05f64942c17becb7ff5b42c7627ccbf73c2..278a2a537117e6ffef3609e13a9b67114d8b812f 100644 (file)
@@ -5,7 +5,7 @@
 #define Z_NULL 0
 
 static void __attribute__((noinline))
-test_1_callee (void *p, void *q)
+__analyzer_test_1_callee (void *p, void *q)
 {
   __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed enode" } */
 
@@ -21,11 +21,11 @@ void test_1 (void *p, void *q)
   if (p == Z_NULL || q == Z_NULL)
     return;
 
-  test_1_callee (p, q);
+  __analyzer_test_1_callee (p, q);
 }
 
 static void __attribute__((noinline))
-test_2_callee (void *p, void *q)
+__analyzer_test_2_callee (void *p, void *q)
 {
   __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed enode" } */
 
@@ -39,5 +39,5 @@ test_2_callee (void *p, void *q)
 void test_2 (void *p, void *q)
 {
   if (p != Z_NULL && q != Z_NULL)
-    test_2_callee (p, q);
+    __analyzer_test_2_callee (p, q);
 }