-
Notifications
You must be signed in to change notification settings - Fork 273
[CONTRACTS] Use unified loop contract config #8356
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
[CONTRACTS] Use unified loop contract config #8356
Conversation
e8c34b4
to
a018316
Compare
loop_contract_configt loop_contract_config = { | ||
cmdline.isset(FLAG_LOOP_CONTRACTS), | ||
!cmdline.isset(FLAG_LOOP_CONTRACTS_NO_UNWIND)}; | ||
|
||
if( | ||
cmdline.isset(FLAG_LOOP_CONTRACTS) && | ||
cmdline.isset(FLAG_LOOP_CONTRACTS_NO_UNWIND)) | ||
{ | ||
// When the model is produced by Kani, we must not automatically unwind | ||
// the backjump introduced by the loop transformation. | ||
// Automatic unwinding duplicates assertions found in the loop body, and | ||
// since Kani expects property identifiers to remain unique. Having | ||
// duplicate instances of the assertions makes Kani fail to handle the | ||
// analysis results. | ||
log.warning() << "**** WARNING: transformed loops will not be unwound " | ||
<< "after applying loop contracts. Remember to unwind " | ||
<< "them at least twice to pass unwinding-assertions." | ||
<< messaget::eom; | ||
} |
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.
I'm a little concern about this. Why would users need to unwind loop at least twice? The advice should be they must unwind the loops to pass unwinding assertions. Also, this option is not set by default, correct? Why is this Kani specific? Maybe we should re-phrase the documentation to be more general. For instance, I don't see a problem for us to require users to unwind the the remaining loops after loop contract instrumentation, maybe we can even report to them the remaining loops, we don't need to justify this by Kani since it's only a user of the feature, right?
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.
Although this is just code moved around in this PR I'm with Felipe that this PR is an opportunity to improve on the documentation.
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.
After the instrumentation, the transformed will contains a back jump. Running CBMC with unwinding bound 1 will fail the unwinding assertions. Not providing unwinding bounds or providing bounds greater than 1 are fine.
By default, goto-instrument will unwinding the instrumented loops by 2.
It is not Kani specific. I will update the documentation and clarify the motivation of this flag.
loop_contract_configt loop_contract_config = { | ||
cmdline.isset(FLAG_LOOP_CONTRACTS), | ||
!cmdline.isset(FLAG_LOOP_CONTRACTS_NO_UNWIND)}; | ||
|
||
if( | ||
cmdline.isset(FLAG_LOOP_CONTRACTS) && | ||
cmdline.isset(FLAG_LOOP_CONTRACTS_NO_UNWIND)) | ||
{ | ||
// When the model is produced by Kani, we must not automatically unwind | ||
// the backjump introduced by the loop transformation. | ||
// Automatic unwinding duplicates assertions found in the loop body, and | ||
// since Kani expects property identifiers to remain unique. Having | ||
// duplicate instances of the assertions makes Kani fail to handle the | ||
// analysis results. | ||
log.warning() << "**** WARNING: transformed loops will not be unwound " | ||
<< "after applying loop contracts. Remember to unwind " | ||
<< "them at least twice to pass unwinding-assertions." | ||
<< messaget::eom; | ||
} |
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.
Although this is just code moved around in this PR I'm with Felipe that this PR is an opportunity to improve on the documentation.
a018316
to
73d8be1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8356 +/- ##
===========================================
+ Coverage 78.09% 78.27% +0.17%
===========================================
Files 1726 1726
Lines 189039 188605 -434
Branches 18399 18443 +44
===========================================
Hits 147629 147629
+ Misses 41410 40976 -434 ☔ View full report in Codecov by Sentry. |
73d8be1
to
a7afbe2
Compare
We now use boolean flag for old loop contracts and
dfcc_loop_contract_mode
for dfcc loop contracts to manage the configuration of loop contracts, which is hard to extend.This PR will add a new struct
loop_contract_config
to manage config for both loop contracts.