Skip to content

Add an --assert-ensures check #4976

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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
18 changes: 18 additions & 0 deletions regression/contracts/assert_ensures_01/main.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// This tests whether the flag --assert-ensures adds the assert of the
// ensures statement after the call

#include <assert.h>

int min(int a, int b) __CPROVER_ensures(
__CPROVER_return_value <= a && __CPROVER_return_value <= b &&
(__CPROVER_return_value == a || __CPROVER_return_value == b))
{
return 100;
}

int main()
{
int x, y, z;
z = min(x, y);
return z;
}
7 changes: 7 additions & 0 deletions regression/contracts/assert_ensures_01/test.desc
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
CORE
main.c
--assert-ensures min
^EXIT=10$
^SIGNAL=0$
^VERIFICATION FAILED$
Copy link
Contributor

Choose a reason for hiding this comment

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

Could fail for other reasons -- suggest checking the particular failure line you're expecting, and/or comparing against a companion test that doesn't use --assert-ensures and for which verification succeeds

--
21 changes: 21 additions & 0 deletions regression/contracts/assert_ensures_02/main.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// This tests whether --assert-ensures correctly handles function
// calls in the contract

#include <assert.h>

int is_min(int min, int a, int b)
{
return min <= a && min <= b && (min == a || min == b);
}

int min(int a, int b) __CPROVER_ensures(is_min(__CPROVER_return_value, a, b))
{
return 100;
}

int main()
{
int x, y, z;
z = min(x, y);
return z;
}
7 changes: 7 additions & 0 deletions regression/contracts/assert_ensures_02/test.desc
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
CORE
main.c
--assert-ensures min
Copy link
Contributor

Choose a reason for hiding this comment

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

Same goes for this test

^EXIT=10$
^SIGNAL=0$
^VERIFICATION FAILED$
--
123 changes: 110 additions & 13 deletions src/goto-instrument/code_contracts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ Date: February 2016

#include <util/expr_util.h>
#include <util/fresh_symbol.h>
#include <util/message.h>
#include <util/replace_symbol.h>

#include <goto-programs/goto_convert.h>
#include <goto-programs/remove_skip.h>

#include <analyses/local_may_alias.h>
Expand All @@ -30,12 +32,18 @@ class code_contractst
public:
code_contractst(
symbol_tablet &_symbol_table,
goto_functionst &_goto_functions):
ns(_symbol_table),
goto_functionst &_goto_functions,
std::list<std::string> _functions_to_assert_ensures)
Copy link
Contributor

Choose a reason for hiding this comment

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

Take by const ref, or std::move into the local variable

: ns(_symbol_table),
symbol_table(_symbol_table),
goto_functions(_goto_functions),
functions_to_assert_ensures(_functions_to_assert_ensures),
temporary_counter(0)
{
if(functions_to_assert_ensures.empty())
mode_assert_ensures = false;
else
mode_assert_ensures = true;
}

void operator()();
Expand All @@ -44,9 +52,12 @@ class code_contractst
namespacet ns;
symbol_tablet &symbol_table;
goto_functionst &goto_functions;
std::list<std::string> functions_to_assert_ensures;

unsigned temporary_counter;

bool mode_assert_ensures;

std::unordered_set<irep_idt> summarized;

void code_contracts(goto_functionst::goto_functiont &goto_function);
Expand All @@ -55,6 +66,12 @@ class code_contractst
goto_programt &goto_program,
goto_programt::targett target);

void
assert_ensures_after_calls(goto_functionst::goto_functiont &goto_function);

void
assert_ensures(goto_programt &goto_program, goto_programt::targett target);
Copy link
Contributor

Choose a reason for hiding this comment

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

_after_call for consistency with function above?


void add_contract_check(
const irep_idt &function,
goto_programt &dest);
Expand Down Expand Up @@ -242,6 +259,70 @@ void code_contractst::code_contracts(
apply_contract(goto_function.body, it);
}

