Skip to content

Skip function parameters and auxiliary variables when checking assigns clauses and check loop-freeness #6527

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

Closed
Show file tree
Hide file tree
Changes from 1 commit
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: 0 additions & 6 deletions regression/contracts/assigns_enforce_04/test.desc
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,6 @@ main.c
--enforce-contract f1
^EXIT=0$
^SIGNAL=0$
^\[f1.\d+\] line \d+ Check that x2 is assignable: SUCCESS$
^\[f1.\d+\] line \d+ Check that y2 is assignable: SUCCESS$
^\[f1.\d+\] line \d+ Check that z2 is assignable: SUCCESS$
^\[f2.\d+\] line \d+ Check that x3 is assignable: SUCCESS$
^\[f2.\d+\] line \d+ Check that y3 is assignable: SUCCESS$
^\[f2.\d+\] line \d+ Check that z3 is assignable: SUCCESS$
^\[f3.\d+\] line \d+ Check that \*x3 is assignable: SUCCESS$
^\[f3.\d+\] line \d+ Check that \*y3 is assignable: SUCCESS$
^\[f3.\d+\] line \d+ Check that \*z3 is assignable: SUCCESS$
Expand Down
1 change: 0 additions & 1 deletion regression/contracts/assigns_enforce_05/test.desc
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ main.c
--enforce-contract f1 --enforce-contract f2 --enforce-contract f3
^EXIT=0$
^SIGNAL=0$
^\[f1.1\] line \d+ .*: SUCCESS$
^VERIFICATION SUCCESSFUL$
--
--
Expand Down
1 change: 0 additions & 1 deletion regression/contracts/assigns_enforce_18/test.desc
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ main.c
^\[bar.\d+\] line \d+ Check that \*b is assignable: SUCCESS$
^\[baz.\d+\] line \d+ Check that POINTER_OBJECT\(\(void \*\)c\) is assignable: FAILURE$
^\[baz.\d+\] line \d+ Check that \*a is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that a is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that yp is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that z is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that \*xp is assignable: SUCCESS$
Expand Down
3 changes: 0 additions & 3 deletions regression/contracts/assigns_enforce_21/test.desc
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@ main.c
main.c function bar
^\[bar.\d+\] line \d+ Check that \*y is assignable: SUCCESS$
^\[bar.\d+\] line \d+ Check that x is assignable: FAILURE$
^\[baz.\d+\] line \d+ Check that w is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that w is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that z is assignable: SUCCESS$
^VERIFICATION FAILED$
--
--
Expand Down
2 changes: 0 additions & 2 deletions regression/contracts/assigns_enforce_free_dead/test.desc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ main.c
^\[foo.\d+\] line \d+ Check that a is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that q is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that w is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that x is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that y is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that z is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that POINTER_OBJECT\(\(void \*\)z\) is assignable: SUCCESS$
Expand All @@ -25,7 +24,6 @@ main.c
^\[foo.\d+\] line \d+ Check that a is assignable: FAILURE$
^\[foo.\d+\] line \d+ Check that q is assignable: FAILURE$
^\[foo.\d+\] line \d+ Check that w is assignable: FAILURE$
^\[foo.\d+\] line \d+ Check that x is assignable: FAILURE$
^\[foo.\d+\] line \d+ Check that y is assignable: FAILURE$
^\[foo.\d+\] line \d+ Check that z is assignable: FAILURE$
^\[foo.\d+\] line \d+ Check that POINTER_OBJECT\(\(void \*\)z\) is assignable: FAILURE$
Expand Down
1 change: 0 additions & 1 deletion regression/contracts/assigns_enforce_structs_04/test.desc
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ main.c
^\[f1.\d+\] line \d+ Check that p->y is assignable: FAILURE$
^\[f2.\d+\] line \d+ Check that p->x is assignable: FAILURE$
^\[f3.\d+\] line \d+ Check that p->y is assignable: SUCCESS$
^\[f4.\d+\] line \d+ Check that p is assignable: SUCCESS$
^VERIFICATION FAILED$
--
--
Expand Down
13 changes: 0 additions & 13 deletions regression/contracts/assigns_enforce_subfunction_calls/test.desc
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,15 @@ main.c
--enforce-contract foo
^EXIT=10$
^SIGNAL=0$
^main.c function bar$
^\[bar.1\] line \d+ Check that x is assignable: SUCCESS$
^\[bar.2\] line \d+ Check that y is assignable: SUCCESS$
^\[bar.3\] line \d+ Check that x is assignable: SUCCESS$
^\[bar.4\] line \d+ Check that y is assignable: SUCCESS$
^main.c function baz$
^\[baz.1\] line \d+ Check that \*x is assignable: SUCCESS$
^\[baz.2\] line \d+ Check that \*x is assignable: SUCCESS$
^\[baz.3\] line \d+ Check that \*x is assignable: FAILURE$
^\[baz.4\] line \d+ Check that \*x is assignable: FAILURE$
^main.c function foo$
^\[foo.1\] line \d+ Check that x is assignable: SUCCESS$
^\[foo.2\] line \d+ Check that y is assignable: SUCCESS$
^\[foo.assertion.1\] line \d+ foo.x.set: SUCCESS$
^\[foo.3\] line \d+ Check that x is assignable: SUCCESS$
^\[foo.4\] line \d+ Check that y is assignable: SUCCESS$
^\[foo.assertion.2\] line \d+ foo.local.set: SUCCESS$
^\[foo.5\] line \d+ Check that x is assignable: SUCCESS$
^\[foo.6\] line \d+ Check that y is assignable: SUCCESS$
^\[foo.assertion.3\] line \d+ foo.y.set: SUCCESS$
^\[foo.7\] line \d+ Check that x is assignable: SUCCESS$
^\[foo.8\] line \d+ Check that y is assignable: SUCCESS$
^\[foo.assertion.4\] line \d+ foo.z.set: SUCCESS$
^VERIFICATION FAILED$
--
Expand Down
1 change: 0 additions & 1 deletion regression/contracts/assigns_function_pointer/test.desc
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ main.c
--enforce-contract bar
^EXIT=0$
^SIGNAL=0$
^\[bar.1\] line \d+ Check that fun\_ptr is assignable: SUCCESS$
^\[main.assertion.\d+\] line \d+ assertion x \=\= 0: SUCCESS$
^\[main.assertion.\d+\] line \d+ assertion x \=\= 1: SUCCESS$
^VERIFICATION SUCCESSFUL$
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,9 @@ main.c
--enforce-contract foo1 --enforce-contract foo2 --enforce-contract foo3 --enforce-contract foo4 --enforce-contract foo5 --enforce-contract foo6 --enforce-contract foo7 --enforce-contract foo8 --enforce-contract foo9 --enforce-contract foo10 _ --pointer-primitive-check
^EXIT=0$
^SIGNAL=0$
^\[foo1.\d+\] line \d+ Check that a is assignable: SUCCESS$
^\[foo10.\d+\] line \d+ Check that buffer->len is assignable: SUCCESS$
^\[foo10.\d+\] line \d+ Check that buffer->aux\.allocated is assignable: SUCCESS$
^\[foo2.\d+\] line \d+ Check that b is assignable: SUCCESS$
^\[foo3.\d+\] line \d+ Check that b is assignable: SUCCESS$
^\[foo3.\d+\] line \d+ Check that y is assignable: SUCCESS$
^\[foo4.\d+\] line \d+ Check that b is assignable: SUCCESS$
^\[foo4.\d+\] line \d+ Check that \*c is assignable: SUCCESS$
^\[foo4.\d+\] line \d+ Check that \*x is assignable: SUCCESS$
^\[foo5.\d+\] line \d+ Check that buffer.data is assignable: SUCCESS$
Expand All @@ -28,7 +24,6 @@ main.c
^\[foo8.\d+\] line \d+ Check that array\[\(.* int\)7\] is assignable: SUCCESS$
^\[foo8.\d+\] line \d+ Check that array\[\(.* int\)8\] is assignable: SUCCESS$
^\[foo8.\d+\] line \d+ Check that array\[\(.* int\)9\] is assignable: SUCCESS$
^\[foo9.\d+\] line \d+ Check that array is assignable: SUCCESS$
^VERIFICATION SUCCESSFUL$
--
Checks whether CBMC parses correctly all standard cases for assigns clauses.
2 changes: 0 additions & 2 deletions regression/contracts/assigns_validity_pointer_02/test.desc
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ main.c
^\[bar.\d+\] line \d+ Check that \*x is assignable: SUCCESS$
^\[bar.\d+\] line \d+ Check that \*y is assignable: SUCCESS$
^\[baz.\d+\] line \d+ Check that \*z is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that x is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that y is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that \*x is assignable: SUCCESS$
^VERIFICATION SUCCESSFUL$
--
Expand Down
2 changes: 1 addition & 1 deletion regression/contracts/history-pointer-enforce-10/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ void bar(struct pair *p) __CPROVER_assigns(p->y)
p->y = (p->y + 5);
}

