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;

src/analyses/local_may_alias.h

@@ -140,7 +140,9 @@ class local_may_alias_factoryt
   typedef std::map<irep_idt, std::unique_ptr<local_may_aliast> > fkt_mapt;
   fkt_mapt fkt_map;
 
-  typedef std::map<goto_programt::const_targett, irep_idt > target_mapt;
+  typedef std::
+    map<goto_programt::const_targett, irep_idt, goto_programt::target_less_than>
+      target_mapt;
   target_mapt target_map;
 };
 

src/analyses/loop_analysis.h

@@ -15,17 +15,17 @@ Author: Diffblue Ltd
 
 #include <goto-programs/goto_model.h>
 
-template <class T>
+template <class T, typename C>
 class loop_analysist;
 
 /// A loop, specified as a set of instructions
-template <class T>
+template <class T, typename C>
 class loop_templatet
 {
-  typedef std::set<T> loop_instructionst;
+  typedef std::set<T, C> loop_instructionst;
   loop_instructionst loop_instructions;
 
-  friend loop_analysist<T>;
+  friend loop_analysist<T, C>;
 
 public:
   loop_templatet() = default;
@@ -77,13 +77,13 @@ class loop_templatet
   }
 };
 
-template <class T>
+template <class T, typename C>
 class loop_analysist
 {
 public:
-  typedef loop_templatet<T> loopt;
+  typedef loop_templatet<T, C> loopt;
   // map loop headers to loops
-  typedef std::map<T, loopt> loop_mapt;
+  typedef std::map<T, loopt, C> loop_mapt;
 
   loop_mapt loop_map;
 
@@ -98,10 +98,10 @@ class loop_analysist
   loop_analysist() = default;
 };
 
-template <typename T>
-class loop_with_parent_analysis_templatet : loop_templatet<T>
+template <typename T, typename C>
+class loop_with_parent_analysis_templatet : loop_templatet<T, C>
 {
-  typedef loop_analysist<T> parent_analysist;
+  typedef loop_analysist<T, C> parent_analysist;
 
 public:
   explicit loop_with_parent_analysis_templatet(parent_analysist &loop_analysis)
@@ -113,14 +113,14 @@ class loop_with_parent_analysis_templatet : loop_templatet<T>
   explicit loop_with_parent_analysis_templatet(
     parent_analysist &loop_analysis,
     InstructionSet &&instructions)
-    : loop_templatet<T>(std::forward<InstructionSet>(instructions)),
+    : loop_templatet<T, C>(std::forward<InstructionSet>(instructions)),
       loop_analysis(loop_analysis)
   {
   }
 
   /// Returns true if \p instruction is in \p loop
   bool loop_contains(
-    const typename loop_analysist<T>::loopt &loop,
+    const typename loop_analysist<T, C>::loopt &loop,
     const T instruction) const
   {
     return loop.loop_instructions.count(instruction);
@@ -141,15 +141,15 @@ class loop_with_parent_analysis_templatet : loop_templatet<T>
   parent_analysist &loop_analysis;
 };
 
-template <class T>
-class linked_loop_analysist : loop_analysist<T>
+template <class T, typename C>
+class linked_loop_analysist : loop_analysist<T, C>
 {
 public:
   linked_loop_analysist() = default;
 
   /// Returns true if \p instruction is in \p loop
   bool loop_contains(
-    const typename loop_analysist<T>::loopt &loop,
+    const typename loop_analysist<T, C>::loopt &loop,
     const T instruction) const
   {
     return loop.loop_instructions.count(instruction);
@@ -166,8 +166,8 @@ class linked_loop_analysist : loop_analysist<T>
 };
 
 /// Print all natural loops that were found
-template <class T>
-void loop_analysist<T>::output(std::ostream &out) const
+template <class T, typename C>
+void loop_analysist<T, C>::output(std::ostream &out) const
 {
   for(const auto &loop : loop_map)
   {

src/analyses/natural_loops.h

@@ -43,10 +43,11 @@ Author: Georg Weissenbacher, [email protected]
 ///   * [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 natural_loops_templatet : public loop_analysist<T>
+/// \tparam C: comparison to use over `T` typed elements
+template <class P, class T, typename C>
+class natural_loops_templatet : public loop_analysist<T, C>
 {
-  typedef loop_analysist<T> parentt;
+  typedef loop_analysist<T, C> parentt;
 
 public:
   typedef typename parentt::loopt natural_loopt;
@@ -80,14 +81,18 @@ class natural_loops_templatet : public loop_analysist<T>
 
 /// A concretized version of
 /// \ref natural_loops_templatet<const goto_programt, goto_programt::const_targett>
-class natural_loopst:
-    public natural_loops_templatet<const goto_programt,
-                                   goto_programt::const_targett>
+class natural_loopst : public natural_loops_templatet<
+                         const goto_programt,
+                         goto_programt::const_targett,
+                         goto_programt::target_less_than>
 {
 };
 
-typedef natural_loops_templatet<goto_programt, goto_programt::targett>
-    natural_loops_mutablet;
+typedef natural_loops_templatet<
+  goto_programt,
+  goto_programt::targett,
+  goto_programt::target_less_than>
+  natural_loops_mutablet;
 
 inline void show_natural_loops(const goto_modelt &goto_model, std::ostream &out)
 {
@@ -99,8 +104,8 @@ inline void show_natural_loops(const goto_modelt &goto_model, std::ostream &out)
 #endif
 
 /// Finds all back-edges and computes the natural loops
-template<class P, class T>
-void natural_loops_templatet<P, T>::compute(P &program)
+template <class P, class T, typename C>
+void natural_loops_templatet<P, T, C>::compute(P &program)
 {
   cfg_dominators(program);
 
@@ -133,8 +138,8 @@ void natural_loops_templatet<P, T>::compute(P &program)
 }
 
 /// Computes the natural loop for a given back-edge (see Muchnick section 7.4)
-template<class P, class T>
-void natural_loops_templatet<P, T>::compute_natural_loop(T m, T n)
+template <class P, class T, typename C>
+void natural_loops_templatet<P, T, C>::compute_natural_loop(T m, T n)
 {
   PRECONDITION(n->location_number <= m->location_number);