From 8a2750086d57d1a2251d9239fa4e6c2dc9ec3a86 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Mon, 1 Feb 2021 21:54:11 -0500 Subject: [PATCH] analyzer: directly explore within static functions [PR93355,PR96374] 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 | 68 +++++-------------- .../gcc.dg/analyzer/conditionals-3.c | 8 +-- gcc/testsuite/gcc.dg/analyzer/data-model-1.c | 4 +- gcc/testsuite/gcc.dg/analyzer/feasibility-1.c | 26 +++++++ gcc/testsuite/gcc.dg/analyzer/params.c | 4 +- gcc/testsuite/gcc.dg/analyzer/pr96651-2.c | 4 +- gcc/testsuite/gcc.dg/analyzer/signal-4b.c | 18 ++--- gcc/testsuite/gcc.dg/analyzer/single-field.c | 8 +-- .../gcc.dg/analyzer/torture/conditionals-2.c | 8 +-- 9 files changed, 69 insertions(+), 79 deletions(-) diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc index fc81e7523fb..45aed8f0d37 100644 --- a/gcc/analyzer/engine.cc +++ b/gcc/analyzer/engine.cc @@ -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. diff --git a/gcc/testsuite/gcc.dg/analyzer/conditionals-3.c b/gcc/testsuite/gcc.dg/analyzer/conditionals-3.c index 5f29f215dd1..f1c6c208405 100644 --- a/gcc/testsuite/gcc.dg/analyzer/conditionals-3.c +++ b/gcc/testsuite/gcc.dg/analyzer/conditionals-3.c @@ -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); } } diff --git a/gcc/testsuite/gcc.dg/analyzer/data-model-1.c b/gcc/testsuite/gcc.dg/analyzer/data-model-1.c index f6681b678af..afd15569460 100644 --- a/gcc/testsuite/gcc.dg/analyzer/data-model-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/data-model-1.c @@ -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" } */ } diff --git a/gcc/testsuite/gcc.dg/analyzer/feasibility-1.c b/gcc/testsuite/gcc.dg/analyzer/feasibility-1.c index f2a8a4cb0be..c96844467a5 100644 --- a/gcc/testsuite/gcc.dg/analyzer/feasibility-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/feasibility-1.c @@ -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; +} diff --git a/gcc/testsuite/gcc.dg/analyzer/params.c b/gcc/testsuite/gcc.dg/analyzer/params.c index f8331ddc094..12bba70d6e4 100644 --- a/gcc/testsuite/gcc.dg/analyzer/params.c +++ b/gcc/testsuite/gcc.dg/analyzer/params.c @@ -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 } */ diff --git a/gcc/testsuite/gcc.dg/analyzer/pr96651-2.c b/gcc/testsuite/gcc.dg/analyzer/pr96651-2.c index 249a32b8ed9..25cda37934e 100644 --- a/gcc/testsuite/gcc.dg/analyzer/pr96651-2.c +++ b/gcc/testsuite/gcc.dg/analyzer/pr96651-2.c @@ -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); diff --git a/gcc/testsuite/gcc.dg/analyzer/signal-4b.c b/gcc/testsuite/gcc.dg/analyzer/signal-4b.c index cb1e7e475ae..5a2ccb1def4 100644 --- a/gcc/testsuite/gcc.dg/analyzer/signal-4b.c +++ b/gcc/testsuite/gcc.dg/analyzer/signal-4b.c @@ -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); | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/gcc/testsuite/gcc.dg/analyzer/single-field.c b/gcc/testsuite/gcc.dg/analyzer/single-field.c index d54cfb0c4d1..31c6feead8f 100644 --- a/gcc/testsuite/gcc.dg/analyzer/single-field.c +++ b/gcc/testsuite/gcc.dg/analyzer/single-field.c @@ -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); } diff --git a/gcc/testsuite/gcc.dg/analyzer/torture/conditionals-2.c b/gcc/testsuite/gcc.dg/analyzer/torture/conditionals-2.c index 35b0a05f649..278a2a53711 100644 --- a/gcc/testsuite/gcc.dg/analyzer/torture/conditionals-2.c +++ b/gcc/testsuite/gcc.dg/analyzer/torture/conditionals-2.c @@ -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); } -- 2.30.2