void baz(struct pair p) __CPROVER_assigns(p)
void baz(struct pair p) __CPROVER_assigns()
__CPROVER_ensures(p == __CPROVER_old(p))
{
struct pair pp = p;
Expand Down
4 changes: 2 additions & 2 deletions regression/contracts/history-pointer-enforce-10/test.desc
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ main.c
^\[postcondition.\d+\] file main.c line \d+ Check ensures clause: SUCCESS$
^\[postcondition.\d+\] file main.c line \d+ Check ensures clause: SUCCESS$
^\[bar.\d+\] line \d+ Check that p->y is assignable: SUCCESS$
^\[baz.\d+\] line \d+ Check that p is assignable: SUCCESS$
^\[baz.\d+\] line \d+ Check that p is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that \*p->y is assignable: SUCCESS$
^\[foo.\d+\] line \d+ Check that z is assignable: SUCCESS$
^\[main.assertion.\d+\] line \d+ assertion \*\(p->y\) == 7: SUCCESS$
Expand All @@ -19,3 +17,5 @@ main.c
This test checks that history variables are supported for structs, symbols, and
struct members. By using the --enforce-contract flag, the postcondition
(with history variable) is asserted. In this case, this assertion should pass.
Note: A function is always authorized to assign the variables that store
its arguments, there is no need to mention them in the assigns clause.
2 changes: 1 addition & 1 deletion regression/contracts/test_aliasing_ensure_indirect/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#include <stdbool.h>
#include <stdlib.h>

void bar(int *z) __CPROVER_assigns(z)
void bar(int *z) __CPROVER_assigns()
__CPROVER_ensures(__CPROVER_is_fresh(z, sizeof(int)))
{
z = malloc(sizeof(*z));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ main.c
^EXIT=0$
^SIGNAL=0$
\[postcondition.\d+\] file main.c line \d+ Check ensures clause: SUCCESS
\[bar.\d+\] line \d+ Check that z is assignable: SUCCESS
\[foo.\d+\] line \d+ Check that \*x is assignable: SUCCESS
\[main.assertion.\d+\] line \d+ assertion \!\(n \< 4\): SUCCESS
^VERIFICATION SUCCESSFUL$
Expand Down
21 changes: 18 additions & 3 deletions src/goto-instrument/contracts/contracts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,13 @@ void code_contractst::check_apply_loop_contracts(

// Perform write set instrumentation on the entire loop.
check_frame_conditions(
function_name, goto_function.body, loop_head, loop_end, loop_assigns);
function_name,
goto_function.body,
loop_head,
loop_end,
loop_assigns,
// do not skip checks on function parameter assignments
false);
Comment on lines +317 to +318
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we see a need to comment on the Boolean value then it may be better to introduce an enum that has informative values. If we always pass constant values, then it might be even better to use a template parameter.


havoc_assigns_targetst havoc_gen(to_havoc, ns);
havoc_gen.append_full_havoc_code(
Expand Down Expand Up @@ -1171,7 +1177,9 @@ bool code_contractst::check_frame_conditions_function(const irep_idt &function)
function_obj->second.body,
instruction_it,
function_obj->second.body.instructions.end(),
assigns);
assigns,
// skip checks on function parameter assignments
true);

return false;
}
Expand All @@ -1181,14 +1189,21 @@ void code_contractst::check_frame_conditions(
goto_programt &body,
goto_programt::targett instruction_it,
const goto_programt::targett &instruction_end,
assigns_clauset &assigns)
assigns_clauset &assigns,
bool skip_parameter_assigns)
{
for(; instruction_it != instruction_end; ++instruction_it)
{
const auto &pragmas = instruction_it->source_location().get_pragmas();
if(pragmas.find(CONTRACT_PRAGMA_DISABLE_ASSIGNS_CHECK) != pragmas.end())
continue;

if(skip_parameter_assigns && is_parameter_assign(instruction_it, ns))
continue;

if(is_auxiliary_decl_dead_or_assign(instruction_it, ns))
continue;

// Do not instrument this instruction again in the future,
// since we are going to instrument it now below.
add_pragma_disable_assigns_check(*instruction_it);
Expand Down
3 changes: 2 additions & 1 deletion src/goto-instrument/contracts/contracts.h
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@ class code_contractst
goto_programt &,
goto_programt::targett,
const goto_programt::targett &,
assigns_clauset &);
assigns_clauset &,
bool skip_parameter_assigns);

