Well-formedness checking of SSA equation [depends on #3480]

src/cbmc/bmc.cpp

@@ -639,6 +639,12 @@ void bmct::perform_symbolic_execution(
   goto_symext::get_goto_functiont get_goto_function)
 {
   symex.symex_from_entry_point_of(get_goto_function, symex_symbol_table);
+
+  if(options.get_bool_option("validate-ssa-equation"))
+  {
+    symex.validate(ns, validation_modet::INVARIANT);
+  }
+
   INVARIANT(
     options.get_bool_option("paths") || path_storage.empty(),
     "Branch points were saved even though we should have been "

src/cbmc/cbmc_parse_options.cpp

@@ -398,6 +398,11 @@ void cbmc_parse_optionst::get_command_line_options(optionst &options)
       "symex-coverage-report",
       cmdline.get_value("symex-coverage-report"));
 
+  if(cmdline.isset("validate-ssa-equation"))
+  {
+    options.set_option("validate-ssa-equation", true);
+  }
+
   PARSE_OPTIONS_GOTO_TRACE(cmdline, options);
 }
 

src/goto-programs/goto_program.cpp

@@ -706,9 +706,11 @@ void goto_programt::instructiont::validate(
     break;
   case ASSIGN:
     DATA_CHECK(
+      vm,
       code.get_statement() == ID_assign,
       "assign instruction should contain an assign statement");
-    DATA_CHECK(targets.empty(), "assign instruction should not have a target");
+    DATA_CHECK(
+      vm, targets.empty(), "assign instruction should not have a target");
     break;
   case DECL:
     break;

src/goto-symex/goto_symex.h

@@ -61,6 +61,12 @@ struct symex_configt final
   bool partial_loops;
   mp_integer debug_level;
 
+  /// \brief Should the additional validation checks be run?
+  ///
+  /// If this flag is set the checks for renaming (both level1 and level2) are
+  /// executed in the goto_symex_statet (in the assignment method).
+  bool run_validation_checks;
+
   explicit symex_configt(const optionst &options);
 };
 
@@ -455,6 +461,11 @@ class goto_symext
       "attempting to read remaining_vccs");
     return _remaining_vccs;
   }
+
+  void validate(const namespacet &ns, const validation_modet vm) const
+  {
+    target.validate(ns, vm);
+  }
 };
 
 void symex_transition(goto_symext::statet &state);

src/goto-symex/goto_symex_state.cpp

@@ -121,32 +121,6 @@ static bool check_renaming(const exprt &expr)
   return false;
 }
 
