Skip to content

Disable unnecessary pointer checks, enable necessary pointer checks i… #6459

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 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: 4 additions & 2 deletions regression/contracts/assigns_enforce_free_dead/test.desc
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ main.c
^\[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$
^.*tmp_cc\$\d+.*: FAILURE$
^.*__car_valid.*: FAILURE$
^.*__car_lb.*: FAILURE$
^.*__car_ub.*: FAILURE$
Comment on lines +32 to +34
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: I'd prefer even more readable names for users: __car_valid, __car_lower_bound, and __car_upper_bound. However, the fact that these variables exist should be transparent to users. Why would this variables appear as failures? I guess you want to make sure here there are no memory safety errors in any operation using these variables, is that correct?

--
Checks that invalidated CARs are correctly tracked on `free` and `DEAD`.

Expand All @@ -40,4 +42,4 @@ We also check (using positive regex matches above) that
`**p` should be assignable in one case and should not be assignable in the other.

Finally, we check that there should be no "internal" assertion failures
on `tmp_cc` variables used to track CARs.
on `__car_valid`, `__car_ub`, `__car_lb` variables used to track CARs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Changes in this file should be folded into the commit introducing the new names.

12 changes: 8 additions & 4 deletions src/goto-instrument/contracts/assigns.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,16 @@ static const slicet normalize_to_slice(const exprt &expr, const namespacet &ns)
}

const symbolt assigns_clauset::conditional_address_ranget::generate_new_symbol(
const std::string &prefix,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Random place for comment, and primarily for future reference: I would like to see commits like this be a pull request of their own. Changing the names of CAR instrumentation variables really has nothing to do with disabling selected checks.

const typet &type,
const source_locationt &location) const
{
return new_tmp_symbol(
type,
location,
parent.symbol_table.lookup_ref(parent.function_name).mode,
parent.symbol_table);
parent.symbol_table,
prefix);
}

assigns_clauset::conditional_address_ranget::conditional_address_ranget(
Expand All @@ -70,11 +72,13 @@ assigns_clauset::conditional_address_ranget::conditional_address_ranget(
parent(parent),
slice(normalize_to_slice(expr, parent.ns)),
validity_condition_var(
generate_new_symbol(bool_typet(), location).symbol_expr()),
generate_new_symbol("__car_valid", bool_typet(), location).symbol_expr()),
lower_bound_address_var(
generate_new_symbol(slice.first.type(), location).symbol_expr()),
generate_new_symbol("__car_lb", slice.first.type(), location)
.symbol_expr()),
upper_bound_address_var(
generate_new_symbol(slice.first.type(), location).symbol_expr())
generate_new_symbol("__car_ub", slice.first.type(), location)
.symbol_expr())
{
}

Expand Down
6 changes: 4 additions & 2 deletions src/goto-instrument/contracts/assigns.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,10 @@ class assigns_clauset
const exprt
generate_unsafe_inclusion_check(const conditional_address_ranget &) const;

const symbolt
generate_new_symbol(const typet &, const source_locationt &) const;
const symbolt generate_new_symbol(
const std::string &prefix,
const typet &,
const source_locationt &) const;
Comment on lines +64 to +66
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency, add the name of the remaining parameters or remove prefix.


friend class assigns_clauset;
};
Expand Down
5 changes: 3 additions & 2 deletions src/goto-instrument/contracts/contracts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -936,9 +936,10 @@ void code_contractst::instrument_call_statement(
const auto object_validity_var_addr =
new_tmp_symbol(
pointer_type(bool_typet{}),
instruction_it->source_location(),
location_no_checks,
symbol_table.lookup_ref(function).mode,
symbol_table)
symbol_table,
"__car_valid")
.symbol_expr();
write_set_validity_addrs.insert(object_validity_var_addr);

Expand Down
13 changes: 9 additions & 4 deletions src/goto-instrument/contracts/utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,18 @@ const symbolt &new_tmp_symbol(
const typet &type,
const source_locationt &location,
const irep_idt &mode,
symbol_table_baset &symtab)
symbol_table_baset &symtab,
std::string suffix,
Comment on lines +145 to +146
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
symbol_table_baset &symtab,
std::string suffix,
symbol_table_baset &symtab,
std::string &suffix,

bool is_auxiliary)
{
return get_fresh_aux_symbol(
symbolt &new_symbol = get_fresh_aux_symbol(
type,
id2string(location.get_function()) + "::tmp_cc",
"tmp_cc",
id2string(location.get_function()) + "::",
suffix,
location,
mode,
symtab);
new_symbol.is_auxiliary = is_auxiliary;
return new_symbol;
}
}
8 changes: 6 additions & 2 deletions src/goto-instrument/contracts/utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -105,17 +105,21 @@ void insert_before_swap_and_advance(
goto_programt::targett &target,
goto_programt &payload);

/// \brief Adds a new temporary symbol to the symbol table.
/// \brief Adds a fresh and uniquely named symbol to the symbol table.
///
/// \param type: The type of the new symbol.
/// \param location: The source location for the new symbol.
/// \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)
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
/// \param suffix: Suffix to use to generate the unique name
/// \param is_auxiliary: Do not print symbol in traces if true (default = false)
/// \param suffix: Suffix to use to generate the unique name.
/// \param is_auxiliary: Do not print symbol in traces if true (default = false).

/// \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);
symbol_table_baset &symtab,
std::string suffix = "tmp_cc",
bool is_auxiliary = false);

#endif // CPROVER_GOTO_INSTRUMENT_CONTRACTS_UTILS_H