Skip to content

Feature/internal invariant #911

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 8 commits into from
Jun 13, 2017
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
3 changes: 3 additions & 0 deletions CODING_STANDARD
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,9 @@ C++ features:
- Use the auto keyword if and only if one of the following
- The type is explictly repeated on the RHS (e.g. a constructor call)
- Adding the type will increase confusion (e.g. iterators, function pointers)
- Avoid assert, if the condition is an actual invariant, use INVARIANT,
PRECONDITION, POSTCONDITION, CHECK_RETURN, UNREACHABLE or DATA_INVARIANT.
If there are possible reasons why it might fail, throw an exception.

Architecture-specific code:
- Avoid if possible.
Expand Down
4 changes: 4 additions & 0 deletions regression/cbmc/invariant-failure/main.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
int main()
{
return 0;
}
10 changes: 10 additions & 0 deletions regression/cbmc/invariant-failure/test.desc
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
CORE
main.c
--test-invariant-failure
^EXIT=(0|127|134|137)$
^SIGNAL=0$
Invariant check failed
^(Backtrace)|(Backtraces not supported)$
--
^warning: ignoring
^VERIFICATION SUCCESSFUL$
10 changes: 10 additions & 0 deletions regression/cpp-linter/assert/main.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// Author: Martin Brain, [email protected]

#include <cassert>
#include <assert.h>

int main(int argc, char **argv)
{
assert(0);
return 0;
}
6 changes: 6 additions & 0 deletions regression/cpp-linter/assert/test.desc
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
CORE
main.cpp

^main\.cpp:8: assert is deprecated, use INVARIANT instead \[build/deprecated\] \[4\]
^Total errors found: 1$
^SIGNAL=0$
1 change: 0 additions & 1 deletion regression/test.pl
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ ($$$$$)
print LOG " Core: $dumped_core\n";

if($signal_num != 0) {
$failed = 1;
print "Killed by signal $signal_num";
if($dumped_core) {
print " (code dumped)";
Expand Down
22 changes: 22 additions & 0 deletions scripts/cpplint.py
Original file line number Diff line number Diff line change
Expand Up @@ -4629,6 +4629,27 @@ def CheckAltTokens(filename, clean_lines, linenum, error):
_ALT_TOKEN_REPLACEMENT[match.group(1)], match.group(1)))


def CheckAssert(filename, clean_lines, linenum, error):
"""Check for uses of assert.

Args:
filename: The name of the current file.
clean_lines: A CleansedLines instance containing the file.
linenum: The number of the line to check.
error: The function to call with any errors found.
"""
line = clean_lines.elided[linenum]
match = Match(r'.*\s+assert\(.*\).*', line)
if match:
if Match(r'.*\s+assert\((0|false)\).*', line):
error(filename, linenum, 'build/deprecated', 4,
'assert is deprecated, use UNREACHABLE instead')
else:
error(filename, linenum, 'build/deprecated', 4,
'assert is deprecated, use INVARIANT, PRECONDITION, CHECK_RETURN, etc. instead')



