Move goto_programt::(const_)targett comparison to a struct

This makes sure no one can inadvertently use less-than comparison on
those iterators, which may lead to non-reproducible behaviour
(see #6782).

Author: tautschnig

jbmc/src/java_bytecode/java_bytecode_convert_method_class.h

@@ -240,7 +240,23 @@ class java_bytecode_convert_methodt
 
 public:
   typedef std::map<method_offsett, converted_instructiont> address_mapt;
-  typedef std::pair<const methodt &, const address_mapt &> method_with_amapt;
+  struct method_with_amapt
+  {
+    method_with_amapt(const methodt &m, const address_mapt &a)
+      : method_with_amap(m, a)
+    {
+    }
+
+    std::pair<const methodt &, const address_mapt &> method_with_amap;
+
+    struct target_less_than // NOLINT(readability/identifiers)
+    {
+      bool operator()(const method_offsett &a, const method_offsett &b) const
+      {
+        return a < b;
+      }
+    };
+  };
   typedef cfg_dominators_templatet<method_with_amapt, method_offsett, false>
     java_cfg_dominatorst;
 

jbmc/src/java_bytecode/java_local_variable_table.cpp

@@ -46,8 +46,8 @@ struct procedure_local_cfg_baset<
 
   void operator()(const method_with_amapt &args)
   {
-    const auto &method=args.first;
-    const auto &amap=args.second;
+    const auto &method = args.method_with_amap.first;
+    const auto &amap = args.method_with_amap.second;
     for(const auto &inst : amap)
     {
       // Map instruction PCs onto node indices:
@@ -109,18 +109,18 @@ struct procedure_local_cfg_baset<
   static java_bytecode_convert_methodt::method_offsett
   get_first_node(const method_with_amapt &args)
   {
-    return args.second.begin()->first;
+    return args.method_with_amap.second.begin()->first;
   }
 
   static java_bytecode_convert_methodt::method_offsett
   get_last_node(const method_with_amapt &args)
   {
-    return (--args.second.end())->first;
+    return (--args.method_with_amap.second.end())->first;
   }
 
   static bool nodes_empty(const method_with_amapt &args)
   {
-    return args.second.empty();
+    return args.method_with_amap.second.empty();
   }
 };
 

src/analyses/ai_storage.h

@@ -100,7 +100,8 @@ class ai_storage_baset
 class trace_map_storaget : public ai_storage_baset
 {
 protected:
-  typedef std::map<locationt, trace_set_ptrt> trace_mapt;
+  typedef std::map<locationt, trace_set_ptrt, goto_programt::target_less_than>
+    trace_mapt;
   trace_mapt trace_map;
 
   // This retains one part of a shared_ptr to the history object

src/analyses/call_graph.h

@@ -98,7 +98,7 @@ class call_grapht
   typedef goto_programt::const_targett locationt;
 
   /// Type of a set of callsites
-  typedef std::set<locationt> locationst;
+  typedef std::set<locationt, goto_programt::target_less_than> locationst;
 
   /// Type mapping from call-graph edges onto the set of call instructions
   /// that make that call.

src/analyses/cfg_dominators.h

@@ -36,7 +36,7 @@ template <class P, class T, bool post_dom>
 class cfg_dominators_templatet
 {
 public:
-  typedef std::set<T> target_sett;
+  typedef std::set<T, typename P::target_less_than> target_sett;
 
   struct nodet
   {
@@ -218,12 +218,12 @@ void cfg_dominators_templatet<P, T, post_dom>::fixedpoint(P &program)
       {
         if(*n_it==current)
           ++n_it;
-        else if(*n_it<*o_it)
+        else if(typename P::target_less_than()(*n_it, *o_it))
         {
           changed=true;
           node.dominators.erase(n_it++);
         }
-        else if(*o_it<*n_it)
+        else if(typename P::target_less_than()(*o_it, *n_it))
           ++o_it;
         else
         {

src/analyses/dependence_graph.h

@@ -172,7 +172,9 @@ class dep_graph_domaint:public ai_domain_baset
   node_indext node_id;
   bool has_changed;
 
-  typedef std::set<goto_programt::const_targett> depst;
+  typedef std::
+    set<goto_programt::const_targett, goto_programt::target_less_than>
+      depst;
 
   // Set of locations with control instructions on which the instruction at this
   // location has a control dependency on

src/analyses/flow_insensitive_analysis.h

@@ -93,9 +93,9 @@ class flow_insensitive_analysis_baset
   typedef flow_insensitive_abstract_domain_baset statet;
   typedef goto_programt::const_targett locationt;
 
-  std::set<locationt> seen_locations;
+  std::set<locationt, goto_programt::target_less_than> seen_locations;
 
-  std::map<locationt, unsigned> statistics;
+  std::map<locationt, unsigned, goto_programt::target_less_than> statistics;
 
   bool seen(const locationt &l)
   {
@@ -155,7 +155,11 @@ class flow_insensitive_analysis_baset
 protected:
   const namespacet &ns;
 
-  typedef std::priority_queue<locationt> working_sett;
+  typedef std::priority_queue<
+    locationt,
+    std::vector<locationt>,
+    goto_programt::target_less_than>
+    working_sett;
 
   locationt get_next(working_sett &working_set);
 

src/analyses/is_threaded.h

@@ -44,7 +44,9 @@ class is_threadedt
   }
 
 protected:
-  typedef std::set<goto_programt::const_targett> is_threaded_sett;
+  typedef std::
+    set<goto_programt::const_targett, goto_programt::target_less_than>
+      is_threaded_sett;
   is_threaded_sett is_threaded_set;
 
   void compute(

src/analyses/lexical_loops.h

@@ -60,10 +60,11 @@ Author: Diffblue Ltd
 ///   * [function] is_backwards_goto() returning a bool.
 ///   * [function] get_target() which returns an object that needs:
 ///     * [field] location_number which is an unsigned int.
-template <class P, class T>
-class lexical_loops_templatet : public loop_analysist<T>
+/// \tparam C: comparison to use over `T` typed elements
+template <class P, class T, typename C>
+class lexical_loops_templatet : public loop_analysist<T, C>
 {
-  typedef loop_analysist<T> parentt;
+  typedef loop_analysist<T, C> parentt;
 
 public:
   typedef typename parentt::loopt lexical_loopt;
@@ -103,16 +104,17 @@ class lexical_loops_templatet : public loop_analysist<T>
 
 typedef lexical_loops_templatet<
   const goto_programt,
-  goto_programt::const_targett>
+  goto_programt::const_targett,
+  goto_programt::target_less_than>
   lexical_loopst;
 
 #ifdef DEBUG
 #  include <iostream>
 #endif
 
 /// Finds all back-edges and computes the lexical loops
-template <class P, class T>
-void lexical_loops_templatet<P, T>::compute(P &program)
+template <class P, class T, typename C>
+void lexical_loops_templatet<P, T, C>::compute(P &program)
 {
   all_in_lexical_loop_form = true;
 
@@ -142,8 +144,8 @@ void lexical_loops_templatet<P, T>::compute(P &program)
 /// the tail, abandoning the candidate loop if we stray outside the bounds of
 /// the lexical region bounded by the head and tail, otherwise recording all
 /// instructions that can reach the backedge as falling within the loop.
-template <class P, class T>
-bool lexical_loops_templatet<P, T>::compute_lexical_loop(
+template <class P, class T, typename C>
+bool lexical_loops_templatet<P, T, C>::compute_lexical_loop(
   T loop_tail,
   T loop_head)
 {
@@ -152,7 +154,7 @@ bool lexical_loops_templatet<P, T>::compute_lexical_loop(
     "loop header should come lexically before the tail");
 
   std::stack<T> stack;
-  std::set<T> loop_instructions;
+  std::set<T, C> loop_instructions;
 
   loop_instructions.insert(loop_head);
   loop_instructions.insert(loop_tail);

src/analyses/local_cfg.h

@@ -29,7 +29,9 @@ class local_cfgt
     successorst successors;
   };
 
-  typedef std::map<goto_programt::const_targett, node_nrt> loc_mapt;
+  typedef std::
+    map<goto_programt::const_targett, node_nrt, goto_programt::target_less_than>
+      loc_mapt;
   loc_mapt loc_map;
 
   typedef std::vector<nodet> nodest;