From fd9982bb0051d1a678191b684bb907d1ac177991 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Wed, 22 Jan 2020 11:45:58 -0500 Subject: [PATCH] analyzer: fix setjmp handling with -g (PR 93378) PR analyzer/93378 reports an ICE at -O1 -g when analyzing a rewind via longjmp to a setjmp call with. The root cause is that the rewind_info_t::get_setjmp_call attempts to locate the setjmp GIMPLE_CALL via within the exploded_node containing it, but the exploded_node has two stmts: a GIMPLE_DEBUG, then the GIMPLE_CALL, and so erroneously picks the GIMPLE_DEBUG, leading to a failed as_a . This patch reworks how the analyzer stores information about a setjmp so that instead of storing an exploded_node *, it instead introduces a "setjmp_record" struct, for use by both setjmp_svalue and rewind_info_t. Hence we store the information directly, rather than attempting to reconstruct it, fixing the bug. gcc/analyzer/ChangeLog: PR analyzer/93378 * engine.cc (setjmp_svalue::compare_fields): Update for replacement of m_enode with m_setjmp_record. (setjmp_svalue::add_to_hash): Likewise. (setjmp_svalue::get_index): Rename... (setjmp_svalue::get_enode_index): ...to this. (setjmp_svalue::print_details): Update for replacement of m_enode with m_setjmp_record. (exploded_node::on_longjmp): Likewise. * exploded-graph.h (rewind_info_t::m_enode_origin): Replace... (rewind_info_t::m_setjmp_record): ...with this. (rewind_info_t::rewind_info_t): Update for replacement of m_enode with m_setjmp_record. (rewind_info_t::get_setjmp_point): Likewise. (rewind_info_t::get_setjmp_call): Likewise. * region-model.cc (region_model::dump_summary_of_map): Likewise. (region_model::on_setjmp): Likewise. * region-model.h (struct setjmp_record): New struct. (setjmp_svalue::m_enode): Replace... (setjmp_svalue::m_setjmp_record): ...with this. (setjmp_svalue::setjmp_svalue): Update for replacement of m_enode with m_setjmp_record. (setjmp_svalue::clone): Likewise. (setjmp_svalue::get_index): Rename... (setjmp_svalue::get_enode_index): ...to this. (setjmp_svalue::get_exploded_node): Replace... (setjmp_svalue::get_setjmp_record): ...with this. gcc/testsuite/ChangeLog: PR analyzer/93378 * gcc.dg/analyzer/setjmp-pr93378.c: New test. --- gcc/analyzer/ChangeLog | 30 ++++++++++++++++ gcc/analyzer/engine.cc | 18 +++++----- gcc/analyzer/exploded-graph.h | 15 ++++---- gcc/analyzer/region-model.cc | 7 ++-- gcc/analyzer/region-model.h | 34 +++++++++++++++---- gcc/testsuite/ChangeLog | 5 +++ .../gcc.dg/analyzer/setjmp-pr93378.c | 15 ++++++++ 7 files changed, 100 insertions(+), 24 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/setjmp-pr93378.c diff --git a/gcc/analyzer/ChangeLog b/gcc/analyzer/ChangeLog index d0a652b4a57..26c1184a624 100644 --- a/gcc/analyzer/ChangeLog +++ b/gcc/analyzer/ChangeLog @@ -1,3 +1,33 @@ +2020-01-22 David Malcolm + + PR analyzer/93378 + * engine.cc (setjmp_svalue::compare_fields): Update for + replacement of m_enode with m_setjmp_record. + (setjmp_svalue::add_to_hash): Likewise. + (setjmp_svalue::get_index): Rename... + (setjmp_svalue::get_enode_index): ...to this. + (setjmp_svalue::print_details): Update for replacement of m_enode + with m_setjmp_record. + (exploded_node::on_longjmp): Likewise. + * exploded-graph.h (rewind_info_t::m_enode_origin): Replace... + (rewind_info_t::m_setjmp_record): ...with this. + (rewind_info_t::rewind_info_t): Update for replacement of m_enode + with m_setjmp_record. + (rewind_info_t::get_setjmp_point): Likewise. + (rewind_info_t::get_setjmp_call): Likewise. + * region-model.cc (region_model::dump_summary_of_map): Likewise. + (region_model::on_setjmp): Likewise. + * region-model.h (struct setjmp_record): New struct. + (setjmp_svalue::m_enode): Replace... + (setjmp_svalue::m_setjmp_record): ...with this. + (setjmp_svalue::setjmp_svalue): Update for replacement of m_enode + with m_setjmp_record. + (setjmp_svalue::clone): Likewise. + (setjmp_svalue::get_index): Rename... + (setjmp_svalue::get_enode_index): ...to this. + (setjmp_svalue::get_exploded_node): Replace... + (setjmp_svalue::get_setjmp_record): ...with this. + 2020-01-22 David Malcolm PR analyzer/93316 diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc index 53c93791a07..737ea1dd6e4 100644 --- a/gcc/analyzer/engine.cc +++ b/gcc/analyzer/engine.cc @@ -155,7 +155,7 @@ impl_region_model_context::on_unknown_change (svalue_id sid) bool setjmp_svalue::compare_fields (const setjmp_svalue &other) const { - return m_enode == other.m_enode; + return m_setjmp_record == other.m_setjmp_record; } /* Implementation of svalue::add_to_hash vfunc for setjmp_svalue. */ @@ -163,15 +163,15 @@ setjmp_svalue::compare_fields (const setjmp_svalue &other) const void setjmp_svalue::add_to_hash (inchash::hash &hstate) const { - hstate.add_int (m_enode->m_index); + hstate.add_int (m_setjmp_record.m_enode->m_index); } /* Get the index of the stored exploded_node. */ int -setjmp_svalue::get_index () const +setjmp_svalue::get_enode_index () const { - return m_enode->m_index; + return m_setjmp_record.m_enode->m_index; } /* Implementation of svalue::print_details vfunc for setjmp_svalue. */ @@ -181,7 +181,7 @@ setjmp_svalue::print_details (const region_model &model ATTRIBUTE_UNUSED, svalue_id this_sid ATTRIBUTE_UNUSED, pretty_printer *pp) const { - pp_printf (pp, "setjmp: EN: %i", m_enode->m_index); + pp_printf (pp, "setjmp: EN: %i", get_enode_index ()); } /* Concrete implementation of sm_context, wiring it up to the rest of this @@ -1172,11 +1172,11 @@ exploded_node::on_longjmp (exploded_graph &eg, if (!setjmp_sval) return; + const setjmp_record tmp_setjmp_record = setjmp_sval->get_setjmp_record (); + /* Build a custom enode and eedge for rewinding from the longjmp call back to the setjmp. */ - - const exploded_node *enode_origin = setjmp_sval->get_exploded_node (); - rewind_info_t rewind_info (enode_origin); + rewind_info_t rewind_info (tmp_setjmp_record); const gcall *setjmp_call = rewind_info.get_setjmp_call (); const program_point &setjmp_point = rewind_info.get_setjmp_point (); @@ -1217,7 +1217,7 @@ exploded_node::on_longjmp (exploded_graph &eg, exploded_edge *eedge = eg.add_edge (const_cast (this), next, NULL, change, - new rewind_info_t (enode_origin)); + new rewind_info_t (tmp_setjmp_record)); /* For any diagnostics that were queued here (such as leaks) we want the checker_path to show the rewinding events after the "final event" diff --git a/gcc/analyzer/exploded-graph.h b/gcc/analyzer/exploded-graph.h index 57636fbe07c..3d1445c87ad 100644 --- a/gcc/analyzer/exploded-graph.h +++ b/gcc/analyzer/exploded-graph.h @@ -307,8 +307,8 @@ private: class rewind_info_t : public exploded_edge::custom_info_t { public: - rewind_info_t (const exploded_node *enode_origin) - : m_enode_origin (enode_origin) + rewind_info_t (const setjmp_record &setjmp_record) + : m_setjmp_record (setjmp_record) {} void print (pretty_printer *pp) FINAL OVERRIDE @@ -324,7 +324,7 @@ public: const program_point &get_setjmp_point () const { - const program_point &origin_point = m_enode_origin->get_point (); + const program_point &origin_point = get_enode_origin ()->get_point (); /* "origin_point" ought to be before the call to "setjmp". */ gcc_assert (origin_point.get_kind () == PK_BEFORE_STMT); @@ -336,13 +336,16 @@ public: const gcall *get_setjmp_call () const { - return as_a (get_setjmp_point ().get_stmt ()); + return m_setjmp_record.m_setjmp_call; } - const exploded_node *get_enode_origin () const { return m_enode_origin; } + const exploded_node *get_enode_origin () const + { + return m_setjmp_record.m_enode; + } private: - const exploded_node *m_enode_origin; + setjmp_record m_setjmp_record; }; /* Statistics about aspects of an exploded_graph. */ diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index 0bca5c0fd71..25a22f8fc65 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -3660,7 +3660,7 @@ region_model::dump_summary_of_map (pretty_printer *pp, case SK_SETJMP: dump_separator (pp, is_first); pp_printf (pp, "setjmp: EN: %i", - sval->dyn_cast_setjmp_svalue ()->get_index ()); + sval->dyn_cast_setjmp_svalue ()->get_enode_index ()); break; } } @@ -4493,10 +4493,11 @@ region_model::on_setjmp (const gcall *call, const exploded_node *enode, region_id buf_rid = deref_rvalue (gimple_call_arg (call, 0), ctxt); region *buf = get_region (buf_rid); - /* Create a setjmp_svalue for ENODE and store it in BUF_RID's region. */ + /* Create a setjmp_svalue for this call and store it in BUF_RID's region. */ if (buf) { - svalue *sval = new setjmp_svalue (enode, buf->get_type ()); + setjmp_record r (enode, call); + svalue *sval = new setjmp_svalue (r, buf->get_type ()); svalue_id new_sid = add_svalue (sval); set_value (buf_rid, new_sid, ctxt); } diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h index 1090b96bfaa..f7fb7b0b6d0 100644 --- a/gcc/analyzer/region-model.h +++ b/gcc/analyzer/region-model.h @@ -718,20 +718,42 @@ is_a_helper ::test (svalue *sval) namespace ana { +/* A bundle of information recording a setjmp call, corresponding roughly + to a jmp_buf. */ + +struct setjmp_record +{ + setjmp_record (const exploded_node *enode, + const gcall *setjmp_call) + : m_enode (enode), m_setjmp_call (setjmp_call) + { + } + + bool operator== (const setjmp_record &other) const + { + return (m_enode == other.m_enode + && m_setjmp_call == other.m_setjmp_call); + } + + const exploded_node *m_enode; + const gcall *m_setjmp_call; +}; + /* Concrete subclass of svalue representing setjmp buffers, so that longjmp can potentially "return" to an entirely different function. */ class setjmp_svalue : public svalue { public: - setjmp_svalue (const exploded_node *enode, tree type) - : svalue (type), m_enode (enode) + setjmp_svalue (const setjmp_record &setjmp_record, + tree type) + : svalue (type), m_setjmp_record (setjmp_record) {} bool compare_fields (const setjmp_svalue &other) const; svalue *clone () const FINAL OVERRIDE - { return new setjmp_svalue (m_enode, get_type ()); } + { return new setjmp_svalue (m_setjmp_record, get_type ()); } enum svalue_kind get_kind () const FINAL OVERRIDE { return SK_SETJMP; } @@ -739,9 +761,9 @@ public: setjmp_svalue *dyn_cast_setjmp_svalue () FINAL OVERRIDE { return this; } - int get_index () const; + int get_enode_index () const; - const exploded_node *get_exploded_node () const { return m_enode; } + const setjmp_record &get_setjmp_record () const { return m_setjmp_record; } private: void print_details (const region_model &model, @@ -749,7 +771,7 @@ public: pretty_printer *pp) const FINAL OVERRIDE; - const exploded_node *m_enode; + setjmp_record m_setjmp_record; }; /* An enum for discriminating between the different concrete subclasses diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index 5f07ffec0f9..dd6d7890d05 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2020-01-22 David Malcolm + + PR analyzer/93378 + * gcc.dg/analyzer/setjmp-pr93378.c: New test. + 2020-01-22 David Malcolm PR analyzer/93316 diff --git a/gcc/testsuite/gcc.dg/analyzer/setjmp-pr93378.c b/gcc/testsuite/gcc.dg/analyzer/setjmp-pr93378.c new file mode 100644 index 00000000000..7934a40301d --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/setjmp-pr93378.c @@ -0,0 +1,15 @@ +/* { dg-additional-options "-O1 -g" } */ + +#include + +jmp_buf buf; + +int +test (void) +{ + if (_setjmp (buf) != 0) + return 0; + + longjmp (buf, 1); + return 1; +} -- 2.30.2