def GetLineWidth(line):
"""Determines the width of the line in column positions.

Expand Down Expand Up @@ -4853,6 +4874,7 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state,
CheckSpacingForFunctionCall(filename, clean_lines, linenum, error)
CheckCheck(filename, clean_lines, linenum, error)
CheckAltTokens(filename, clean_lines, linenum, error)
CheckAssert(filename, clean_lines, linenum, error)
classinfo = nesting_state.InnermostClass()
if classinfo:
CheckSectionSpacing(filename, clean_lines, classinfo, linenum, error)
Expand Down
22 changes: 22 additions & 0 deletions src/cbmc/cbmc_parse_options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Author: Daniel Kroening, [email protected]
#include <util/language.h>
#include <util/unicode.h>
#include <util/memory_info.h>
#include <util/invariant.h>

#include <ansi-c/c_preprocess.h>

Expand Down Expand Up @@ -104,6 +105,27 @@ void cbmc_parse_optionst::get_command_line_options(optionst &options)
exit(1); // should contemplate EX_USAGE from sysexits.h
}

// Test only; do not use for input validation
if(cmdline.isset("test-invariant-failure"))
{
// Have to catch this as the default handling of uncaught exceptions
// on windows appears to be silent termination.
try
{
INVARIANT(0, "Test invariant failure");
}
catch (const invariant_failedt &e)
{
std::cerr << e.what();
exit(0); // should contemplate EX_OK from sysexits.h
}
catch (...)
{
error() << "Unexpected exception type\n";
}
exit(1);
}

if(cmdline.isset("program-only"))
options.set_option("program-only", true);

Expand Down
1 change: 1 addition & 0 deletions src/cbmc/cbmc_parse_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ class optionst;
"(java-cp-include-files):" \
"(localize-faults)(localize-faults-method):" \
"(lazy-methods)" \
"(test-invariant-failure)" \
"(fixedbv)(floatbv)(all-claims)(all-properties)" // legacy, and will eventually disappear // NOLINT(whitespace/line_length)

class cbmc_parse_optionst:
Expand Down
32 changes: 16 additions & 16 deletions src/solvers/smt2/smt2_conv.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ Author: Daniel Kroening, [email protected]
#include <util/ieee_float.h>
#include <util/base_type.h>
#include <util/string2int.h>
#include <util/invariant.h>

#include <ansi-c/string_constant.h>

Expand All @@ -34,7 +35,6 @@ Author: Daniel Kroening, [email protected]

// Mark different kinds of error condition
// General
#define UNREACHABLE throw "Supposidly unreachable location reached"
#define PARSERERROR(S) throw S

// Error checking the expression type
Expand All @@ -45,7 +45,7 @@ Author: Daniel Kroening, [email protected]
#define UNEXPECTEDCASE(S) throw "Unexpected case: " S

// General todos
#define TODO(S) throw "TODO: " S
#define SMT2_TODO(S) throw "TODO: " S
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not a TODO or UNIMPLEMENTED?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted this commit to be as minimal as possible and to be clearly not changing functionality (the whole set should be very low risk / minimal impact). To replace with the new TODO I'd have to move the more complex error messages to use error(), which would mean a bigger change.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, this can be done separately.


void smt2_convt::print_assignment(std::ostream &out) const
{
Expand Down Expand Up @@ -952,7 +952,7 @@ void smt2_convt::convert_expr(const exprt &expr)
out << "))"; // mk-, let
}
else
TODO("bitnot for vectors");
SMT2_TODO("bitnot for vectors");
}
else
{
Expand Down Expand Up @@ -1017,7 +1017,7 @@ void smt2_convt::convert_expr(const exprt &expr)
out << "))"; // mk-, let
}
else
TODO("unary minus for vector");
SMT2_TODO("unary minus for vector");
}
else
{
Expand Down Expand Up @@ -1363,7 +1363,7 @@ void smt2_convt::convert_expr(const exprt &expr)
assert(expr.operands().size()==1);

out << "false"; // TODO
TODO("pointer_object_has_type not implemented");
SMT2_TODO("pointer_object_has_type not implemented");
}
else if(expr.id()==ID_string_constant)
{
Expand Down Expand Up @@ -1432,7 +1432,7 @@ void smt2_convt::convert_expr(const exprt &expr)
convert_expr(tmp);
out << ")) bin1)"; // bvlshr, extract, =
#endif
TODO("smt2: extractbits with non-constant index");
SMT2_TODO("smt2: extractbits with non-constant index");
}
}
else if(expr.id()==ID_replication)
Expand Down Expand Up @@ -1944,7 +1944,7 @@ void smt2_convt::convert_typecast(const typecast_exprt &expr)
// This conversion is non-trivial as it requires creating a
// new bit-vector variable and then asserting that it converts
// to the required floating-point number.
TODO("bit-wise floatbv to bv");
SMT2_TODO("bit-wise floatbv to bv");
}
else
{
Expand Down Expand Up @@ -2017,7 +2017,7 @@ void smt2_convt::convert_typecast(const typecast_exprt &expr)
out << "(_ bv" << i << " " << to_width << ")";
}
else
TODO("can't convert non-constant integer to bitvector");
SMT2_TODO("can't convert non-constant integer to bitvector");
}
else if(src_type.id()==ID_struct) // flatten a struct to a bit-vector
{
Expand Down Expand Up @@ -2207,7 +2207,7 @@ void smt2_convt::convert_typecast(const typecast_exprt &expr)
}
else if(dest_type.id()==ID_range)
{
TODO("range typecast");
SMT2_TODO("range typecast");
}
else if(dest_type.id()==ID_floatbv)
{
Expand Down Expand Up @@ -3031,11 +3031,11 @@ void smt2_convt::convert_floatbv_plus(const ieee_float_op_exprt &expr)
}
else if(type.id()==ID_complex)
{
TODO("+ for floatbv complex");
SMT2_TODO("+ for floatbv complex");
}
else if(type.id()==ID_vector)
{
TODO("+ for floatbv vector");
SMT2_TODO("+ for floatbv vector");
}
else
UNEXPECTEDCASE("unsupported type for +: "+type.id_string());
Expand Down Expand Up @@ -3093,7 +3093,7 @@ void smt2_convt::convert_minus(const minus_exprt &expr)
}
else if(expr.type().id()==ID_pointer)
{
TODO("pointer subtraction");
SMT2_TODO("pointer subtraction");
}
else if(expr.type().id()==ID_vector)
{
Expand Down Expand Up @@ -3527,7 +3527,7 @@ void smt2_convt::convert_with(const with_exprt &expr)
typecast_exprt index_tc(index, expr_type);

// TODO: SMT2-ify
TODO("SMT2-ify");
SMT2_TODO("SMT2-ify");
out << "(bvor ";
out << "(band ";

Expand Down Expand Up @@ -3565,7 +3565,7 @@ void smt2_convt::convert_update(const exprt &expr)
{
assert(expr.operands().size()==3);

TODO("smt2_convt::convert_update to be implemented");
SMT2_TODO("smt2_convt::convert_update to be implemented");
}

void smt2_convt::convert_index(const index_exprt &expr)
Expand Down Expand Up @@ -3651,7 +3651,7 @@ void smt2_convt::convert_index(const index_exprt &expr)
mp_integer index_int;
if(to_integer(expr.index(), index_int))
{
TODO("non-constant index on vectors");
SMT2_TODO("non-constant index on vectors");
}
else
{
Expand All @@ -3662,7 +3662,7 @@ void smt2_convt::convert_index(const index_exprt &expr)
}
else
{
TODO("index on vectors");
SMT2_TODO("index on vectors");
}
}
else
Expand Down
1 change: 1 addition & 0 deletions src/util/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ SRC = arith_tools.cpp \
guard.cpp \
identifier.cpp \
ieee_float.cpp \
invariant.cpp \
irep.cpp \
irep_hash.cpp \
irep_hash_container.cpp \
Expand Down
12 changes: 6 additions & 6 deletions src/util/expr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,13 @@ Author: Daniel Kroening, [email protected]
/// \file
/// Expression Representation

#include <cassert>

#include <stack>

#include "string2int.h"
#include "mp_arith.h"
#include "fixedbv.h"
#include "ieee_float.h"
#include "invariant.h"
#include "expr.h"
#include "rational.h"
#include "rational_tools.h"
Expand Down Expand Up @@ -203,15 +202,16 @@ void exprt::negate()
else
{
make_nil();
assert(false);
UNREACHABLE;
}
}
else
{
if(id()==ID_unary_minus)
{
exprt tmp;
assert(operands().size()==1);
DATA_INVARIANT(operands().size()==1,
"Unary minus must have one operand");
tmp.swap(op0());
swap(tmp);
}
Expand Down Expand Up @@ -245,7 +245,7 @@ bool exprt::is_zero() const
{
rationalt rat_value;
if(to_rational(*this, rat_value))
assert(false);
CHECK_RETURN(false);
Copy link
Member

Choose a reason for hiding this comment

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

It would actually be even more convenient if we could write CHECK_RETURN(to_rational(*this, rat_value));. However, we must guarantee that the implementation of CHECK_RETURN always evaluates its argument.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes... this patch set is intended to replace assert but there are a few other invariant handling patterns that could benefit from some kind of support. This is one of them, another is:

if (invariant) {
  debug() << "Actually helpful message with " << variables << " in it "
                << "maybe even " << e.pretty();
  assert(0); 
}

We should do something about these but I don't think we should do so in this patch set. I this will come out of trying to convert things to this way of doing things. We will get a better idea of what patterns there are and how they could be improved.

return rat_value.is_zero();
}
else if(type_id==ID_unsignedbv ||
Expand Down Expand Up @@ -291,7 +291,7 @@ bool exprt::is_one() const
{
rationalt rat_value;
if(to_rational(*this, rat_value))
assert(false);
CHECK_RETURN(false);
return rat_value.is_one();
}
else if(type_id==ID_unsignedbv || type_id==ID_signedbv)
Expand Down
14 changes: 7 additions & 7 deletions src/util/expr_util.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,14 @@ exprt make_binary(const exprt &expr)
const typet &type=expr.type();

exprt previous=operands.front();
assert(previous.type()==type);
PRECONDITION(previous.type()==type);

for(exprt::operandst::const_iterator
it=++operands.begin();
it!=operands.end();
++it)
{
assert(it->type()==type);
PRECONDITION(it->type()==type);

exprt tmp=expr;
tmp.operands().clear();
Expand All @@ -59,7 +59,7 @@ exprt make_binary(const exprt &expr)
with_exprt make_with_expr(const update_exprt &src)
{
const exprt::operandst &designator=src.designator();
assert(!designator.empty());
PRECONDITION(!designator.empty());

with_exprt result;
exprt *dest=&result;
Expand All @@ -78,7 +78,7 @@ with_exprt make_with_expr(const update_exprt &src)
// to_member_designator(*it).get_component_name();
}
else
assert(false);
UNREACHABLE;

*dest=tmp;
dest=&to_with_expr(*dest).new_value();
Expand Down Expand Up @@ -108,7 +108,7 @@ exprt is_not_zero(
src_type.id()==ID_floatbv?ID_ieee_float_notequal:ID_notequal;

exprt zero=from_integer(0, src_type);
assert(zero.is_not_nil());
CHECK_RETURN(zero.is_not_nil());

binary_exprt comparison(src, id, zero, bool_typet());
comparison.add_source_location()=src.source_location();
Expand Down Expand Up @@ -142,8 +142,8 @@ bool has_subexpr(const exprt &src, const irep_idt &id)

if_exprt lift_if(const exprt &src, std::size_t operand_number)
{
assert(operand_number<src.operands().size());
assert(src.operands()[operand_number].id()==ID_if);
PRECONDITION(operand_number<src.operands().size());
PRECONDITION(src.operands()[operand_number].id()==ID_if);

const if_exprt if_expr=to_if_expr(src.operands()[operand_number]);
const exprt true_case=if_expr.true_case();
Expand Down
Loading