From 41f99ba6c576b84ca0f2de7d66ebc087454e93cf Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Thu, 5 Mar 2020 12:06:58 -0500 Subject: [PATCH] analyzer: improvements to state dumping This patch fixes a bug in which summarized state dumps involving a non-NULL pointer to a region for which get_representative_path_var returned NULL were erroneously dumped as "NULL". It also extends sm-state dumps so that they show representative tree values, where available. Finally, it adds some selftest coverage for such dumps. Doing so requires replacing some %qE with a dump_quoted_tree, to avoid C vs C++ differences between "make selftest-c" and "make selftest-c++". gcc/analyzer/ChangeLog: * analyzer.h (dump_quoted_tree): New decl. * engine.cc (exploded_node::dump_dot): Pass region model to sm_state_map::print. * program-state.cc: Include diagnostic-core.h. (sm_state_map::print): Add "model" param and use it to print representative trees. Only print origin information if non-null. (sm_state_map::dump): Pass NULL for model to print call. (program_state::print): Pass region model to sm_state_map::print. (program_state::dump_to_pp): Use spaces rather than newlines when summarizing. Pass region_model to sm_state_map::print. (ana::selftest::assert_dump_eq): New function. (ASSERT_DUMP_EQ): New macro. (ana::selftest::test_program_state_dumping): New function. (ana::selftest::analyzer_program_state_cc_tests): Call it. * program-state.h (program_state::print): Add model param. * region-model.cc (dump_quoted_tree): New function. (map_region::print_fields): Use dump_quoted_tree rather than %qE to avoid lang-dependent output. (map_region::dump_child_label): Likewise. (region_model::dump_summary_of_map): For SK_REGION, when get_representative_path_var fails, print the region id rather than erroneously printing NULL. * sm.cc (state_machine::get_state_by_name): New function. * sm.h (state_machine::get_state_by_name): New decl. --- gcc/analyzer/ChangeLog | 27 +++++++ gcc/analyzer/analyzer.h | 8 ++- gcc/analyzer/engine.cc | 2 +- gcc/analyzer/program-state.cc | 128 ++++++++++++++++++++++++++++++---- gcc/analyzer/program-state.h | 3 +- gcc/analyzer/region-model.cc | 29 +++++--- gcc/analyzer/sm.cc | 15 ++++ gcc/analyzer/sm.h | 2 + 8 files changed, 188 insertions(+), 26 deletions(-) diff --git a/gcc/analyzer/ChangeLog b/gcc/analyzer/ChangeLog index 4a95fa60f9a..84c619e40c4 100644 --- a/gcc/analyzer/ChangeLog +++ b/gcc/analyzer/ChangeLog @@ -1,3 +1,30 @@ +2020-03-06 David Malcolm + + * analyzer.h (dump_quoted_tree): New decl. + * engine.cc (exploded_node::dump_dot): Pass region model to + sm_state_map::print. + * program-state.cc: Include diagnostic-core.h. + (sm_state_map::print): Add "model" param and use it to print + representative trees. Only print origin information if non-null. + (sm_state_map::dump): Pass NULL for model to print call. + (program_state::print): Pass region model to sm_state_map::print. + (program_state::dump_to_pp): Use spaces rather than newlines when + summarizing. Pass region_model to sm_state_map::print. + (ana::selftest::assert_dump_eq): New function. + (ASSERT_DUMP_EQ): New macro. + (ana::selftest::test_program_state_dumping): New function. + (ana::selftest::analyzer_program_state_cc_tests): Call it. + * program-state.h (program_state::print): Add model param. + * region-model.cc (dump_quoted_tree): New function. + (map_region::print_fields): Use dump_quoted_tree rather than + %qE to avoid lang-dependent output. + (map_region::dump_child_label): Likewise. + (region_model::dump_summary_of_map): For SK_REGION, when + get_representative_path_var fails, print the region id rather than + erroneously printing NULL. + * sm.cc (state_machine::get_state_by_name): New function. + * sm.h (state_machine::get_state_by_name): New decl. + 2020-03-04 David Malcolm * region-model.cc (region::validate): Convert model param from ptr diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h index 5364edb3d96..78d6009e8a3 100644 --- a/gcc/analyzer/analyzer.h +++ b/gcc/analyzer/analyzer.h @@ -21,12 +21,12 @@ along with GCC; see the file COPYING3. If not see #ifndef GCC_ANALYZER_ANALYZER_H #define GCC_ANALYZER_ANALYZER_H -/* Forward decls of common types, with indentation to show inheritance. */ - class graphviz_out; namespace ana { +/* Forward decls of common types, with indentation to show inheritance. */ + class supergraph; class supernode; class superedge; @@ -71,6 +71,10 @@ class state_purge_per_ssa_name; class state_change; class rewind_info_t; +/* Forward decls of functions. */ + +extern void dump_quoted_tree (pretty_printer *pp, tree t); + } // namespace ana extern bool is_special_named_call_p (const gcall *call, const char *funcname, diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc index e411d5b40e7..2431ae34474 100644 --- a/gcc/analyzer/engine.cc +++ b/gcc/analyzer/engine.cc @@ -869,7 +869,7 @@ exploded_node::dump_dot (graphviz_out *gv, const dump_args_t &args) const if (!smap->is_empty_p ()) { pp_printf (pp, "%s: ", ext_state.get_name (i)); - smap->print (ext_state.get_sm (i), pp); + smap->print (ext_state.get_sm (i), state.m_region_model, pp); pp_newline (pp); } } diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc index 971e8e0a7d6..804800f65fe 100644 --- a/gcc/analyzer/program-state.cc +++ b/gcc/analyzer/program-state.cc @@ -22,6 +22,7 @@ along with GCC; see the file COPYING3. If not see #include "system.h" #include "coretypes.h" #include "tree.h" +#include "diagnostic-core.h" #include "diagnostic.h" #include "function.h" #include "analyzer/analyzer.h" @@ -147,10 +148,13 @@ sm_state_map::clone_with_remapping (const one_way_svalue_id_map &id_map) const return result; } -/* Print this sm_state_map (for SM) to PP. */ +/* Print this sm_state_map (for SM) to PP. + If MODEL is non-NULL, print representative tree values where + available. */ void -sm_state_map::print (const state_machine &sm, pretty_printer *pp) const +sm_state_map::print (const state_machine &sm, const region_model *model, + pretty_printer *pp) const { bool first = true; pp_string (pp, "{"); @@ -170,10 +174,27 @@ sm_state_map::print (const state_machine &sm, pretty_printer *pp) const sid.print (pp); entry_t e = (*iter).second; - pp_printf (pp, ": %s (origin: ", - sm.get_state_name (e.m_state)); - e.m_origin.print (pp); - pp_string (pp, ")"); + pp_printf (pp, ": %s", sm.get_state_name (e.m_state)); + if (model) + if (tree rep = model->get_representative_tree (sid)) + { + pp_string (pp, " ("); + dump_quoted_tree (pp, rep); + pp_character (pp, ')'); + } + if (!e.m_origin.null_p ()) + { + pp_string (pp, " (origin: "); + e.m_origin.print (pp); + if (model) + if (tree rep = model->get_representative_tree (e.m_origin)) + { + pp_string (pp, " ("); + dump_quoted_tree (pp, rep); + pp_character (pp, ')'); + } + pp_string (pp, ")"); + } } pp_string (pp, "}"); } @@ -186,7 +207,7 @@ sm_state_map::dump (const state_machine &sm) const pretty_printer pp; pp_show_color (&pp) = pp_show_color (global_dc->printer); pp.buffer->stream = stderr; - print (sm, &pp); + print (sm, NULL, &pp); pp_newline (&pp); pp_flush (&pp); } @@ -696,7 +717,7 @@ program_state::print (const extrinsic_state &ext_state, if (!smap->is_empty_p ()) { pp_printf (pp, "%s: ", ext_state.get_name (i)); - smap->print (ext_state.get_sm (i), pp); + smap->print (ext_state.get_sm (i), m_region_model, pp); pp_newline (pp); } } @@ -707,7 +728,9 @@ program_state::print (const extrinsic_state &ext_state, } } -/* Dump a multiline representation of this state to PP. */ +/* Dump a representation of this state to PP. + If SUMMARIZE is true, print a one-line summary; + if false, print a detailed multiline representation. */ void program_state::dump_to_pp (const extrinsic_state &ext_state, @@ -723,16 +746,22 @@ program_state::dump_to_pp (const extrinsic_state &ext_state, { if (!smap->is_empty_p ()) { + if (summarize) + pp_space (pp); pp_printf (pp, "%s: ", ext_state.get_name (i)); - smap->print (ext_state.get_sm (i), pp); - pp_newline (pp); + smap->print (ext_state.get_sm (i), m_region_model, pp); + if (!summarize) + pp_newline (pp); } } if (!m_valid) { + if (summarize) + pp_space (pp); pp_printf (pp, "invalid state"); - pp_newline (pp); + if (!summarize) + pp_newline (pp); } } @@ -1231,6 +1260,30 @@ state_change::validate (const program_state &new_state, namespace selftest { +/* Implementation detail of ASSERT_DUMP_EQ. */ + +static void +assert_dump_eq (const location &loc, + const program_state &state, + const extrinsic_state &ext_state, + bool summarize, + const char *expected) +{ + auto_fix_quotes sentinel; + pretty_printer pp; + pp_format_decoder (&pp) = default_tree_printer; + state.dump_to_pp (ext_state, summarize, &pp); + ASSERT_STREQ_AT (loc, pp_formatted_text (&pp), expected); +} + +/* Assert that STATE.dump_to_pp (SUMMARIZE) is EXPECTED. */ + +#define ASSERT_DUMP_EQ(STATE, EXT_STATE, SUMMARIZE, EXPECTED) \ + SELFTEST_BEGIN_STMT \ + assert_dump_eq ((SELFTEST_LOCATION), (STATE), (EXT_STATE), (SUMMARIZE), \ + (EXPECTED)); \ + SELFTEST_END_STMT + /* Tests for sm_state_map. */ static void @@ -1364,6 +1417,56 @@ test_sm_state_map () // TODO: coverage for purging } +/* Verify that program_state::dump_to_pp works as expected. */ + +static void +test_program_state_dumping () +{ + /* Create a program_state for a global ptr "p" that has + malloc sm-state, pointing to a region on the heap. */ + tree p = build_global_decl ("p", ptr_type_node); + + state_machine *sm = make_malloc_state_machine (NULL); + const state_machine::state_t UNCHECKED_STATE + = sm->get_state_by_name ("unchecked"); + auto_delete_vec checkers; + checkers.safe_push (sm); + extrinsic_state ext_state (checkers); + + program_state s (ext_state); + region_model *model = s.m_region_model; + region_id new_rid = model->add_new_malloc_region (); + svalue_id ptr_sid + = model->get_or_create_ptr_svalue (ptr_type_node, new_rid); + model->set_value (model->get_lvalue (p, NULL), + ptr_sid, NULL); + sm_state_map *smap = s.m_checker_states[0]; + + smap->impl_set_state (ptr_sid, UNCHECKED_STATE, svalue_id::null ()); + ASSERT_EQ (smap->get_state (ptr_sid), UNCHECKED_STATE); + + ASSERT_DUMP_EQ + (s, ext_state, false, + "rmodel: r0: {kind: `root', parent: null, sval: null}\n" + "|-heap: r1: {kind: `heap', parent: r0, sval: sv0}\n" + "| |: sval: sv0: {poisoned: uninit}\n" + "| `-r2: {kind: `symbolic', parent: r1, sval: null}\n" + "`-globals: r3: {kind: `globals', parent: r0, sval: null, map: {`p': r4}}\n" + " `-`p': r4: {kind: `primitive', parent: r3, sval: sv1, type: `void *'}\n" + " |: sval: sv1: {type: `void *', &r2}\n" + " |: type: `void *'\n" + "svalues:\n" + " sv0: {poisoned: uninit}\n" + " sv1: {type: `void *', &r2}\n" + "constraint manager:\n" + " equiv classes:\n" + " constraints:\n" + "malloc: {sv1: unchecked (`p')}\n"); + + ASSERT_DUMP_EQ (s, ext_state, true, + "rmodel: p: &r2 malloc: {sv1: unchecked (`p')}"); +} + /* Verify that program_states with identical sm-state can be merged, and that the merged program_state preserves the sm-state. */ @@ -1466,6 +1569,7 @@ void analyzer_program_state_cc_tests () { test_sm_state_map (); + test_program_state_dumping (); test_program_state_merging (); test_program_state_merging_2 (); } diff --git a/gcc/analyzer/program-state.h b/gcc/analyzer/program-state.h index 2c778ccb9ac..3637516ec1b 100644 --- a/gcc/analyzer/program-state.h +++ b/gcc/analyzer/program-state.h @@ -146,7 +146,8 @@ public: sm_state_map * clone_with_remapping (const one_way_svalue_id_map &id_map) const; - void print (const state_machine &sm, pretty_printer *pp) const; + void print (const state_machine &sm, const region_model *model, + pretty_printer *pp) const; void dump (const state_machine &sm) const; bool is_empty_p () const; diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index 0ceeab45a02..e7e517ade15 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -73,6 +73,17 @@ dump_tree (pretty_printer *pp, tree t) dump_generic_node (pp, t, 0, TDF_SLIM, 0); } +/* Dump T to PP in language-independent form in quotes, for + debugging/logging/dumping purposes. */ + +void +dump_quoted_tree (pretty_printer *pp, tree t) +{ + pp_begin_quote (pp, pp_show_color (pp)); + dump_tree (pp, t); + pp_end_quote (pp, pp_show_color (pp)); +} + /* Equivalent to pp_printf (pp, "%qT", t), to avoid nesting pp_printf calls within other pp_printf calls. @@ -1595,7 +1606,8 @@ map_region::print_fields (const region_model &model, pp_string (pp, ", "); tree expr = (*iter).first; region_id child_rid = (*iter).second; - pp_printf (pp, "%qE: ", expr); + dump_quoted_tree (pp, expr); + pp_string (pp, ": "); child_rid.print (pp); } pp_string (pp, "}"); @@ -1665,10 +1677,8 @@ map_region::dump_child_label (const region_model &model, if (child_rid == (*iter).second) { tree key = (*iter).first; - if (DECL_P (key)) - pp_printf (pp, "%qD: ", key); - else - pp_printf (pp, "%qE: ", key); + dump_quoted_tree (pp, key); + pp_string (pp, ": "); } } } @@ -3706,17 +3716,16 @@ region_model::dump_summary_of_map (pretty_printer *pp, { region_svalue *region_sval = as_a (sval); region_id pointee_rid = region_sval->get_pointee (); + gcc_assert (!pointee_rid.null_p ()); tree pointee = get_representative_path_var (pointee_rid).m_tree; dump_separator (pp, is_first); dump_tree (pp, key); pp_string (pp, ": "); + pp_character (pp, '&'); if (pointee) - { - pp_character (pp, '&'); - dump_tree (pp, pointee); - } + dump_tree (pp, pointee); else - pp_string (pp, "NULL"); + pointee_rid.print (pp); } break; case SK_CONSTANT: diff --git a/gcc/analyzer/sm.cc b/gcc/analyzer/sm.cc index b1f156fecc9..affb5aa07db 100644 --- a/gcc/analyzer/sm.cc +++ b/gcc/analyzer/sm.cc @@ -84,6 +84,21 @@ state_machine::get_state_name (state_t s) const return m_state_names[s]; } +/* Get the state with name NAME, which must exist. + This is purely intended for use in selftests. */ + +state_machine::state_t +state_machine::get_state_by_name (const char *name) +{ + unsigned i; + const char *iter_name; + FOR_EACH_VEC_ELT (m_state_names, i, iter_name) + if (!strcmp (name, iter_name)) + return i; + /* Name not found. */ + gcc_unreachable (); +} + /* Assert that S is a valid state for this state_machine. */ void diff --git a/gcc/analyzer/sm.h b/gcc/analyzer/sm.h index 2f00aaec7cc..ebd067a4541 100644 --- a/gcc/analyzer/sm.h +++ b/gcc/analyzer/sm.h @@ -57,6 +57,8 @@ public: const char *get_state_name (state_t s) const; + state_t get_state_by_name (const char *name); + /* Return true if STMT is a function call recognized by this sm. */ virtual bool on_stmt (sm_context *sm_ctxt, const supernode *node, -- 2.30.2