analyzer: unify bounds-checking class hierarchies

Convert out-of-bounds class hierarchy from:

  pending_diagnostic
    out_of_bounds
      past_the_end
        buffer_overflow (*)
        buffer_over_read (*)
      buffer_underwrite (*)
      buffer_under_read (*)
    symbolic_past_the_end
      symbolic_buffer_overflow (*)
      symbolic_buffer_over_read (*)

to:

  pending_diagnostic
    out_of_bounds
      concrete_out_of_bounds
        concrete_past_the_end
          concrete_buffer_overflow (*)
          concrete_buffer_over_read (*)
        concrete_buffer_underwrite (*)
        concrete_buffer_under_read (*)
      symbolic_past_the_end
        symbolic_buffer_overflow (*)
        symbolic_buffer_over_read (*)

where the concrete classes (i.e. the instantiable ones) are marked
with a (*).

Doing so undercovered a bug where, for CWE-131-examples.c, we were
emitting an extra:
  warning: heap-based buffer over-read [CWE-122] [-Wanalyzer-out-of-bounds]
at the:
  WidgetList[numWidgets] = NULL;
The issue was that within set_next_state we get the rvalue for the LHS,
which looks like a read to the bounds-checker.  The patch fixes this by
passing NULL as the region_model_context * for such accesses.

gcc/analyzer/ChangeLog:
	* bounds-checking.cc (class out_of_bounds): Split out from...
	(class concrete_out_of_bounds): New abstract subclass.
	(class past_the_end): Rename to...
	(class concrete_past_the_end): ...this, and make a subclass of
	concrete_out_of_bounds.
	(class buffer_overflow): Rename to...
	(class concrete_buffer_overflow): ...this, and make a subclass of
	concrete_past_the_end.
	(class buffer_over_read): Rename to...
	(class concrete_buffer_over_read): ...this, and make a subclass of
	concrete_past_the_end.
	(class buffer_underwrite): Rename to...
	(class concrete_buffer_underwrite): ...this, and make a subclass
	of concrete_out_of_bounds.
	(class buffer_under_read): Rename to...
	(class concrete_buffer_under_read): ...this, and make a subclass
	of concrete_out_of_bounds.
	(class symbolic_past_the_end): Convert to a subclass of
	out_of_bounds.
	(symbolic_buffer_overflow::get_kind): New.
	(symbolic_buffer_over_read::get_kind): New.
	(region_model::check_region_bounds): Update for renamings.
	* engine.cc (impl_sm_context::set_next_state): Eliminate
	"new_ctxt", passing NULL to get_rvalue instead.
	(impl_sm_context::warn): Likewise.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>

2 files changed, 115 insertions, 94 deletions

