-
Notifications
You must be signed in to change notification settings - Fork 274
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
Cover failed assertions #5636
Changes from 3 commits
cefd3ce
a442dbb
d3bfa2b
b3d0bfa
c039da3
a86ffc7
e93654c
3d21a85
bad0d16
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
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; | ||
} |
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW shouldn’t this file be under There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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"). | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -73,8 +75,9 @@ class optionst; | |
"(property):(stop-on-fail)(trace)" \ | ||
"(error-label):(verbosity):(no-library)" \ | ||
"(nondet-static)" \ | ||
"(version)" \ | ||
"(cover):(symex-coverage-report):" \ | ||
"(version)" \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)" \ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -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; | ||
} | ||
|
||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not make |
||
} | ||
|
||
i_it->type = goto_program_instruction_typet::ASSUME; | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,18 @@ class message_handlert; | |
class cmdlinet; | ||
class optionst; | ||
|
||
#define OPT_COVER \ | ||
"(cover):" \ | ||
"(cover-failed-assertions)" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to have this as an argument to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⛏️ If we keep this as a separate option from |
||
|
||
#define HELP_COVER \ | ||
" --cover CC create test-suite with coverage criterion " \ | ||
"CC\n" \ | ||
" --cover-failed-assertions do not stop coverage checking at failed " \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⛏️ Because of the preceding refactor, this option now applies to |
||
"assertions\n" \ | ||
" (this is the default for --cover " \ | ||
"assertions)\n" | ||
|
||
enum class coverage_criteriont | ||
{ | ||
LOCATION, | ||
|
@@ -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; | ||
|
There was a problem hiding this comment.
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.