Fix invalidation of CARs on free and DEAD instructions #6385
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SaswatPadhi
added
bugfix
aws
SaswatPadhi
force-pushed
the
free-dead-fix
branch
3 times, most recently
from
baa88bd to
0b64870
Compare
SaswatPadhi
force-pushed
the
free-dead-fix
branch
from
0b64870 to
04d07a7
Compare
remi-delmas-3000
approved these changes
Oct 6, 2021
Previously, corresponding CARs were completely removed on DEAD instructions. While this is "okay" for unconditionally dead symbols, it resulted in spurious non-assignable errors when symbols were only conditionally DEAD, e.g. RETURN within a conditional block. Previously, invalid CARs from free calls were simply left behind, because (in theory) the CAR validation condition should automatically guard against conditions. However, in both these cases, we need to check dead/deallocated status of individual objects to avoid spurious errors from CBMC's internal pointer checks, which only track a single dead/deallocated object nondeterministically.
SaswatPadhi
force-pushed
the
free-dead-fix
branch
from
04d07a7 to
998d141
Compare
Codecov Report
@@ Coverage Diff @@
## develop #6385 +/- ##
========================================
Coverage 75.92% 75.93%
========================================
Files 1518 1518
Lines 164007 164040 +33
========================================
+ Hits 124526 124565 +39
+ Misses 39481 39475 -6
Continue to review full report at Codecov.
|
feliperodri
approved these changes
Oct 7, 2021
Comment on lines
+935
to
+937
| throw incorrect_goto_program_exceptiont( | ||
| "Found a `DEAD` variable without corresponding `DECL`!", | ||
| instruction_it->source_location); |
There was a problem hiding this comment.
Nice! We should use this instead of throw 0.
jimgrundy
reviewed
Oct 7, 2021
| .symbol_expr(); | ||
| write_set_validity_addrs.insert(object_validity_var_addr); | ||
|
|
||
| alias_checking_instructions.add(goto_programt::make_decl( |
There was a problem hiding this comment.
Should we be concerned that there is no test covering this line?
There was a problem hiding this comment.
Most likely a false positive from Codecov. Other statements in the same loop are covered, and there is no break or continue in between.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, corresponding CARs were completely removed on
DEADinstructions. While this is "okay" for unconditionally dead symbols, it resulted in spurious non-assignable errors when symbols were conditionallyDEAD, e.g.RETURNwithin a conditional block.Previously, invalid CARs from
freecalls were simply left behind, because (in theory) the CAR validation condition should automatically guard against conditions.However, in both these cases, we need to check dead/deallocated status of individual objects to avoid spurious errors from CBMC's internal pointer checks. This PR fixes this.
The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/My commit message includes data points confirming performance improvements (if claimed).