Skip to content

Cover failed assertions #5636

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

2 changes: 1 addition & 1 deletion jbmc/src/jdiff/jdiff_parse_options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ void jdiff_parse_optionst::help()
"\n"
"Program instrumentation options:\n"
HELP_GOTO_CHECK
" --cover CC create test-suite with coverage criterion CC\n" // NOLINT(*)
HELP_COVER
"Java Bytecode frontend options:\n"
JAVA_BYTECODE_LANGUAGE_OPTIONS_HELP
"Other options:\n"
Expand Down
4 changes: 3 additions & 1 deletion jbmc/src/jdiff/jdiff_parse_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ Author: Peter Schrammel
#include <goto-programs/show_goto_functions.h>
#include <goto-programs/show_properties.h>

#include <goto-instrument/cover.h>

class goto_modelt;

// clang-format off
Expand All @@ -31,7 +33,7 @@ class goto_modelt;
OPT_SHOW_GOTO_FUNCTIONS \
OPT_SHOW_PROPERTIES \
OPT_GOTO_CHECK \
"(cover):" \
OPT_COVER \
"(verbosity):(version)" \
"(no-lazy-methods)" /* should go away */ \
"(no-refine-strings)" /* should go away */ \
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
CORE
test.c
--cover location --pointer-check --malloc-may-fail --malloc-fail-null
\[main.coverage.4\] file test.c line \d+ function main block 4 \(lines test.c:main:12,13\): SATISFIED
\[main.coverage.3\] file test.c line \d+ function main block 3 \(lines test.c:main:10\): FAILED
\[main.coverage.2\] file test.c line \d+ function main block 2 \(lines test.c:main:4,5,7,9\): SATISFIED
\[main.coverage.1\] file test.c line \d+ function main block 1 \(lines test.c:main:4\): SATISFIED
^EXIT=0$
^SIGNAL=0$
--
^warning: ignoring
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ It would be nice to have a comment in each of .desc files explaining what the test is intended to test. Even something pretty short could make this a while lot easier to maintain.

14 changes: 14 additions & 0 deletions regression/cbmc/cover-failed-assertions/test.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#include <stdlib.h>

int main()
{
int *ptr = malloc(sizeof(*ptr));
int a;

a = *ptr;

if(ptr == NULL)
a = 1;

return 0;
}
11 changes: 11 additions & 0 deletions regression/cbmc/cover-failed-assertions/test.desc
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
CORE
test.c
--cover location --cover-failed-assertions --pointer-check --malloc-may-fail --malloc-fail-null
\[main.coverage.4\] file test.c line \d+ function main block 4 \(lines test.c:main:12,13\): SATISFIED
\[main.coverage.3\] file test.c line \d+ function main block 3 \(lines test.c:main:10\): SATISFIED
\[main.coverage.2\] file test.c line \d+ function main block 2 \(lines test.c:main:4,5,7,9\): SATISFIED
\[main.coverage.1\] file test.c line \d+ function main block 1 \(lines test.c:main:4\): SATISFIED
^EXIT=0$
^SIGNAL=0$
--
^warning: ignoring
14 changes: 8 additions & 6 deletions src/cbmc/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,15 @@ by other mechanism may categorise their properties differently.

### Coverage mode

There is one further BMC mode that differs more fundamentally: when `--cover` is
passed, assertions in the program text are converted into assumptions (these
There is one further BMC mode that differs more fundamentally: when `--cover`
is passed, assertions in the program text are converted into assumptions (these
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't want to open a massive can of worms here but ... why is this the default? Is this actually useful? Given that there are options for --no-assertions, --no-assumptions and --assert-to-assume (whether they work or even do anything at all ... is an open question) could we make the transformation of assertions and assumptions orthogonal to --cover. It feels like this should be possible and even desirable...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martin-cs I think this is the default because whoever wrote this had the expectation that programs should stop at failed assertions. I suppose we could change it, but I’m always a bit wary about changing defaults.

Copy link
Contributor Author

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue Dec 11, 2020

Choose a reason for hiding this comment

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

@martin-cs FWIW because cover treats each failed goal as something relating to coverage right now we have to do something with asserts. That said, I think the alternative (just making cover cleverer about how it counts coverage) would be more desirable.

I'd consider that as something we can take a look at in terms of future work though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, if you are doing a naive count then, I see the need to do something with pre-existing assertions or otherwise true assertions (generally a good thing) will count against your coverage. Smarter counting would certainly be one way to address this.

It feels to me that making the "doing something about asserts" independent might be a desirable option as well. I can see use-cases for converting to SKIP, to ASSUME and leaving as they are.

Then again, I am not a user of this code at the moment and do not have time to work on it so my views maybe not so important.

must hold for control flow to proceed past them, but are not goals for the
equation solver), while new `assert(false)` statements are added throughout the
source program representing coverage goals. The equation solving process then
proceeds the same as in all-properties mode. Coverage solving is implemented by
`bmc_covert`, but is structurally practically identical to
equation solver; In cases where this behaviour is undesirable you can pass the
`--cover-failed-assertions` which makes coverage checking continue even for
Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW shouldn’t this file be under doc rather than src? I don’t really understand our documentation structure...

Copy link
Member

Choose a reason for hiding this comment

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

The READMEs which are in each module are pulled into here: http://cprover.diffblue.com/folder-walkthrough.html (click on cbmc, for example).
We decided to keep them in the source folders so that they are as close as possible to the code.

paths where assertions fail), while new `assert(false)` statements are added
throughout the source program representing coverage goals. The equation solving
process then proceeds the same as in all-properties mode. Coverage solving is
implemented by `bmc_covert`, but is structurally practically identical to
`bmc_all_propertiest`-- only the reporting differs (goals are called "covered"
rather than "failed").

