Skip to content

Add support for path exploration to CBMC/JBMC, in addition to full bounded model checking #1641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 21, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ src/goto-cc/goto-cc.exe
src/goto-cc/goto-cl.exe
src/goto-instrument/goto-instrument
src/goto-instrument/goto-instrument.exe
src/jbmc/jbmc
src/musketeer/musketeer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

src/musketeer/musketeer.exe
src/symex/symex
Expand Down
4 changes: 2 additions & 2 deletions scripts/cpplint.py
Original file line number Diff line number Diff line change
Expand Up @@ -3625,8 +3625,8 @@ def CheckOperatorSpacing(filename, clean_lines, linenum, error):
# 'Remove spaces around %s' % match.group(0))

# check any inherited classes don't have a space between the type and the :
if Search(r'(struct|class)\s[\w_]*\s+:', line):
error(filename, linenum, 'readability/identifier_spacing', 4, 'There shouldn\'t be a space between class identifier and :')
if Search(r'(struct|class)\s[\w_]*:', line):
error(filename, linenum, 'readability/identifier_spacing', 4, 'There should be a space between class identifier and :')

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sigh Another lint vs. clang-format issue. Thanks for fixing.

#check type definitions end with t
# Look for class declarations and check the final character is a t
Expand Down
1 change: 1 addition & 0 deletions src/analyses/dirty.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ void dirtyt::find_dirty_address_of(const exprt &expr)

void dirtyt::output(std::ostream &out) const
{
die_if_uninitialized();
for(const auto &d : dirty)
out << d << '\n';
}
Expand Down
41 changes: 36 additions & 5 deletions src/analyses/dirty.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,49 +17,80 @@ Date: March 2013
#include <unordered_set>

#include <util/std_expr.h>
#include <util/invariant.h>
#include <goto-programs/goto_functions.h>