-static void assert_l1_renaming(const exprt &expr)
-{
-  #if 1
-  if(check_renaming_l1(expr))
-  {
-    std::cerr << expr.pretty() << '\n';
-    UNREACHABLE;
-  }
-  #else
-  (void)expr;
-  #endif
-}
-
-static void assert_l2_renaming(const exprt &expr)
-{
-  #if 1
-  if(check_renaming(expr))
-  {
-    std::cerr << expr.pretty() << '\n';
-    UNREACHABLE;
-  }
-  #else
-  (void)expr;
-  #endif
-}
-
 class goto_symex_is_constantt : public is_constantt
 {
 protected:
@@ -197,15 +171,18 @@ void goto_symex_statet::assignment(
   // the type might need renaming
   rename(lhs.type(), l1_identifier, ns);
   lhs.update_type();
-  assert_l1_renaming(lhs);
+  if(run_validation_checks)
+  {
+    DATA_INVARIANT(!check_renaming_l1(lhs), "lhs renaming failed on l1");
+  }
 
-  #if 0
+#if 0
   PRECONDITION(l1_identifier != get_original_name(l1_identifier)
       || l1_identifier=="goto_symex::\\guard"
       || ns.lookup(l1_identifier).is_shared()
       || has_prefix(id2string(l1_identifier), "symex::invalid_object")
       || has_prefix(id2string(l1_identifier), "symex_dynamic::dynamic_object"));
-  #endif
+#endif
 
   // do the l2 renaming
   if(level2.current_names.find(l1_identifier)==level2.current_names.end())
@@ -216,8 +193,11 @@ void goto_symex_statet::assignment(
   // in case we happen to be multi-threaded, record the memory access
   bool is_shared=l2_thread_write_encoding(lhs, ns);
 
-  assert_l2_renaming(lhs);
-  assert_l2_renaming(rhs);
+  if(run_validation_checks)
+  {
+    DATA_INVARIANT(!check_renaming(lhs), "lhs renaming failed on l2");
+    DATA_INVARIANT(!check_renaming(rhs), "rhs renaming failed on l2");
+  }
 
   // see #305 on GitHub for a simple example and possible discussion
   if(is_shared && lhs.type().id() == ID_pointer && !allow_pointer_unsoundness)
@@ -240,8 +220,11 @@ void goto_symex_statet::assignment(
     ssa_exprt l1_lhs(lhs);
     get_l1_name(l1_lhs);
 
-    assert_l1_renaming(l1_lhs);
-    assert_l1_renaming(l1_rhs);
+    if(run_validation_checks)
+    {
+      DATA_INVARIANT(!check_renaming_l1(l1_lhs), "lhs renaming failed on l1");
+      DATA_INVARIANT(!check_renaming_l1(l1_rhs), "rhs renaming failed on l1");
+    }
 
     value_set.assign(l1_lhs, l1_rhs, ns, rhs_is_simplified, is_shared);
   }

src/goto-symex/goto_symex_state.h

@@ -268,6 +268,9 @@ class goto_symex_statet final
   /// of a GOTO
   bool has_saved_next_instruction;
 
+  /// \brief Should the additional validation checks be run?
+  bool run_validation_checks;
+
 private:
   /// \brief Dangerous, do not use
   ///

src/goto-symex/symex_main.cpp

@@ -34,7 +34,8 @@ symex_configt::symex_configt(const optionst &options)
     simplify_opt(options.get_bool_option("simplify")),
     unwinding_assertions(options.get_bool_option("unwinding-assertions")),
     partial_loops(options.get_bool_option("partial-loops")),
-    debug_level(unsafe_string2int(options.get_option("debug-level")))
+    debug_level(unsafe_string2int(options.get_option("debug-level"))),
+    run_validation_checks(options.get_bool_option("validate-ssa-equation"))
 {
 }
 
@@ -307,6 +308,8 @@ void goto_symext::symex_from_entry_point_of(
 
   statet state;
 
+  state.run_validation_checks = symex_config.run_validation_checks;
+
   initialize_entry_point(
     state,
     get_goto_function,

src/goto-symex/symex_target_equation.cpp

@@ -978,3 +978,63 @@ void symex_target_equationt::SSA_stept::output(std::ostream &out) const
 
   out << "Guard: " << format(guard) << '\n';
 }
+
+/// Check that the SSA step is well-formed
+/// \param ns namespace to lookup identifiers
+/// \param vm validation mode to be used for reporting failures
+void symex_target_equationt::SSA_stept::validate(
+  const namespacet &ns,
+  const validation_modet vm) const
+{
+  validate_full_expr(guard, ns, vm);
+
+  switch(type)
+  {
+  case goto_trace_stept::typet::ASSERT:
+  case goto_trace_stept::typet::ASSUME:
+  case goto_trace_stept::typet::GOTO:
+  case goto_trace_stept::typet::CONSTRAINT:
+    validate_full_expr(cond_expr, ns, vm);
+    break;
+  case goto_trace_stept::typet::ASSIGNMENT:
+  case goto_trace_stept::typet::DECL:
+    validate_full_expr(ssa_lhs, ns, vm);
+    validate_full_expr(ssa_full_lhs, ns, vm);
+    validate_full_expr(original_full_lhs, ns, vm);
+    validate_full_expr(ssa_rhs, ns, vm);
+    DATA_CHECK(
+      vm,
+      base_type_eq(ssa_lhs.get_original_expr(), ssa_rhs, ns),
+      "Type inequality in SSA assignment\nlhs-type: " +
+        ssa_lhs.get_original_expr().type().id_string() +
+        "\nrhs-type: " + ssa_rhs.type().id_string());
+    break;
+  case goto_trace_stept::typet::INPUT:
+  case goto_trace_stept::typet::OUTPUT:
+    for(const auto &expr : io_args)
+      validate_full_expr(expr, ns, vm);
+    break;
+  case goto_trace_stept::typet::FUNCTION_CALL:
+    for(const auto &expr : ssa_function_arguments)
+      validate_full_expr(expr, ns, vm);
+  case goto_trace_stept::typet::FUNCTION_RETURN:
+  {
+    const symbolt *symbol;
+    DATA_CHECK(
+      vm,
+      called_function.empty() || !ns.lookup(called_function, symbol),
+      "unknown function identifier " + id2string(called_function));
+  }
+  break;
+  case goto_trace_stept::typet::NONE:
+  case goto_trace_stept::typet::LOCATION:
+  case goto_trace_stept::typet::DEAD:
+  case goto_trace_stept::typet::SHARED_READ:
+  case goto_trace_stept::typet::SHARED_WRITE:
+  case goto_trace_stept::typet::SPAWN:
+  case goto_trace_stept::typet::MEMORY_BARRIER:
+  case goto_trace_stept::typet::ATOMIC_BEGIN:
+  case goto_trace_stept::typet::ATOMIC_END:
+    break;
+  }
+}

src/goto-symex/symex_target_equation.h

@@ -267,6 +267,8 @@ class symex_target_equationt:public symex_targett
       std::ostream &out) const;
 
     void output(std::ostream &out) const;
+
+    void validate(const namespacet &ns, const validation_modet vm) const;
   };
 
   std::size_t count_assertions() const
@@ -323,6 +325,12 @@ class symex_target_equationt:public symex_targett
     return false;
   }
 
+  void validate(const namespacet &ns, const validation_modet vm) const
+  {
+    for(const SSA_stept &step : SSA_steps)
+      step.validate(ns, vm);
+  }
+
 protected:
   // for enforcing sharing in the expressions stored
   merge_irept merge_irep;

src/util/ssa_expr.h

@@ -139,6 +139,27 @@ class ssa_exprt:public symbol_exprt
   /* Used to determine whether or not an identifier can be built
    * before trying and getting an exception */
   static bool can_build_identifier(const exprt &src);
+
+  static void check(
+    const exprt &expr,
+    const validation_modet vm = validation_modet::INVARIANT)
+  {
+    DATA_CHECK(
+      vm, !expr.has_operands(), "SSA expression should not have operands");
+    DATA_CHECK(
+      vm, expr.id() == ID_symbol, "SSA expression symbols are symbols");
+    DATA_CHECK(vm, expr.get_bool(ID_C_SSA_symbol), "wrong SSA expression ID");
+  }
+
+  static void validate(
+    const exprt &expr,
+    const namespacet &ns,
+    const validation_modet vm = validation_modet::INVARIANT)
+  {
+    check(expr, vm);
+    validate_full_expr(
+      static_cast<const exprt &>(expr.find(ID_expression)), ns, vm);
+  }
 };
 
 /*! \brief Cast a generic exprt to an \ref ssa_exprt

src/util/std_code.h

@@ -291,7 +291,7 @@ class code_assignt:public codet
     const validation_modet vm = validation_modet::INVARIANT)
   {
     DATA_CHECK(
-      code.operands().size() == 2, "assignment must have two operands");
+      vm, code.operands().size() == 2, "assignment must have two operands");
   }
 
   static void validate(
@@ -302,6 +302,7 @@ class code_assignt:public codet
     check(code, vm);
 
     DATA_CHECK(
+      vm,
       base_type_eq(code.op0().type(), code.op1().type(), ns),
       "lhs and rhs of assignment must have same type");
   }

src/util/std_expr.h

@@ -777,7 +777,9 @@ class binary_exprt:public exprt
     const validation_modet vm = validation_modet::INVARIANT)
   {
     DATA_CHECK(
-      expr.operands().size() == 2, "binary expression must have two operands");
+      vm,
+      expr.operands().size() == 2,
+      "binary expression must have two operands");
   }
 
   static void validate(
@@ -859,6 +861,7 @@ class binary_predicate_exprt:public binary_exprt
     binary_exprt::validate(expr, ns, vm);
 
     DATA_CHECK(
+      vm,
       expr.type().id() == ID_bool,
       "result of binary predicate expression should be of type bool");
   }
@@ -901,6 +904,7 @@ class binary_relation_exprt:public binary_predicate_exprt
 
     // check types
     DATA_CHECK(
+      vm,
       base_type_eq(expr.op0().type(), expr.op1().type(), ns),
       "lhs and rhs of binary relation expression should have same type");
   }

src/util/std_types.h

@@ -1149,7 +1149,8 @@ class bitvector_typet : public typet
     const typet &type,
     const validation_modet vm = validation_modet::INVARIANT)
   {
-    DATA_CHECK(!type.get(ID_width).empty(), "bitvector type must have width");
+    DATA_CHECK(
+      vm, !type.get(ID_width).empty(), "bitvector type must have width");
   }
 };
 

src/util/symbol_table.cpp

@@ -130,6 +130,7 @@ void symbol_tablet::validate(const validation_modet vm) const
 
     // Check that symbols[id].name == id
     DATA_CHECK_WITH_DIAGNOSTICS(
+      vm,
       symbol.name == symbol_key,
       "Symbol table entry must map to a symbol with the correct identifier",
       "Symbol table key '",
@@ -152,6 +153,7 @@ void symbol_tablet::validate(const validation_modet vm) const
           }) != symbol_base_map.end();
 
       DATA_CHECK_WITH_DIAGNOSTICS(
+        vm,
         base_map_matches_symbol,
         "The base_name of a symbol should map to itself",
         "Symbol table key '",
@@ -174,6 +176,7 @@ void symbol_tablet::validate(const validation_modet vm) const
           }) != symbol_module_map.end();
 
       DATA_CHECK_WITH_DIAGNOSTICS(
+        vm,
         module_map_matches_symbol,
         "Symbol table module map should map to symbol",
         "Symbol table key '",
@@ -188,6 +191,7 @@ void symbol_tablet::validate(const validation_modet vm) const
   for(auto base_map_entry : symbol_base_map)
   {
     DATA_CHECK_WITH_DIAGNOSTICS(
+      vm,
       has_symbol(base_map_entry.second),
       "Symbol table base_name map entries must map to a symbol name",
       "base_name map entry '",
@@ -201,6 +205,7 @@ void symbol_tablet::validate(const validation_modet vm) const
   for(auto module_map_entry : symbol_module_map)
   {
     DATA_CHECK_WITH_DIAGNOSTICS(
+      vm,
       has_symbol(module_map_entry.second),
       "Symbol table module map entries must map to a symbol name",
       "base_name map entry '",