/// Inserts an assertion into the goto program to ensure that
/// an expression is within the assignable memory frame.
Expand Down
47 changes: 47 additions & 0 deletions src/goto-instrument/contracts/utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -163,3 +163,50 @@ void disable_pointer_checks(source_locationt &source_location)
source_location.add_pragma("disable:pointer-primitive-check");
source_location.add_pragma("disable:pointer-overflow-check");
}

bool is_parameter_assign(
const goto_programt::targett &instruction_it,
namespacet &ns)
{
optionalt<symbol_exprt> sym = {};

// extract symbol
if(instruction_it->is_assign())
{
const auto &lhs = instruction_it->assign_lhs();
if(can_cast_expr<symbol_exprt>(lhs))
sym = to_symbol_expr(lhs);
}

// check condition
if(sym.has_value())
return ns.lookup(sym.value().get_identifier()).is_parameter;

return false;
Comment on lines +175 to +189
Copy link
Collaborator

@feliperodri feliperodri Dec 14, 2021

Choose a reason for hiding this comment

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

Suggested change
optionalt<symbol_exprt> sym = {};
// extract symbol
if(instruction_it->is_assign())
{
const auto &lhs = instruction_it->assign_lhs();
if(can_cast_expr<symbol_exprt>(lhs))
sym = to_symbol_expr(lhs);
}
// check condition
if(sym.has_value())
return ns.lookup(sym.value().get_identifier()).is_parameter;
return false;
if(instruction_it->is_assign() && can_cast_expr<symbol_exprt>(instruction_it->assign_lhs()))
{
// Extract symbol from lhs
auto lhs_sym = to_symbol_expr(instruction_it->assign_lhs());
// Check whether is parameter
if(lhs_sym.has_value())
return ns.lookup(lhs_sym.value().get_identifier()).is_parameter;
}
return false;

Copy link
Collaborator

Choose a reason for hiding this comment

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

On top of what Felipe suggested: there's no need for the optionalt anymore.

}