Expand Down
2 changes: 1 addition & 1 deletion src/cbmc/cbmc_parse_options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1118,7 +1118,7 @@ void cbmc_parse_optionst::help()
" --no-assertions ignore user assertions\n"
" --no-assumptions ignore user assumptions\n"
" --error-label label check that label is unreachable\n"
" --cover CC create test-suite with coverage criterion CC\n" // NOLINT(*)
HELP_COVER
" --mm MM memory consistency model for concurrent programs\n" // NOLINT(*)
// NOLINTNEXTLINE(whitespace/line_length)
" --malloc-fail-assert set malloc failure mode to assert-then-assume\n"
Expand Down
7 changes: 5 additions & 2 deletions src/cbmc/cbmc_parse_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ Author: Daniel Kroening, [email protected]
#include <json/json_interface.h>
#include <xmllang/xml_interface.h>

#include <goto-instrument/cover.h>

class goto_functionst;
class optionst;

Expand Down Expand Up @@ -73,8 +75,9 @@ class optionst;
"(property):(stop-on-fail)(trace)" \
"(error-label):(verbosity):(no-library)" \
"(nondet-static)" \
"(version)" \
"(cover):(symex-coverage-report):" \
"(version)" \
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ Is this additional spacing accidental?

OPT_COVER \
"(symex-coverage-report):" \
"(mm):" \
OPT_TIMESTAMP \
"(i386-linux)(i386-macos)(i386-win32)(win32)(winx64)(gcc)" \
Expand Down
2 changes: 1 addition & 1 deletion src/goto-diff/goto_diff_parse_options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ void goto_diff_parse_optionst::help()
"\n"
"Program instrumentation options:\n"
HELP_GOTO_CHECK
" --cover CC create test-suite with coverage criterion CC\n" // NOLINT(*)
HELP_COVER
"Other options:\n"
" --version show version and exit\n"
" --json-ui use JSON-formatted output\n"
Expand Down
4 changes: 3 additions & 1 deletion src/goto-diff/goto_diff_parse_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ Author: Peter Schrammel
#include <goto-programs/show_goto_functions.h>
#include <goto-programs/show_properties.h>

#include <goto-instrument/cover.h>

class goto_modelt;
class optionst;

Expand All @@ -31,7 +33,7 @@ class optionst;
OPT_SHOW_GOTO_FUNCTIONS \
OPT_SHOW_PROPERTIES \
OPT_GOTO_CHECK \
"(cover):" \
OPT_COVER \
"(verbosity):(version)" \
OPT_FLUSH \
OPT_TIMESTAMP \
Expand Down
27 changes: 19 additions & 8 deletions src/goto-instrument/cover.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ void parse_cover_options(const cmdlinet &cmdline, optionst &options)
options.set_option(
"cover-traces-must-terminate",
cmdline.isset("cover-traces-must-terminate"));
options.set_option(
"cover-failed-assertions", cmdline.isset("cover-failed-assertions"));
}

/// Build data structures controlling coverage from command-line options.
Expand Down Expand Up @@ -223,6 +225,9 @@ cover_configt get_cover_config(
cover_config.traces_must_terminate =
options.get_bool_option("cover-traces-must-terminate");

cover_config.cover_failed_assertions =
options.get_bool_option("cover-failed-assertions");

return cover_config;
}

Expand Down Expand Up @@ -285,16 +290,22 @@ static void instrument_cover_goals(
// Simplify the common case where we have ASSERT(x); ASSUME(x):
if(i_it->is_assert())
{
auto successor = std::next(i_it);
if(
successor != function.body.instructions.end() &&
successor->is_assume() &&
successor->get_condition() == i_it->get_condition())
if(!cover_config.cover_failed_assertions)
{
successor->turn_into_skip();
auto successor = std::next(i_it);
if(
successor != function.body.instructions.end() &&
successor->is_assume() &&
successor->get_condition() == i_it->get_condition())
{
successor->turn_into_skip();
}
i_it->type = goto_program_instruction_typet::ASSUME;
}
else
{
i_it->turn_into_skip();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw we have to skip them rather than just leaving them be because otherwise cbmc would consider these assertions coverage goals.

Copy link
Member

Choose a reason for hiding this comment

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

Why not make --no-assertions (aka turn-assertions-into-skips) useable with --cover then (as @martin-cs suggests)? That would make the behaviour more transparent to the user.

}

i_it->type = goto_program_instruction_typet::ASSUME;
}
}
}
Expand Down
13 changes: 13 additions & 0 deletions src/goto-instrument/cover.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,18 @@ class message_handlert;
class cmdlinet;
class optionst;

#define OPT_COVER \
"(cover):" \
"(cover-failed-assertions)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to have this as an argument to --cover?

Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ If we keep this as a separate option from --cover then we should check that --cover-failed-assertions is not passed without --cover and exit with a suitable error message in the case of that particular invalid usage.


#define HELP_COVER \
" --cover CC create test-suite with coverage criterion " \
"CC\n" \
" --cover-failed-assertions do not stop coverage checking at failed " \
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ Because of the preceding refactor, this option now applies to jdiff and goto-diff as well as cbmc. This means that we should probably test that it works with those entry points.

"assertions\n" \
" (this is the default for --cover " \
"assertions)\n"

enum class coverage_criteriont
{
LOCATION,
Expand All @@ -37,6 +49,7 @@ enum class coverage_criteriont
struct cover_configt
{
bool keep_assertions;
bool cover_failed_assertions;
bool traces_must_terminate;
irep_idt mode;
function_filterst function_filters;
Expand Down