class dirtyt
{
private:
void die_if_uninitialized() const
{
INVARIANT(
initialized,
"Uninitialized dirtyt. This dirtyt was constructed using the default "
"constructor and not subsequently initialized using "
"dirtyt::build().");
}

public:
bool initialized;
typedef std::unordered_set<irep_idt, irep_id_hash> id_sett;
typedef goto_functionst::goto_functiont goto_functiont;

dirtyt()
/// \post dirtyt objects that are created through this constructor are not
/// safe to use. If you copied a dirtyt (for example, by adding an object
/// that owns a dirtyt to a container and then copying it back out), you will
/// need to re-initialize the dirtyt by calling dirtyt::build().
dirtyt() : initialized(false)
{
}

explicit dirtyt(const goto_functiont &goto_function)
explicit dirtyt(const goto_functiont &goto_function) : initialized(false)
{
build(goto_function);
initialized = true;
}

explicit dirtyt(const goto_functionst &goto_functions)
explicit dirtyt(const goto_functionst &goto_functions) : initialized(false)
{
forall_goto_functions(it, goto_functions)
build(it->second);
build(goto_functions);
// build(g_funs) responsible for setting initialized to true, since
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This possibly isn't your design - but it would seem to be better for this constructor only to be visible, then you don't need to separately track initialised and have constructors as the only way build this object (of course removing the default constructor). (Possibly mirroring Martin's comment above).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks thk123. I believe that this isn't possible:

  • goto_symex_statet has a dirtyt member, which needs to be initialized upon creation of the goto_symex_statet.
  • However, goto_symex_statet is not actually supplied with a goto_functions object until just before it is used for symbolic execution---it does not receive a goto_functions object when it is constructed. The code was previously designed so that statets were only constructed when needed in goto_symex_main since they take so much memory, and so they are not a member of goto_symext.
  • Therefore, we need to initialize the dirtyt only after the statet has been given a goto_functionst.

Does this make sense? I'll add comments clarifying this. Note that the code had previously been doing a similar (but less safe) thing: statet had a pointer to a dirtyt instead of a member, and the dirtyt was a nullptr until supplied with a goto_functions. I changed this because we now are actually copying states around and copying objects that contain stale pointers turned out to lead to much more obscure error messages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, the only way round this I can think of is having a interface dirtyt, an unitialized_dirtyt and an initalized_dirtyt that inherit from it. Then the unitialized one could just have INVARIANT(false)) on all the relevant method calls and the initlaized_dirtyt can only be constructed with the right values.

However, not even convinced myself this is better so I won't block the PR on it.

// it is public and can be called independently
}

void output(std::ostream &out) const;

bool operator()(const irep_idt &id) const
{
die_if_uninitialized();
return dirty.find(id)!=dirty.end();
}

bool operator()(const symbol_exprt &expr) const
{
die_if_uninitialized();
return operator()(expr.get_identifier());
}

const id_sett &get_dirty_ids() const
{
die_if_uninitialized();
return dirty;
}

void add_function(const goto_functiont &goto_function)
{
build(goto_function);
initialized = true;
}

void build(const goto_functionst &goto_functions)
{
// dirtyts should not be initialized twice
PRECONDITION(!initialized);
forall_goto_functions(it, goto_functions)
build(it->second);
initialized = true;
}

protected:
Expand Down
123 changes: 121 additions & 2 deletions src/cbmc/bmc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,24 @@ Author: Daniel Kroening, [email protected]
#include "bmc.h"

#include <chrono>
#include <exception>
#include <fstream>
#include <iostream>
#include <memory>

#include <util/exit_codes.h>
#include <util/string2int.h>
#include <util/source_location.h>
#include <util/string_utils.h>
#include <util/memory_info.h>
#include <util/message.h>
#include <util/json.h>
#include <util/cprover_prefix.h>

#include <langapi/mode.h>
#include <langapi/language_util.h>

#include <goto-programs/goto_model.h>
#include <goto-programs/xml_goto_trace.h>
#include <goto-programs/json_goto_trace.h>
#include <goto-programs/graphml_witness.h>
Expand All @@ -37,6 +41,7 @@ Author: Daniel Kroening, [email protected]
#include <goto-symex/memory_model_tso.h>
#include <goto-symex/memory_model_pso.h>

#include "cbmc_solvers.h"
#include "counterexample_beautification.h"
#include "fault_localization.h"

Expand Down Expand Up @@ -359,8 +364,7 @@ safety_checkert::resultt bmct::execute(const goto_functionst &goto_functions)
{
try
{
// perform symbolic execution
symex.symex_from_entry_point_of(goto_functions);
perform_symbolic_execution(goto_functions);

// add a partial ordering, if required
if(equation.has_threads())
Expand Down Expand Up @@ -595,3 +599,118 @@ void bmct::setup_unwind()
if(options.get_option("unwind")!="")
symex.set_unwind_limit(options.get_unsigned_int_option("unwind"));
}

int bmct::do_language_agnostic_bmc(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add function-level doxy comments

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prevalent style seems to be having the doxy comments in header files, right? I already have one there, but it's a bit brief so I'll expand it.

const optionst &opts,
const goto_modelt &goto_model,
const ui_message_handlert::uit &ui,
messaget &message,
std::function<void(bmct &, const goto_modelt &)> frontend_configure_bmc)
{
message_handlert &mh = message.get_message_handler();
safety_checkert::resultt result;
goto_symext::branch_worklistt worklist;
try
{
{
cbmc_solverst solvers(
opts, goto_model.symbol_table, message.get_message_handler());
solvers.set_ui(ui);
std::unique_ptr<cbmc_solverst::solvert> cbmc_solver;
cbmc_solver = solvers.get_solver();
prop_convt &pc = cbmc_solver->prop_conv();
bmct bmc(opts, goto_model.symbol_table, mh, pc, worklist);
bmc.set_ui(ui);
frontend_configure_bmc(bmc, goto_model);
result = bmc.run(goto_model.goto_functions);
}
INVARIANT(
opts.get_bool_option("paths") || worklist.empty(),
"the worklist should be empty after doing full-program "
"model checking, but the worklist contains " +
std::to_string(worklist.size()) + " unexplored branches.");

// When model checking, the bmc.run() above will already have explored
// the entire program, and result contains the verification result. The
// worklist (containing paths that have not yet been explored) is thus
// empty, and we don't enter this loop.
//
// When doing path exploration, there will be some saved paths left to
// explore in the worklist. We thus need to run the above code again,
// once for each saved path in the worklist, to continue symbolically
// execute the program. Note that the code in the loop is similar to
// the code above except that we construct a path_explorert rather than
// a bmct, which allows us to execute from a saved state rather than
// from the entry point. See the path_explorert documentation, and the
// difference between the implementations of perform_symbolic_exection()
// in bmct and path_explorert, for more information.

while(!worklist.empty())
{
message.status() << "___________________________\n"
<< "Starting new path (" << worklist.size()
<< " to go)\n"
<< message.eom;
cbmc_solverst solvers(
opts, goto_model.symbol_table, message.get_message_handler());
solvers.set_ui(ui);
std::unique_ptr<cbmc_solverst::solvert> cbmc_solver;
cbmc_solver = solvers.get_solver();
prop_convt &pc = cbmc_solver->prop_conv();
goto_symext::branch_pointt &resume = worklist.front();
path_explorert pe(
opts,
goto_model.symbol_table,
mh,
pc,
resume.equation,
resume.state,
worklist);
frontend_configure_bmc(pe, goto_model);
result &= pe.run(goto_model.goto_functions);
worklist.pop_front();
}
}
catch(const char *error_msg)
{
message.error() << error_msg << message.eom;
return CPROVER_EXIT_EXCEPTION;
}
catch(const std::string &error_msg)
{
message.error() << error_msg << message.eom;
return CPROVER_EXIT_EXCEPTION;
}
catch(...)
{
message.error() << "unable to get solver" << message.eom;
throw std::current_exception();
}

switch(result)
{
case safety_checkert::resultt::SAFE:
return CPROVER_EXIT_VERIFICATION_SAFE;
case safety_checkert::resultt::UNSAFE:
return CPROVER_EXIT_VERIFICATION_UNSAFE;
case safety_checkert::resultt::ERROR:
return CPROVER_EXIT_INTERNAL_ERROR;
}
UNREACHABLE;
}

void bmct::perform_symbolic_execution(const goto_functionst &goto_functions)
{
symex.symex_from_entry_point_of(goto_functions, symex_symbol_table);
INVARIANT(
options.get_bool_option("paths") || branch_worklist.empty(),
"Branch points were saved even though we should have been "
"executing the entire program and merging paths");
}

void path_explorert::perform_symbolic_execution(
const goto_functionst &goto_functions)
{
symex.resume_symex_from_saved_state(
goto_functions, saved_state, &equation, symex_symbol_table);
}
Loading