void code_contractst::assert_ensures(
goto_programt &goto_program,
goto_programt::targett target)
{
const code_function_callt &call = target->get_function_call();

// we don't handle function pointers
if(call.function().id() != ID_symbol)
return;

const irep_idt &function = to_symbol_expr(call.function()).get_identifier();
const symbolt &f_sym = ns.lookup(function);
const code_typet &type = to_code_type(f_sym.type);

exprt ensures = static_cast<const exprt &>(type.find(ID_C_spec_ensures));

// is there a contract?
if(ensures.is_nil())
return;

// replace formal parameters by arguments, replace return
replace_symbolt replace;

// TODO: return value could be nil
Copy link
Contributor

Choose a reason for hiding this comment

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

? As it just not set at all? I don't think that's valid

if(type.return_type() != empty_typet())
{
symbol_exprt ret_val(CPROVER_PREFIX "return_value", call.lhs().type());
replace.insert(ret_val, call.lhs());
}

// formal parameters
code_function_callt::argumentst::const_iterator a_it =
call.arguments().begin();
for(code_typet::parameterst::const_iterator p_it = type.parameters().begin();
p_it != type.parameters().end() && a_it != call.arguments().end();
Copy link
Contributor

Choose a reason for hiding this comment

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

arguments ending before parameters would be a static error AFAIK -- should probably just INVARIANT that you have at least enough arguments (and decide what you want to do for varargs?)

++p_it, ++a_it)
if(!p_it->get_identifier().empty())
{
symbol_exprt p(p_it->get_identifier(), p_it->type());
replace.insert(p, *a_it);
}

replace(ensures);

// assert the ensures part of the contract after the call
++target;
code_assertt a(ensures);
goto_programt new_goto_program;
null_message_handlert message_handler;
// We first need to convert the expression to a goto_program
goto_convert(a, symbol_table, new_goto_program, message_handler, ID_C);

goto_program.insert_before_swap(target, new_goto_program);
}

void code_contractst::assert_ensures_after_calls(
goto_functionst::goto_functiont &goto_function)
{
// look at all function calls
Forall_goto_program_instructions(it, goto_function.body)
if(it->is_function_call())
assert_ensures(goto_function.body, it);
}

