analyzer: fix ICEs in region_model::get_lvalue_1 [PR 93388]
authorDavid Malcolm <dmalcolm@redhat.com>
Fri, 14 Feb 2020 02:17:11 +0000 (21:17 -0500)
committerDavid Malcolm <dmalcolm@redhat.com>
Mon, 17 Feb 2020 07:20:36 +0000 (02:20 -0500)
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.

gcc/analyzer/ChangeLog
gcc/analyzer/engine.cc
gcc/analyzer/exploded-graph.h
gcc/analyzer/program-state.cc
gcc/analyzer/program-state.h
gcc/analyzer/region-model.cc
gcc/analyzer/region-model.h
gcc/testsuite/ChangeLog
gcc/testsuite/gcc.dg/analyzer/torture/20060625-1.c [new file with mode: 0644]
gcc/testsuite/gcc.dg/analyzer/torture/pr51628-30.c [new file with mode: 0644]
gcc/testsuite/gcc.dg/analyzer/torture/pr59037.c [new file with mode: 0644]

index 5945abc04ee98acaa24e208e9ed5a693384892cb..d669c989ac119f20824a098ea40e249a331ab691 100644 (file)
@@ -1,3 +1,28 @@
+2020-02-17  David Malcolm  <dmalcolm@redhat.com>
+
+       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  <dmalcolm@redhat.com>
 
        * sm-malloc.cc (malloc_diagnostic::describe_state_change): For
index 17507c7c08ea23a200cb5219bcd411e08354458a..cd4ffe55dc5f8622732890c3159af2371b9b441a 100644 (file)
@@ -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 ());
index bb1f3ccff1ad9c535eea1c7366800ae2f8cdb6ff..614c37ce9afb74a86dd082d81eff1da3f371e203 100644 (file)
@@ -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
index 82b921eb969d6d6c3cad9ceff474dea9ca685573..fb96e3c976bba3f4f0ad11bdcba27c2f124a1b8f 100644 (file)
@@ -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.  */
index a4608c7498d09cc7230bb53d1a2b4f78c8f3ea07..80df649f565378064e8c59b92f45ffab79502d2f 100644 (file)
@@ -286,6 +286,11 @@ public:
   /* TODO: lose the pointer here (const-correctness issues?).  */
   region_model *m_region_model;
   auto_delete_vec<sm_state_map> 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.  */
index ae810f5eb4b64d59b668e7194435520ea4295441..b67660cf864b776875fbe8666234fe8abb573624 100644 (file)
@@ -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;
     }
 }
 
index 9c9a936fae2cd38e133133faf0bb437f3a4d9aa6..dc56d64a43bb6f1a408265884245ed9092c8ba97 100644 (file)
@@ -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<pending_diagnostic> m_diagnostics;
index a08ad2e71755586b37562a9927ca3dff310d784b..6c34e0bbe36a865d11befd0b3bbc7799b03084dd 100644 (file)
@@ -1,3 +1,10 @@
+2020-02-17  David Malcolm  <dmalcolm@redhat.com>
+
+       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  <dmalcolm@redhat.com>
 
        * 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 (file)
index 0000000..2657a92
--- /dev/null
@@ -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 (file)
index 0000000..4513e0f
--- /dev/null
@@ -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 (file)
index 0000000..d01aaec
--- /dev/null
@@ -0,0 +1 @@
+#include "../../../c-c++-common/pr59037.c"