From 5864e229ab236d109573a7a820dbb3ca3ba944b2 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Mon, 27 May 2019 23:57:04 +0100
Subject: [PATCH 20/28] FIXME: various cleanups

---
 gcc/analyzer/constraint-manager.cc |  4 +--
 gcc/analyzer/region-model.cc       | 74 ++++++++++++++++++++++++--------------
 gcc/analyzer/region-model.h        | 24 +++++++------
 3 files changed, 64 insertions(+), 38 deletions(-)

diff --git a/gcc/analyzer/constraint-manager.cc b/gcc/analyzer/constraint-manager.cc
index 5685c48..6d245ac 100644
--- a/gcc/analyzer/constraint-manager.cc
+++ b/gcc/analyzer/constraint-manager.cc
@@ -222,8 +222,8 @@ equiv_class::remap_svalue_ids (const svalue_id_map &map)
   int i;
   svalue_id *iv;
   FOR_EACH_VEC_ELT (m_vars, i, iv)
-    *iv = map.get_dst_for_src (*iv);
-  m_cst_sid = map.get_dst_for_src (m_cst_sid);
+    map.update (iv);
+  map.update (&m_cst_sid);
 }
 
 /* FIXME.  */
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 57ce0c4..bc49e5d 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -151,7 +151,7 @@ region_id::dump_node_name_to_pp (pretty_printer *pp) const
 
 /* class id_map.  */
 
-/* FIXME.  */
+/* id_map's ctor, which populates the map with dummy null values.  */
 
 template <typename T>
 id_map<T>::id_map (int num_svalues)
@@ -165,7 +165,7 @@ id_map<T>::id_map (int num_svalues)
     }
 }
 
-/* FIXME.  */
+/* Record that SRC is to be mapped to DST.  */
 
 template <typename T>
 void
@@ -175,7 +175,7 @@ id_map<T>::put (T src, T dst)
   m_dst_to_src[dst.as_int ()] = src;
 }
 
-/* FIXME.  */
+/* Get the new value for SRC within the map.  */
 
 template <typename T>
 T
@@ -186,7 +186,8 @@ id_map<T>::get_dst_for_src (T src) const
   return m_src_to_dst[src.as_int ()];
 }
 
-/* FIXME.  */
+/* Given DST, a new value, determine which old value will be mapped to it
+   (the inverse of the map).  */
 
 template <typename T>
 T
@@ -245,19 +246,21 @@ id_map<T>::dump () const
   pp_flush (&pp);
 }
 
-/* FIXME.  */
+/* Update *ID from the old value to its new value in this map.  */
 
 template <typename T>
 void
-id_map<T>::update (T &id) const
+id_map<T>::update (T *id) const
 {
-  id = get_dst_for_src (id);
+  *id = get_dst_for_src (*id);
 }
 
 ////////////////////////////////////////////////////////////////////////////
 
 /* class id_set.  */
 
+/* id_set<region_id>'s ctor.  */
+
 template<>
 id_set<region_id>::id_set (const region_model *model)
 : m_bitmap (model->m_regions.length ())
@@ -452,7 +455,7 @@ region_svalue::dump_dot_to_pp (const region_model &model,
 void
 region_svalue::remap_region_ids (const region_id_map &map)
 {
-  m_rid = map.get_dst_for_src (m_rid);
+  map.update (&m_rid);
 }
 
 /* class constant_svalue : public svalue.  */
@@ -876,7 +879,7 @@ region::to_str () const
 void
 region::remap_svalue_ids (const svalue_id_map &map)
 {
-  m_sval_id = map.get_dst_for_src (m_sval_id);
+  map.update (&m_sval_id);
 }
 
 /* FIXME.  */
@@ -884,7 +887,7 @@ region::remap_svalue_ids (const svalue_id_map &map)
 void
 region::remap_region_ids (const region_id_map &map)
 {
-  map.update (m_parent_rid);
+  map.update (&m_parent_rid);
 }
 
 /* FIXME.  */
@@ -1300,7 +1303,7 @@ stack_region::remap_region_ids (const region_id_map &map)
   int i;
   region_id *frame_rid;
   FOR_EACH_VEC_ELT (m_frame_rids, i, frame_rid)
-    map.update (m_frame_rids[i]);
+    map.update (&m_frame_rids[i]);
 }
 
 /* class heap_region : public region.  */