const symbolt &code_contractst::new_tmp_symbol(
const typet &type,
const source_locationt &source_location,
Expand Down Expand Up @@ -372,23 +453,39 @@ void code_contractst::add_contract_check(

void code_contractst::operator()()
{
Forall_goto_functions(it, goto_functions)
code_contracts(it->second);

goto_functionst::function_mapt::iterator i_it=
goto_functions.function_map.find(INITIALIZE_FUNCTION);
assert(i_it!=goto_functions.function_map.end());
if(mode_assert_ensures)
{
Forall_goto_functions(it, goto_functions)
assert_ensures_after_calls(it->second);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this share any code with code_contracts? If so perhaps it should be a different pass rather than two modes of the same one

}
else
{
Forall_goto_functions(it, goto_functions)
code_contracts(it->second);

for(const auto &contract : summarized)
add_contract_check(contract, i_it->second.body);
goto_functionst::function_mapt::iterator i_it =
goto_functions.function_map.find(INITIALIZE_FUNCTION);
INVARIANT(
i_it != goto_functions.function_map.end(),
"There must exist an INITIALIZE_FUNCTION");

// remove skips
remove_skip(i_it->second.body);
for(const auto &contract : summarized)
add_contract_check(contract, i_it->second.body);

// remove skips
remove_skip(i_it->second.body);
}
goto_functions.update();
}

void assert_ensures(goto_modelt &goto_model, std::list<std::string> functions)
{
code_contractst(
goto_model.symbol_table, goto_model.goto_functions, functions)();
}

void code_contracts(goto_modelt &goto_model)
{
code_contractst(goto_model.symbol_table, goto_model.goto_functions)();
std::list<std::string> empty;
code_contractst(goto_model.symbol_table, goto_model.goto_functions, empty)();
}
5 changes: 5 additions & 0 deletions src/goto-instrument/code_contracts.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,13 @@ Date: February 2016
#ifndef CPROVER_GOTO_INSTRUMENT_CODE_CONTRACTS_H
#define CPROVER_GOTO_INSTRUMENT_CODE_CONTRACTS_H

#include <list>
#include <string>

class goto_modelt;

void code_contracts(goto_modelt &);

void assert_ensures(goto_modelt &goto_model, std::list<std::string> functions);

#endif // CPROVER_GOTO_INSTRUMENT_CODE_CONTRACTS_H
17 changes: 17 additions & 0 deletions src/goto-instrument/goto_instrument_parse_options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1088,6 +1088,22 @@ void goto_instrument_parse_optionst::instrument_goto_program()
code_contracts(goto_model);
}

// assert the ensures part of the contract for the given functions
if(cmdline.isset("assert-ensures"))
{
log.status() << "Asserting code contract" << messaget::eom;
std::list<std::string> functions_to_assert_ensures =
cmdline.get_values("assert-ensures");
if(functions_to_assert_ensures.empty())
{
log.error()
<< "At least one function name has to be given with --assert-ensures"
<< messaget::eom;
throw 0;
}
assert_ensures(goto_model, functions_to_assert_ensures);
}

// replace function pointers, if explicitly requested
if(cmdline.isset("remove-function-pointers"))
{
Expand Down Expand Up @@ -1643,6 +1659,7 @@ void goto_instrument_parse_optionst::help()
" --nondet-static-exclude e same as nondet-static except for the variable e\n" //NOLINT(*)
" (use multiple times if required)\n"
" --check-invariant function instruments invariant checking function\n"
" --assert-ensures function checks that function satisfies its contract ensures after each call\n" // NOLINT(*)
" --remove-pointers converts pointer arithmetic to base+offset expressions\n" // NOLINT(*)
Copy link
Member

Choose a reason for hiding this comment

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

What does "after each call" mean?
So this is really checking the post-conditions in particular calling contexts?

Copy link
Contributor Author

@angelhof angelhof Aug 6, 2019

Choose a reason for hiding this comment

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

@kroening

Seems sensible overall -- perhaps call it --check-postconditions?

I agree that this is a much better name.

What does "after each call" mean?
So this is really checking the post-conditions in particular calling contexts?

It asserts the postcondition after all calls to the function. So it checks the postcondition if the function is reachable, and is called with its name (i.e. it wouldn't work for function pointers). If this is a problem, I could re-implement this as a check before each return point in the function body. I didn't do that initially for simplicity. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe --check-ensures (because they are written in the source code as __CPROVER_ensures) or --check-contracts ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about --check-contract-postconditions or --check-contract-ensures?

Copy link
Contributor

Choose a reason for hiding this comment

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

My personal opinion is that I like these --check-blah options to mirror whatever is in the source code - so I'd lean towards --check-ensures or --check-contract-ensures - others may have different opinions :-)

" --splice-call caller,callee prepends a call to callee in the body of caller\n" // NOLINT(*)
" --undefined-function-is-assume-false\n"
Expand Down
3 changes: 2 additions & 1 deletion src/goto-instrument/goto_instrument_parse_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ Author: Daniel Kroening, [email protected]
"(interpreter)(show-reaching-definitions)" \
"(list-symbols)(list-undefined-functions)" \
"(z3)(add-library)(show-dependence-graph)" \
"(horn)(skip-loops):(apply-code-contracts)(model-argc-argv):" \
"(horn)(skip-loops):(apply-code-contracts)" \
"(assert-ensures):(model-argc-argv):" \
"(show-threaded)(list-calls-args)" \
"(undefined-function-is-assume-false)" \
"(remove-function-body):"\
Expand Down