Skip to content

Cleanup error handling of cbmc/ folder #2703

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
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
6 changes: 3 additions & 3 deletions src/cbmc/bmc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Author: Daniel Kroening, [email protected]
#include <chrono>
#include <iostream>

#include <util/exception_utils.h>
#include <util/exit_codes.h>

#include <langapi/language_util.h>
Expand Down Expand Up @@ -214,9 +215,8 @@ void bmct::get_memory_model()
memory_model=util_make_unique<memory_model_psot>(ns);
else
{
error() << "Invalid memory model " << mm
<< " -- use one of sc, tso, pso" << eom;
throw "invalid memory model";
throw invalid_user_input_exceptiont(
"invalid parameter " + mm, "--mm", "try values of sc, tso, pso");
}
}

Expand Down
6 changes: 4 additions & 2 deletions src/cbmc/bmc_cover.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ bool bmc_covert::operator()()
{
if(it->is_assert())
{
assert(it->source.pc->is_assert());
PRECONDITION(it->source.pc->is_assert());
const and_exprt c(
literal_exprt(it->guard_literal), literal_exprt(!it->cond_literal));
literalt l_c=solver.convert(c);
Expand All @@ -239,7 +239,9 @@ bool bmc_covert::operator()()
cover_goals.add(l);
}

assert(cover_goals.size()==goal_map.size());
INVARIANT(cover_goals.size() == goal_map.size(),
"we add coverage for each goal");


status() << "Running " << solver.decision_procedure_text() << eom;

Expand Down
2 changes: 1 addition & 1 deletion src/cbmc/cbmc_main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ int main(int argc, const char **argv)
#endif
cbmc_parse_optionst parse_options(argc, argv);

int res=parse_options.main();
int res = parse_options.main();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary change?


#ifdef IREP_HASH_STATS
std::cout << "IREP_HASH_CNT=" << irep_hash_cnt << '\n';
Expand Down
43 changes: 31 additions & 12 deletions src/cbmc/cbmc_solvers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ Author: Daniel Kroening, [email protected]
#include <fstream>
#include <iostream>
#include <memory>
#include <string>

#include <util/exception_utils.h>
#include <util/make_unique.h>
#include <util/unicode.h>
#include <util/version.h>
Expand All @@ -32,7 +34,8 @@ Author: Daniel Kroening, [email protected]
/// \return An smt2_dect::solvert giving the solver to use.
smt2_dect::solvert cbmc_solverst::get_smt2_solver_type() const
{
assert(options.get_bool_option("smt2"));
// we shouldn't get here if this option isn't set
PRECONDITION(options.get_bool_option("smt2"));

smt2_dect::solvert s=smt2_dect::solvert::GENERIC;

Expand Down Expand Up @@ -163,8 +166,10 @@ std::unique_ptr<cbmc_solverst::solvert> cbmc_solverst::get_smt2(
{
if(solver==smt2_dect::solvert::GENERIC)
{
error() << "please use --outfile" << eom;
throw 0;
throw invalid_user_input_exceptiont(
"required filename not provided",
"--outfile",
"provide a filename with --outfile");
}

auto smt2_dec = util_make_unique<smt2_dect>(
Expand Down Expand Up @@ -206,8 +211,8 @@ std::unique_ptr<cbmc_solverst::solvert> cbmc_solverst::get_smt2(

if(!*out)
{
error() << "failed to open " << filename << eom;
throw 0;
throw invalid_user_input_exceptiont(
"failed to open file: " + filename, "--outfile");
}

auto smt2_conv = util_make_unique<smt2_convt>(
Expand All @@ -231,18 +236,32 @@ void cbmc_solverst::no_beautification()
{
if(options.get_bool_option("beautify"))
{
error() << "sorry, this solver does not support beautification" << eom;
throw 0;
throw invalid_user_input_exceptiont(
"the chosen solver does not support beautification", "--beautify");
}
}

void cbmc_solverst::no_incremental_check()
{
if(options.get_bool_option("all-properties") ||
options.get_option("cover")!="" ||
options.get_option("incremental-check")!="")
const bool all_properties = options.get_bool_option("all-properties");
const bool cover = options.is_set("cover");
const bool incremental_check = options.is_set("incremental-check");

if(all_properties)
{
throw invalid_user_input_exceptiont(
"the chosen solver does not support incremental solving",
"--all_properties");
}
else if(cover)
{
throw invalid_user_input_exceptiont(
"the chosen solver does not support incremental solving", "--cover");
}
else if(incremental_check)
{
error() << "sorry, this solver does not support incremental solving" << eom;
throw 0;
throw invalid_user_input_exceptiont(
"the chosen solver does not support incremental solving",
"--incremental-check");
}
}
4 changes: 2 additions & 2 deletions src/cbmc/cbmc_solvers.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,13 @@ class cbmc_solverst:public messaget

prop_convt &prop_conv() const
{
assert(prop_conv_ptr!=nullptr);
PRECONDITION(prop_conv_ptr != nullptr);
return *prop_conv_ptr;
}

propt &prop() const
{
assert(prop_ptr!=nullptr);
PRECONDITION(prop_ptr != nullptr);
return *prop_ptr;
}

Expand Down
4 changes: 2 additions & 2 deletions src/cbmc/fault_localization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ fault_localizationt::get_failed_property()
bool fault_localizationt::check(const lpointst &lpoints,
const lpoints_valuet &value)
{
assert(value.size()==lpoints.size());
PRECONDITION(value.size() == lpoints.size());
bvt assumptions;
lpoints_valuet::const_iterator v_it=value.begin();
for(const auto &l : lpoints)
Expand Down Expand Up @@ -142,7 +142,7 @@ void fault_localizationt::run(irep_idt goal_id)
{
// find failed property
failed=get_failed_property();
assert(failed!=bmc.equation.SSA_steps.end());
PRECONDITION(failed != bmc.equation.SSA_steps.end());

if(goal_id==ID_nil)
goal_id=failed->source.pc->source_location.get_property_id();
Expand Down
22 changes: 15 additions & 7 deletions src/cbmc/symex_coverage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,17 @@ goto_program_coverage_recordt::goto_program_coverage_recordt(
const symex_coveraget::coveraget &coverage):
coverage_recordt("method")
{
assert(gf_it->second.body_available());
PRECONDITION(gf_it->second.body_available());

// identify the file name, inlined functions aren't properly
// accounted for
goto_programt::const_targett end_function=
--gf_it->second.body.instructions.end();
assert(end_function->is_end_function());
DATA_INVARIANT(
end_function->is_end_function(),
"last instruction in a function body is end function");
file_name=end_function->source_location.get_file();
assert(!file_name.empty());
DATA_INVARIANT(!file_name.empty(), "should have a valid source location");

// compute the maximum coverage of individual source-code lines
coverage_lines_mapt coverage_lines_map;
Expand Down Expand Up @@ -260,11 +262,15 @@ void goto_program_coverage_recordt::compute_coverage_lines(
for(const auto &cov : c_entry->second)
std::cerr << cov.second.succ->location_number << '\n';
}
assert(c_entry->second.size()==1 || is_branch);
DATA_INVARIANT(
c_entry->second.size() == 1 || is_branch,
"instructions other than branch instructions have exactly 1 successor");

for(const auto &cov : c_entry->second)
{
assert(cov.second.num_executions>0);
DATA_INVARIANT(
cov.second.num_executions > 0,
"coverage entries can only exist with at least one execution");

if(entry.first->second.hits==0)
++lines_covered;
Expand All @@ -275,7 +281,9 @@ void goto_program_coverage_recordt::compute_coverage_lines(
if(is_branch)
{
auto cond_entry=entry.first->second.conditions.find(it);
assert(cond_entry!=entry.first->second.conditions.end());
INVARIANT(
cond_entry != entry.first->second.conditions.end(),
"branch should have condition");

if(it->get_target()==cov.second.succ)
{
Expand Down Expand Up @@ -439,7 +447,7 @@ bool symex_coveraget::generate_report(
const goto_functionst &goto_functions,
const std::string &path) const
{
assert(!path.empty());
PRECONDITION(!path.empty());

if(path=="-")
return output_report(goto_functions, std::cout);
Expand Down
1 change: 1 addition & 0 deletions src/util/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ SRC = arith_tools.cpp \
expr.cpp \
expr_initializer.cpp \
expr_util.cpp \
exception_utils.cpp \
file_util.cpp \
find_macros.cpp \
find_symbols.cpp \
Expand Down
20 changes: 20 additions & 0 deletions src/util/exception_utils.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*******************************************************************\

Module: Exception helper utilities

Author: Fotis Koutoulakis, [email protected]

\*******************************************************************/

#include "exception_utils.h"

std::string invalid_user_input_exceptiont::what() const noexcept
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, there should also be a version hat can serialise into json and XML in a more structure way (in a future PR).

{
std::string res;
res += "\nInvalid User Input\n";
res += "Option: " + option + "\n";
res += "Reason: " + reason;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wishlist: please add a test for this, i.e., intentionally use CBMC with incorrect options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you need this as part of this PR, or can it go in as part of a follow up PR for example?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Up to you, as long as it happens at some point :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

We plan to make some further improvements to user error handling, so we'll definitely be adding suitable test cases as part of that work, so I'd be happy for this PR to go in, with tests following as part of the later improvements.

// Print an optional correct usage message assuming correct input parameters have been passed
res += correct_input + "\n";
return res;
}
36 changes: 36 additions & 0 deletions src/util/exception_utils.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*******************************************************************\

Module: Exception helper utilities

Author: Fotis Koutoulakis, [email protected]

\*******************************************************************/

#ifndef CPROVER_UTIL_EXCEPTION_UTILS_H
#define CPROVER_UTIL_EXCEPTION_UTILS_H

#include <string>

class invalid_user_input_exceptiont
{
/// The reason this exception was generated.
std::string reason;
/// The full command line option (not the argument) that got
/// erroneous input.
std::string option;
/// In case we have samples of correct input to the option.
std::string correct_input;

public:
invalid_user_input_exceptiont(
std::string reason,
std::string option,
std::string correct_input = "")
: reason(reason), option(option), correct_input(correct_input)
{
}

std::string what() const noexcept;
};

#endif // CPROVER_UTIL_EXCEPTION_UTILS_H
39 changes: 26 additions & 13 deletions src/util/parse_options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ Author: Daniel Kroening, [email protected]
#endif

#include "cmdline.h"
#include "exception_utils.h"
#include "exit_codes.h"
#include "signal_catcher.h"

parse_options_baset::parse_options_baset(
Expand Down Expand Up @@ -47,23 +49,34 @@ void parse_options_baset::unknown_option_msg()

int parse_options_baset::main()
{
if(parse_result)
// catch all exceptions here so that this code is not duplicated
// for each tool
try
{
usage_error();
unknown_option_msg();
return EX_USAGE;
if(parse_result)
{
usage_error();
unknown_option_msg();
return EX_USAGE;
}

if(cmdline.isset('?') || cmdline.isset('h') || cmdline.isset("help"))
{
help();
return EX_OK;
}

// install signal catcher
install_signal_catcher();

return doit();
}

if(cmdline.isset('?') || cmdline.isset('h') || cmdline.isset("help"))
catch(invalid_user_input_exceptiont &e)
{
help();
return EX_OK;
std::cerr << e.what() << "\n";
return CPROVER_EXIT_USAGE_ERROR;
}

// install signal catcher
install_signal_catcher();

return doit();
return CPROVER_EXIT_SUCCESS;
}

std::string
Expand Down