@@ -1544,9 +1547,9 @@ root_region::ensure_heap_region (region_model *model)
 void
 root_region::remap_region_ids (const region_id_map &map)
 {
-  map.update (m_stack_rid);
-  map.update (m_globals_rid);
-  map.update (m_heap_rid);
+  map.update (&m_stack_rid);
+  map.update (&m_globals_rid);
+  map.update (&m_heap_rid);
 }
 
 /* FIXME.  */
@@ -2563,7 +2566,8 @@ region_model::add_constraint (tree lhs, enum tree_code op, tree rhs)
   if (t_cond.is_true ())
     return true;
 
-  /* Reject constraint that contradicts existing knowledge as unsatisfiable.  */
+  /* Reject a constraint that would contradict existing knowledge, as
+     unsatisfiable.  */
   if (t_cond.is_false ())
     return false;
 
@@ -2583,7 +2587,7 @@ region_model::eval_condition (tree lhs,
   return eval_condition (get_rvalue (lhs), op, get_rvalue (rhs));
 }
 
-/* If SID is a constant, return the underlying tree value.
+/* If SID is a constant value, return the underlying tree constant.
    Otherwise, return NULL_TREE.  */
 
 tree
@@ -2596,7 +2600,7 @@ region_model::maybe_get_constant (svalue_id sid)
   return NULL_TREE;
 }
 
-/* FIXME.  */
+/* Set DST_RID value to be a new unknown value of type TYPE.  */
 
 svalue_id
 region_model::set_to_new_unknown_value (region_id dst_rid, tree type)
@@ -2657,7 +2661,10 @@ region_model::apply_constraints_for_edge (const superedge &edge,
   return true;
 }
 
-/* FIXME.  */
+/* Push a new frame_region on to the stack region.
+   Populate the frame_region with child regions for the function call's
+   parameters, using values from the arguments at the callsite in the
+   caller's frame.  */
 
 void
 region_model::update_for_call_superedge (const call_superedge &call_edge)
@@ -2680,7 +2687,9 @@ region_model::update_for_call_superedge (const call_superedge &call_edge)
   push_frame (call_edge.get_callee_function (), &arg_sids);
 }
 
-/* FIXME.  */
+/* Pop the top-most frame_region from the stack, and store the svalue
+   for any returned value into the region for the lvalue of the LHS of
+   the call (if any).  */
 
 void
 region_model::update_for_return_superedge (const return_superedge &return_edge)
@@ -2699,7 +2708,10 @@ region_model::update_for_return_superedge (const return_superedge &return_edge)
     set_value (get_lvalue (lhs), result_sid);
 }
 
-/* FIXME.  */
+/* Given a true or false edge guarded by conditional statement COND_STMT,
+   add appropriate constraints from the conditional and return true,
+   or return false if the constraints contradict existing knowledge
+   (and so the edge should not be taken).  */
 
 bool
 region_model::apply_constraints_for_gcond (const superedge &edge,
@@ -2895,13 +2907,17 @@ private:
 
 /* Remove unused svalues from this model, accumulating stats into STATS.
 
-   Treat KNOWN_USED as used (this is for handling values being returned
-   from functions as their frame is popped).
-
    This is important for ensuring that the analysis of loops terminates.
    Otherwise, we could generate a succession of models with unreferenced
    "unknown" values, where the number of redundant unknown values could
-   grow without bounds, and each such model would be treated as distinct.  */
+   grow without bounds, and each such model would be treated as distinct.
+
+   If KNOWN_USED is non-NULL, treat *KNOWN_USED as used (this is for
+   handling values being returned from functions as their frame is popped,
+   since otherwise we'd have to simultaneously determine both the rvalue
+   of the return expr in the callee frame and the lvalue for the gcall's
+   assignment in the caller frame, and it seems cleaner to express all
+   lvalue and rvalue lookups implicitly relative to a "current" frame).  */
 
 void
 region_model::purge_svalues (purge_stats *stats, svalue_id *known_used_sid)
@@ -2975,7 +2991,7 @@ region_model::purge_svalues (purge_stats *stats, svalue_id *known_used_sid)
     }
 
   if (known_used_sid)