diff --git a/gcc/analyzer/bounds-checking.cc b/gcc/analyzer/bounds-checking.cc
index bc7d2dd..aaf3f22 100644
--- a/gcc/analyzer/bounds-checking.cc
+++ b/gcc/analyzer/bounds-checking.cc
@@ -37,27 +37,21 @@ along with GCC; see the file COPYING3.  If not see
 
 namespace ana {
 
-/* Abstract base class for all out-of-bounds warnings with concrete values.  */
+/* Abstract base class for all out-of-bounds warnings.  */
 
-class out_of_bounds : public pending_diagnostic_subclass<out_of_bounds>
+class out_of_bounds : public pending_diagnostic
 {
 public:
-  out_of_bounds (const region *reg, tree diag_arg,
-		 byte_range out_of_bounds_range)
-  : m_reg (reg), m_diag_arg (diag_arg),
-    m_out_of_bounds_range (out_of_bounds_range)
+  out_of_bounds (const region *reg, tree diag_arg)
+  : m_reg (reg), m_diag_arg (diag_arg)
   {}
 
-  const char *get_kind () const final override
-  {
-    return "out_of_bounds_diagnostic";
-  }
-
-  bool operator== (const out_of_bounds &other) const
+  bool subclass_equal_p (const pending_diagnostic &base_other) const override
   {
-    return m_reg == other.m_reg
-	   && m_out_of_bounds_range == other.m_out_of_bounds_range
-	   && pending_diagnostic::same_tree_p (m_diag_arg, other.m_diag_arg);
+    const out_of_bounds &other
+      (static_cast <const out_of_bounds &>(base_other));
+    return (m_reg == other.m_reg
+	    && pending_diagnostic::same_tree_p (m_diag_arg, other.m_diag_arg));
   }
 
   int get_controlling_option () const final override
@@ -106,25 +100,51 @@ protected:
 
   const region *m_reg;
   tree m_diag_arg;
+};
+
+/* Abstract base class for all out-of-bounds warnings where the
+   out-of-bounds range is concrete.  */
+
+class concrete_out_of_bounds : public out_of_bounds
+{
+public:
+  concrete_out_of_bounds (const region *reg, tree diag_arg,
+			  byte_range out_of_bounds_range)
+  : out_of_bounds (reg, diag_arg),
+    m_out_of_bounds_range (out_of_bounds_range)
+  {}
+
+  bool subclass_equal_p (const pending_diagnostic &base_other) const override
+  {
+    const concrete_out_of_bounds &other
+      (static_cast <const concrete_out_of_bounds &>(base_other));
+    return (out_of_bounds::subclass_equal_p (other)
+	    && m_out_of_bounds_range == other.m_out_of_bounds_range);
+  }
+
+protected:
   byte_range m_out_of_bounds_range;
 };
 
-/* Abstract subclass to complaing about out-of-bounds
+/* Abstract subclass to complaing about concrete out-of-bounds
    past the end of the buffer.  */
 
-class past_the_end : public out_of_bounds
+class concrete_past_the_end : public concrete_out_of_bounds
 {
 public:
-  past_the_end (const region *reg, tree diag_arg, byte_range range,
-		tree byte_bound)
-  : out_of_bounds (reg, diag_arg, range), m_byte_bound (byte_bound)
+  concrete_past_the_end (const region *reg, tree diag_arg, byte_range range,
+			 tree byte_bound)
+  : concrete_out_of_bounds (reg, diag_arg, range), m_byte_bound (byte_bound)
   {}
 
-  bool operator== (const past_the_end &other) const
+  bool
+  subclass_equal_p (const pending_diagnostic &base_other) const final override
   {
-    return out_of_bounds::operator== (other)
-	   && pending_diagnostic::same_tree_p (m_byte_bound,
-					       other.m_byte_bound);
+    const concrete_past_the_end &other
+      (static_cast <const concrete_past_the_end &>(base_other));
+    return (concrete_out_of_bounds::subclass_equal_p (other)
+	    && pending_diagnostic::same_tree_p (m_byte_bound,
+						other.m_byte_bound));
   }
 
   label_text
@@ -143,14 +163,19 @@ protected:
 
 /* Concrete subclass to complain about buffer overflows.  */
 
-class buffer_overflow : public past_the_end
+class concrete_buffer_overflow : public concrete_past_the_end
 {
 public:
-  buffer_overflow (const region *reg, tree diag_arg,
+  concrete_buffer_overflow (const region *reg, tree diag_arg,
 		   byte_range range, tree byte_bound)
-  : past_the_end (reg, diag_arg, range, byte_bound)
+  : concrete_past_the_end (reg, diag_arg, range, byte_bound)
   {}
 
+  const char *get_kind () const final override
+  {
+    return "concrete_buffer_overflow";
+  }
+
   bool emit (rich_location *rich_loc) final override
   {
     diagnostic_metadata m;
@@ -241,14 +266,19 @@ public:
 
 /* Concrete subclass to complain about buffer over-reads.  */
 
-class buffer_over_read : public past_the_end
+class concrete_buffer_over_read : public concrete_past_the_end
 {
 public:
-  buffer_over_read (const region *reg, tree diag_arg,
-		    byte_range range, tree byte_bound)
-  : past_the_end (reg, diag_arg, range, byte_bound)
+  concrete_buffer_over_read (const region *reg, tree diag_arg,
+			     byte_range range, tree byte_bound)
+  : concrete_past_the_end (reg, diag_arg, range, byte_bound)
   {}
 
+  const char *get_kind () const final override
+  {
+    return "concrete_buffer_over_read";
+  }
+
   bool emit (rich_location *rich_loc) final override
   {
     diagnostic_metadata m;
@@ -337,13 +367,19 @@ public:
 
 /* Concrete subclass to complain about buffer underwrites.  */
 
-class buffer_underwrite : public out_of_bounds
+class concrete_buffer_underwrite : public concrete_out_of_bounds
 {
 public:
-  buffer_underwrite (const region *reg, tree diag_arg, byte_range range)
-  : out_of_bounds (reg, diag_arg, range)
+  concrete_buffer_underwrite (const region *reg, tree diag_arg,
+			      byte_range range)
+  : concrete_out_of_bounds (reg, diag_arg, range)
   {}
 
+  const char *get_kind () const final override
+  {
+    return "concrete_buffer_underwrite";
+  }
+
   bool emit (rich_location *rich_loc) final override
   {
     diagnostic_metadata m;
@@ -403,13 +439,19 @@ public:
 
 /* Concrete subclass to complain about buffer under-reads.  */
 
-class buffer_under_read : public out_of_bounds
+class concrete_buffer_under_read : public concrete_out_of_bounds
 {
 public:
-  buffer_under_read (const region *reg, tree diag_arg, byte_range range)
-  : out_of_bounds (reg, diag_arg, range)
+  concrete_buffer_under_read (const region *reg, tree diag_arg,
+			      byte_range range)
+  : concrete_out_of_bounds (reg, diag_arg, range)
   {}
 
+  const char *get_kind () const final override
+  {
+    return "concrete_buffer_under_read";
+  }
+
   bool emit (rich_location *rich_loc) final override
   {
     diagnostic_metadata m;
@@ -470,38 +512,26 @@ public:
 /* Abstract class to complain about out-of-bounds read/writes where
    the values are symbolic.  */
 
-class symbolic_past_the_end
-  : public pending_diagnostic_subclass<symbolic_past_the_end>
+class symbolic_past_the_end : public out_of_bounds
 {
 public:
   symbolic_past_the_end (const region *reg, tree diag_arg, tree offset,
 			 tree num_bytes, tree capacity)
-  : m_reg (reg), m_diag_arg (diag_arg), m_offset (offset),
-    m_num_bytes (num_bytes), m_capacity (capacity)
+  : out_of_bounds (reg, diag_arg),
+    m_offset (offset),
+    m_num_bytes (num_bytes),
+    m_capacity (capacity)
   {}
 
-  const char *get_kind () const final override
-  {
-    return "symbolic_past_the_end";
-  }
-
-  bool operator== (const symbolic_past_the_end &other) const
-  {
-    return m_reg == other.m_reg
-	   && pending_diagnostic::same_tree_p (m_diag_arg, other.m_diag_arg)
-	   && pending_diagnostic::same_tree_p (m_offset, other.m_offset)
-	   && pending_diagnostic::same_tree_p (m_num_bytes, other.m_num_bytes)
-	   && pending_diagnostic::same_tree_p (m_capacity, other.m_capacity);
-  }
-
-  int get_controlling_option () const final override
-  {
-    return OPT_Wanalyzer_out_of_bounds;
-  }
-
-  void mark_interesting_stuff (interesting_t *interest) final override
+  bool
+  subclass_equal_p (const pending_diagnostic &base_other) const final override
   {
-    interest->add_region_creation (m_reg);
+    const symbolic_past_the_end &other
+      (static_cast <const symbolic_past_the_end &>(base_other));
+    return (out_of_bounds::subclass_equal_p (other)
+	    && pending_diagnostic::same_tree_p (m_offset, other.m_offset)
+	    && pending_diagnostic::same_tree_p (m_num_bytes, other.m_num_bytes)
+	    && pending_diagnostic::same_tree_p (m_capacity, other.m_capacity));
   }
 
   label_text
@@ -566,13 +596,6 @@ public:
   }
 
 protected:
-  enum memory_space get_memory_space () const
-  {
-    return m_reg->get_memory_space ();
-  }
-
-  const region *m_reg;
-  tree m_diag_arg;
   tree m_offset;
   tree m_num_bytes;
   tree m_capacity;
@@ -591,6 +614,11 @@ public:
     m_dir_str = "write";
   }
 
+  const char *get_kind () const final override
+  {
+    return "symbolic_buffer_overflow";
+  }
+
   bool emit (rich_location *rich_loc) final override
   {
     diagnostic_metadata m;
@@ -624,6 +652,11 @@ public:
     m_dir_str = "read";
   }
 
+  const char *get_kind () const final override
+  {
+    return "symbolic_buffer_over_read";
+  }
+
   bool emit (rich_location *rich_loc) final override
   {
     diagnostic_metadata m;
@@ -776,10 +809,12 @@ region_model::check_region_bounds (const region *reg,
 	  gcc_unreachable ();
 	  break;
 	case DIR_READ:
-	  ctxt->warn (make_unique<buffer_under_read> (reg, diag_arg, out));
+	  ctxt->warn (make_unique<concrete_buffer_under_read> (reg, diag_arg,
+							       out));
 	  break;
 	case DIR_WRITE:
-	  ctxt->warn (make_unique<buffer_underwrite> (reg, diag_arg, out));
+	  ctxt->warn (make_unique<concrete_buffer_underwrite> (reg, diag_arg,
+							       out));
 	  break;
 	}
     }
@@ -804,12 +839,12 @@ region_model::check_region_bounds (const region *reg,
 	  gcc_unreachable ();
 	  break;
 	case DIR_READ:
-	  ctxt->warn (make_unique<buffer_over_read> (reg, diag_arg,
-						     out, byte_bound));
+	  ctxt->warn (make_unique<concrete_buffer_over_read> (reg, diag_arg,
+							      out, byte_bound));
 	  break;
 	case DIR_WRITE:
-	  ctxt->warn (make_unique<buffer_overflow> (reg, diag_arg,
-						    out, byte_bound));
+	  ctxt->warn (make_unique<concrete_buffer_overflow> (reg, diag_arg,
+							     out, byte_bound));
 	  break;
 	}
     }
diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
index 0c49bb2..991b592 100644
--- a/gcc/analyzer/engine.cc
+++ b/gcc/analyzer/engine.cc
@@ -310,21 +310,17 @@ public:
   }
 
 
-  void set_next_state (const gimple *stmt,
+  void set_next_state (const gimple *,
 		       tree var,
 		       state_machine::state_t to,
 		       tree origin) final override
   {
     logger * const logger = get_logger ();
     LOG_FUNC (logger);
-    impl_region_model_context new_ctxt (m_eg, m_enode_for_diag,
-					m_old_state, m_new_state,
-					NULL, NULL,
-					stmt);
     const svalue *var_new_sval
-      = m_new_state->m_region_model->get_rvalue (var, &new_ctxt);
+      = m_new_state->m_region_model->get_rvalue (var, NULL);
     const svalue *origin_new_sval
-      = m_new_state->m_region_model->get_rvalue (origin, &new_ctxt);
+      = m_new_state->m_region_model->get_rvalue (origin, NULL);
 
     /* We use the new sval here to avoid issues with uninitialized values.  */
     state_machine::state_t current
@@ -350,12 +346,8 @@ public:
       (m_eg, m_enode_for_diag, NULL, NULL, NULL/*m_enode->get_state ()*/,
        NULL, stmt);
 
-    impl_region_model_context new_ctxt (m_eg, m_enode_for_diag,
-					m_old_state, m_new_state,
-					NULL, NULL,
-					stmt);
     const svalue *origin_new_sval
-      = m_new_state->m_region_model->get_rvalue (origin, &new_ctxt);
+      = m_new_state->m_region_model->get_rvalue (origin, NULL);
 
     state_machine::state_t current
       = m_old_smap->get_state (sval, m_eg.get_ext_state ());
@@ -380,11 +372,8 @@ public:
   {
     LOG_FUNC (get_logger ());
     gcc_assert (d);
-    impl_region_model_context old_ctxt
-      (m_eg, m_enode_for_diag, m_old_state, m_new_state, NULL, NULL, NULL);
-
     const svalue *var_old_sval
-      = m_old_state->m_region_model->get_rvalue (var, &old_ctxt);
+      = m_old_state->m_region_model->get_rvalue (var, NULL);
     state_machine::state_t current
       = (var
 	 ? m_old_smap->get_state (var_old_sval, m_eg.get_ext_state ())
@@ -400,9 +389,6 @@ public:
   {
     LOG_FUNC (get_logger ());
     gcc_assert (d);
-    impl_region_model_context old_ctxt
-      (m_eg, m_enode_for_diag, m_old_state, m_new_state, NULL, NULL, NULL);
-
     state_machine::state_t current
       = (sval
 	 ? m_old_smap->get_state (sval, m_eg.get_ext_state ())