From: David Malcolm Date: Fri, 14 Feb 2020 02:17:11 +0000 (-0500) Subject: analyzer: fix ICEs in region_model::get_lvalue_1 [PR 93388] X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=f76a88ebf089871dcce215aa0cb1956ccc060895;p=gcc.git analyzer: fix ICEs in region_model::get_lvalue_1 [PR 93388] There have been various ICEs with -fanalyzer involving unhandled tree codes in region_model::get_lvalue_1; PR analyzer/93388 reports various others e.g. for IMAGPART_EXPR, REALPART_EXPR, and VIEW_CONVERT_EXPR seen when running the testsuite with -fanalyzer forcibly enabled. Whilst we could implement lvalue-handling in the region model for every tree code, for some of these we're straying far from my primary goal for GCC 10 of implementing a double-free checker for C. This patch implements a fallback for unimplemented tree codes: create a dummy region, but mark the new state as being invalid, and stop exploring state along this path. It also implements VIEW_CONVERT_EXPR. Doing so fixes the ICEs, whilst effectively turning off the analyzer along code paths that use such tree codes. Hopefully this compromise is sensible for GCC 10. gcc/analyzer/ChangeLog: PR analyzer/93388 * engine.cc (impl_region_model_context::on_unknown_tree_code): New. (exploded_graph::get_or_create_node): Reject invalid states. * exploded-graph.h (impl_region_model_context::on_unknown_tree_code): New decl. (point_and_state::point_and_state): Assert that the state is valid. * program-state.cc (program_state::program_state): Initialize m_valid to true. (program_state::operator=): Copy m_valid. (program_state::program_state): Likewise for move constructor. (program_state::print): Print m_valid. (program_state::dump_to_pp): Likewise. * program-state.h (program_state::m_valid): New field. * region-model.cc (region_model::get_lvalue_1): Implement the default case by returning a new symbolic region and calling the context's on_unknown_tree_code, rather than issuing an internal_error. Implement VIEW_CONVERT_EXPR. * region-model.h (region_model_context::on_unknown_tree_code): New vfunc. (test_region_model_context::on_unknown_tree_code): New. gcc/testsuite/ChangeLog: PR analyzer/93388 * gcc.dg/analyzer/torture/20060625-1.c: New test. * gcc.dg/analyzer/torture/pr51628-30.c: New test. * gcc.dg/analyzer/torture/pr59037.c: New test. --- diff --git a/gcc/analyzer/ChangeLog b/gcc/analyzer/ChangeLog index 5945abc04ee..d669c989ac1 100644 --- a/gcc/analyzer/ChangeLog +++ b/gcc/analyzer/ChangeLog @@ -1,3 +1,28 @@ +2020-02-17 David Malcolm + + PR analyzer/93388 + * engine.cc (impl_region_model_context::on_unknown_tree_code): + New. + (exploded_graph::get_or_create_node): Reject invalid states. + * exploded-graph.h + (impl_region_model_context::on_unknown_tree_code): New decl. + (point_and_state::point_and_state): Assert that the state is + valid. + * program-state.cc (program_state::program_state): Initialize + m_valid to true. + (program_state::operator=): Copy m_valid. + (program_state::program_state): Likewise for move constructor. + (program_state::print): Print m_valid. + (program_state::dump_to_pp): Likewise. + * program-state.h (program_state::m_valid): New field. + * region-model.cc (region_model::get_lvalue_1): Implement the + default case by returning a new symbolic region and calling + the context's on_unknown_tree_code, rather than issuing an + internal_error. Implement VIEW_CONVERT_EXPR. + * region-model.h (region_model_context::on_unknown_tree_code): New + vfunc. + (test_region_model_context::on_unknown_tree_code): New. + 2020-02-17 David Malcolm * sm-malloc.cc (malloc_diagnostic::describe_state_change): For diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc index 17507c7c08e..cd4ffe55dc5 100644 --- a/gcc/analyzer/engine.cc +++ b/gcc/analyzer/engine.cc @@ -684,6 +684,25 @@ impl_region_model_context::on_phi (const gphi *phi, tree rhs) } } +/* Implementation of region_model_context::on_unknown_tree_code vfunc. + Mark the new state as being invalid for further exploration. + TODO(stage1): introduce a warning for when this occurs. */ + +void +impl_region_model_context::on_unknown_tree_code (path_var pv, + const dump_location_t &loc) +{ + logger * const logger = get_logger (); + if (logger) + logger->log ("unhandled tree code: %qs in %qs at %s:%i", + get_tree_code_name (TREE_CODE (pv.m_tree)), + loc.get_impl_location ().m_function, + loc.get_impl_location ().m_file, + loc.get_impl_location ().m_line); + if (m_new_state) + m_new_state->m_valid = false; +} + /* struct point_and_state. */ /* Assert that this object is sane. */ @@ -1845,6 +1864,15 @@ exploded_graph::get_or_create_node (const program_point &point, logger->end_log_line (); } + /* Stop exploring paths for which we don't know how to effectively + model the state. */ + if (!state.m_valid) + { + if (logger) + logger->log ("invalid state; not creating node"); + return NULL; + } + auto_cfun sentinel (point.get_function ()); state.validate (get_ext_state ()); diff --git a/gcc/analyzer/exploded-graph.h b/gcc/analyzer/exploded-graph.h index bb1f3ccff1a..614c37ce9af 100644 --- a/gcc/analyzer/exploded-graph.h +++ b/gcc/analyzer/exploded-graph.h @@ -76,6 +76,9 @@ class impl_region_model_context : public region_model_context void on_phi (const gphi *phi, tree rhs) FINAL OVERRIDE; + void on_unknown_tree_code (path_var pv, + const dump_location_t &loc) FINAL OVERRIDE; + exploded_graph *m_eg; log_user m_logger; const exploded_node *m_enode_for_diag; @@ -100,6 +103,9 @@ public: m_state (state), m_hash (m_point.hash () ^ m_state.hash ()) { + /* We shouldn't be building point_and_states and thus exploded_nodes + for states that aren't valid. */ + gcc_assert (state.m_valid); } hashval_t hash () const diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc index 82b921eb969..fb96e3c976b 100644 --- a/gcc/analyzer/program-state.cc +++ b/gcc/analyzer/program-state.cc @@ -573,7 +573,8 @@ sm_state_map::validate (const state_machine &sm, program_state::program_state (const extrinsic_state &ext_state) : m_region_model (new region_model ()), - m_checker_states (ext_state.get_num_checkers ()) + m_checker_states (ext_state.get_num_checkers ()), + m_valid (true) { int num_states = ext_state.get_num_checkers (); for (int i = 0; i < num_states; i++) @@ -584,7 +585,8 @@ program_state::program_state (const extrinsic_state &ext_state) program_state::program_state (const program_state &other) : m_region_model (new region_model (*other.m_region_model)), - m_checker_states (other.m_checker_states.length ()) + m_checker_states (other.m_checker_states.length ()), + m_valid (true) { int i; sm_state_map *smap; @@ -610,6 +612,8 @@ program_state::operator= (const program_state &other) FOR_EACH_VEC_ELT (other.m_checker_states, i, smap) m_checker_states.quick_push (smap->clone ()); + m_valid = other.m_valid; + return *this; } @@ -626,6 +630,8 @@ program_state::program_state (program_state &&other) FOR_EACH_VEC_ELT (other.m_checker_states, i, smap) m_checker_states.quick_push (smap); other.m_checker_states.truncate (0); + + m_valid = other.m_valid; } #endif @@ -693,6 +699,11 @@ program_state::print (const extrinsic_state &ext_state, pp_newline (pp); } } + if (!m_valid) + { + pp_printf (pp, "invalid state"); + pp_newline (pp); + } } /* Dump a multiline representation of this state to PP. */ @@ -716,6 +727,12 @@ program_state::dump_to_pp (const extrinsic_state &ext_state, pp_newline (pp); } } + + if (!m_valid) + { + pp_printf (pp, "invalid state"); + pp_newline (pp); + } } /* Dump a multiline representation of this state to OUTF. */ diff --git a/gcc/analyzer/program-state.h b/gcc/analyzer/program-state.h index a4608c7498d..80df649f565 100644 --- a/gcc/analyzer/program-state.h +++ b/gcc/analyzer/program-state.h @@ -286,6 +286,11 @@ public: /* TODO: lose the pointer here (const-correctness issues?). */ region_model *m_region_model; auto_delete_vec m_checker_states; + + /* If false, then don't attempt to explore further states along this path. + For use in "handling" lvalues for tree codes we haven't yet + implemented. */ + bool m_valid; }; /* An abstract base class for use with for_each_state_change. */ diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index ae810f5eb4b..b67660cf864 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -4641,9 +4641,19 @@ region_model::get_lvalue_1 (path_var pv, region_model_context *ctxt) switch (TREE_CODE (expr)) { default: - internal_error ("unhandled tree code in region_model::get_lvalue_1: %qs", - get_tree_code_name (TREE_CODE (expr))); - gcc_unreachable (); + { + /* If we see a tree code we we don't know how to handle, rather than + ICE or generate bogus results, create a dummy region, and notify + CTXT so that it can mark the new state as being not properly + modelled. The exploded graph can then stop exploring that path, + since any diagnostics we might issue will have questionable + validity. */ + region_id new_rid + = add_region (new symbolic_region (m_root_rid, NULL_TREE, false)); + ctxt->on_unknown_tree_code (pv, dump_location_t ()); + return new_rid; + } + break; case ARRAY_REF: { @@ -4750,6 +4760,13 @@ region_model::get_lvalue_1 (path_var pv, region_model_context *ctxt) return cst_rid; } break; + + case VIEW_CONVERT_EXPR: + { + tree obj = TREE_OPERAND (expr, 0); + return get_or_create_view (get_lvalue (obj, ctxt), TREE_TYPE (expr)); + }; + break; } } diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h index 9c9a936fae2..dc56d64a43b 100644 --- a/gcc/analyzer/region-model.h +++ b/gcc/analyzer/region-model.h @@ -1937,6 +1937,11 @@ class region_model_context /* Hooks for clients to be notified when a phi node is handled, where RHS is the pertinent argument. */ virtual void on_phi (const gphi *phi, tree rhs) = 0; + + /* Hooks for clients to be notified when the region model doesn't + know how to handle the tree code of PV at LOC. */ + virtual void on_unknown_tree_code (path_var pv, + const dump_location_t &loc) = 0; }; /* A bundle of data for use when attempting to merge two region_model @@ -2118,6 +2123,13 @@ public: { } + void on_unknown_tree_code (path_var pv, const dump_location_t &) + FINAL OVERRIDE + { + internal_error ("unhandled tree code: %qs", + get_tree_code_name (TREE_CODE (pv.m_tree))); + } + private: /* Implicitly delete any diagnostics in the dtor. */ auto_delete_vec m_diagnostics; diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index a08ad2e7175..6c34e0bbe36 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,10 @@ +2020-02-17 David Malcolm + + PR analyzer/93388 + * gcc.dg/analyzer/torture/20060625-1.c: New test. + * gcc.dg/analyzer/torture/pr51628-30.c: New test. + * gcc.dg/analyzer/torture/pr59037.c: New test. + 2020-02-17 David Malcolm * gcc.dg/analyzer/malloc-1.c (test_48): New. diff --git a/gcc/testsuite/gcc.dg/analyzer/torture/20060625-1.c b/gcc/testsuite/gcc.dg/analyzer/torture/20060625-1.c new file mode 100644 index 00000000000..2657a925a7a --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/torture/20060625-1.c @@ -0,0 +1 @@ +#include "../../../gcc.c-torture/compile/20060625-1.c" diff --git a/gcc/testsuite/gcc.dg/analyzer/torture/pr51628-30.c b/gcc/testsuite/gcc.dg/analyzer/torture/pr51628-30.c new file mode 100644 index 00000000000..4513e0f890c --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/torture/pr51628-30.c @@ -0,0 +1,3 @@ +/* { dg-additional-options "-Wno-address-of-packed-member" } */ + +#include "../../../c-c++-common/pr51628-30.c" diff --git a/gcc/testsuite/gcc.dg/analyzer/torture/pr59037.c b/gcc/testsuite/gcc.dg/analyzer/torture/pr59037.c new file mode 100644 index 00000000000..d01aaec8eea --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/torture/pr59037.c @@ -0,0 +1 @@ +#include "../../../c-c++-common/pr59037.c"