-    map.update (*known_used_sid);
+    map.update (known_used_sid);
 
   validate ();
 }
@@ -3138,6 +3154,8 @@ debug (const region_model &rmodel)
 
 namespace selftest {
 
+/* Smoketest for region_model::dump_to_pp.  */
+
 static void
 test_dump ()
 {
@@ -3165,6 +3183,9 @@ test_dump ()
 		"  constraints:\n");
 }
 
+/* Verify that calling region_model::get_rvalue repeatedly on the same
+   tree constant retrieves the same svalue_id.  */
+
 static void
 test_unique_constants ()
 {
@@ -3173,6 +3194,7 @@ test_unique_constants ()
 
   region_model model;
   ASSERT_EQ (model.get_rvalue (int_0), model.get_rvalue (int_0));
+  ASSERT_EQ (model.get_rvalue (int_42), model.get_rvalue (int_42));
   ASSERT_NE (model.get_rvalue (int_0), model.get_rvalue (int_42));
 }
 
@@ -3348,7 +3370,7 @@ test_assignment ()
 
 #endif
 
-/* FIXME.  */
+/* Verify the details of pushing and popping stack frames.  */
 
 static void
 test_stack_frames ()
diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
index 8957532..dc20d6b 100644
--- a/gcc/analyzer/region-model.h
+++ b/gcc/analyzer/region-model.h
@@ -42,8 +42,8 @@ class constraint_manager;
    We store per-model IDs rather than pointers to make it easier to clone
    and to compare graphs.  */
 
-/* An ID for an svalue within a region_model.  Internally this is an index
-   into a vector.  */
+/* An ID for an svalue within a region_model.  Internally, this is an index
+   into a vector of svalue * within the region_model.  */
 
 class svalue_id
 {
@@ -77,8 +77,8 @@ private:
   int m_idx;
 };
 
-/* An ID for a region within a region_model.  Internally this is an index
-   into a vector.  */
+/* An ID for a region within a region_model.  Internally, this is an index
+   into a vector of region * within the region_model.  */
 
 class region_id
 {
@@ -111,8 +111,10 @@ private:
   int m_idx;
 };
 
-/* A mapping of IDs to IDs (e.g. when removing one or more elements,
-   thus needing to renumber).  */
+/* A class for renumbering IDs within a region_model, mapping old IDs
+   to new IDs (e.g. when removing one or more elements, thus needing to
+   renumber).  */
+// TODO: could this be useful for equiv_class_ids?
 
 template <typename T>
 class id_map
@@ -124,7 +126,7 @@ class id_map
   T get_src_for_dst (T dst) const;
   void dump_to_pp (pretty_printer *pp) const;
   void dump () const;
-  void update (T&) const;
+  void update (T *) const;
 
  private:
   auto_vec<T> m_src_to_dst;
@@ -136,7 +138,7 @@ typedef id_map<region_id> region_id_map;
 
 ////////////////////////////////////////////////////////////////////////////
 
-/* FIXME.  */
+/* A set of IDs within a region_model (either svalue_id or region_id).  */
 
 template <typename T>
 class id_set
@@ -170,8 +172,10 @@ typedef id_set<region_id> region_id_set;
 
 ////////////////////////////////////////////////////////////////////////////
 
-/* Stats on how much was purged, for various operations that delete
-   information.  */
+/* Various operations delete information from a region_model.
+
+   This struct tracks how many of each kind of entity were purged (e.g.
+   for selftests, and for debugging).  */
 
 struct purge_stats
 {
-- 
1.8.5.3