bool is_auxiliary_decl_dead_or_assign(
const goto_programt::targett &instruction_it,
namespacet &ns)
{
optionalt<symbol_exprt> sym = {};

// extract symbol
if(instruction_it->is_decl())
sym = instruction_it->decl_symbol();
else if(instruction_it->is_dead())
sym = instruction_it->dead_symbol();
else if(instruction_it->is_assign())
{
const auto &lhs = instruction_it->assign_lhs();
if(can_cast_expr<symbol_exprt>(lhs))
sym = to_symbol_expr(lhs);
}

// check condition
if(sym.has_value())
return ns.lookup(sym.value().get_identifier()).is_auxiliary;

return false;
}
Comment on lines +192 to +215
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bool is_auxiliary_decl_dead_or_assign(
const goto_programt::targett &instruction_it,
namespacet &ns)
{
optionalt<symbol_exprt> sym = {};
// extract symbol
if(instruction_it->is_decl())
sym = instruction_it->decl_symbol();
else if(instruction_it->is_dead())
sym = instruction_it->dead_symbol();
else if(instruction_it->is_assign())
{
const auto &lhs = instruction_it->assign_lhs();
if(can_cast_expr<symbol_exprt>(lhs))
sym = to_symbol_expr(lhs);
}
// check condition
if(sym.has_value())
return ns.lookup(sym.value().get_identifier()).is_auxiliary;
return false;
}
bool is_auxiliary_decl_dead_or_assign(
const goto_programt::targett &instruction_it,
namespacet &ns)
{
// Extract symbol
optionalt<symbol_exprt> sym = {};
if(instruction_it->is_assign() && can_cast_expr<symbol_exprt>(instruction_it->assign_lhs()))
sym = to_symbol_expr(instruction_it->assign_lhs());
else if(instruction_it->is_decl())
sym = instruction_it->decl_symbol();
else if(instruction_it->is_dead())
sym = instruction_it->dead_symbol();
else
return false;
// Check whether is auxiliary
return ns.lookup(sym.value().get_identifier()).is_auxiliary;
}


16 changes: 14 additions & 2 deletions src/goto-instrument/contracts/utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -112,17 +112,29 @@ void insert_before_swap_and_advance(
/// \param mode: The mode for the new symbol, e.g. ID_C, ID_java.
/// \param symtab: The symbol table to which the new symbol is to be added.
/// \param suffix: Suffix to use to generate the unique name
/// \param is_auxiliary: Do not print symbol in traces if true (default = false)
/// \param is_auxiliary: Do not print symbol in traces if true (default = true)
/// \return The new symbolt object.
const symbolt &new_tmp_symbol(
const typet &type,
const source_locationt &location,
const irep_idt &mode,
symbol_table_baset &symtab,
std::string suffix = "tmp_cc",
bool is_auxiliary = false);
bool is_auxiliary = true);
Comment on lines +117 to +125
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this being changed as part of this PR?


/// Add disable pragmas for all pointer checks on the given location
void disable_pointer_checks(source_locationt &source_location);

/// Returns true iff the instruction is a `DECL x`, `DEAD x`,
/// or `ASSIGN x := expr` where `x` has the `is_parameter` flag set.
bool is_parameter_assign(
const goto_programt::targett &instruction_it,
namespacet &ns);

/// Returns true iff the instruction is a `DECL x`, `DEAD x`,
/// or `ASSIGN x := expr` where `x` has the `is_auxiliary` flag set.
bool is_auxiliary_decl_dead_or_assign(
const goto_programt::targett &instruction_it,
namespacet &ns);

#endif // CPROVER_GOTO_INSTRUMENT_CONTRACTS_UTILS_H