shift left overflow check now done in goto_check #3662
Conversation
kroening
commented
Jan 3, 2019
kroening
commented
- Each commit message has a non-empty body, explaining why the change was made.
- n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
- n/a The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
- Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
- n/a My commit message includes data points confirming performance improvements (if claimed).
- My PR is restricted to a single feature or bugfix.
- n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.
kroening
requested review from
chrisr-diffblue,
martin-cs,
peterschrammel,
pkesseli,
smowton,
tautschnig and
thk123
as code owners
kroening
force-pushed
the
shl-overflow
branch
4 times, most recently
from
d637d5a to
6e68a12
Compare
There was a problem hiding this comment.
🚫
This PR failed Diffblue compatibility checks (cbmc commit: 6e68a12).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/96221565
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.
Common spurious failures:
- the cbmc commit has disappeared in the mean time (e.g. in a force-push)
- the author is not in the list of contributors (e.g. first-time contributors).
The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.
| ^\[.*\] .* arithmetic overflow on signed shl in .*: FAILURE$ | ||
| ^\*\* 2 of 4 failed | ||
| ^\[.*\] line 6 arithmetic overflow on signed shl in .*: FAILURE$ | ||
| ^\[.*\] line 9 arithmetic overflow on signed shl in .*: SUCCESS$ |
There was a problem hiding this comment.
Perhaps add a must-not-match for "line 12 arithmetic overflow" if that's intentionally not tested?
src/analyses/goto_check.cpp
Outdated
| neg_dist_shift = | ||
| binary_relation_exprt(op, ID_lt, from_integer(0, distance_type)); | ||
|
|
||
| // an undefined shift of a non-zero value always results in overflow |
There was a problem hiding this comment.
an undefined shift of a non-zero value --> shifting a non-zero value by more than its width?
src/analyses/goto_check.cpp
Outdated
| binary_relation_exprt(op, ID_lt, from_integer(0, distance_type)); | ||
|
|
||
| // an undefined shift of a non-zero value always results in overflow | ||
| const exprt undef = binary_predicate_exprt( |
There was a problem hiding this comment.
undef is defined but not used, so this probably doesn't achieve the "always overflow" behaviour the comment asks for
| const exprt op_ext_shifted = shl_exprt(op_ext, distance); | ||
|
|
||
| // get top bits of the shifted operand | ||
| const exprt top_bits = extractbits_exprt( |
There was a problem hiding this comment.
? Why shift and then extract? All the bits we're interested in testing are present in the original value.
There was a problem hiding this comment.
Our extactbits matches the one in the SMT-LIB, and thus, only does constants.
Uses standard pattern, enables stronger typing.
The current implementation generates an overflow-shl predicate, which is then interpreted by the solver APIs. This has the disadvantage that the predicate has semantics that are both complicated and highly language-dependent, which is not a good fit for a solver. The new implementation defines the meaning of signed left shift overflows in goto-check, similar as it is already done for division and unary minus. This is covered by an existing test: regression/cbmc/Overflow_Leftshift1/test.desc
|
ok, added |
There was a problem hiding this comment.
🚫
This PR failed Diffblue compatibility checks (cbmc commit: 4446667).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/96338889
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.
Common spurious failures:
- the cbmc commit has disappeared in the mean time (e.g. in a force-push)
- the author is not in the list of contributors (e.g. first-time contributors).
The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.
There was a problem hiding this comment.
✔️
Passed Diffblue compatibility checks (cbmc commit: 3be9b5e).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/96339556
