-
Notifications
You must be signed in to change notification settings - Fork 77
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
Unsound congruence domain arithmetic #1587
base: master
Are you sure you want to change the base?
Conversation
Nice catch, I guess the warning is immediately starting to pay off! It's impressive this hasn't lead to unsound results anywhere. I guess it is because congruences are not enabled that often by the autotuner.
Iirc, there is some problem to be considered for when things become negative... But I would have to check that again. |
That might explain some of this because subtraction involves two cases:
Since congruences are unaware of intervals, they cannot distinguish the two (this has also come up in #1156) and must account for both. Thus, Although addition should also have two cases: normal and wrap modulo power of 2. I wonder if |
We might want to think about feeding back interval information somehow. |
… with which to add.
…s bug, but goes through when it is fixed.
let m = m -: c in | ||
let y = Some (c, m) in | ||
add ~no_ov ik x y |
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.
How does this work for constants that are represented as (k,0)
in this domain, which this operation turns into (k,-k)
. Is this correct? Such a value looks like it would be bot?
I inspected the first
ERROR (both branches dead)
result after #1579 on sv-benchmarks and it lead to unsound congruence domain arithmetic which presents itself as both branches dead in a reduced sv-benchmarks program (in this PR).It's from a hardness task, and those make up 72 of the 81 cases from a nightly run. I didn't check others because they might all be for the same reason.
In particular, the unsound operations are visible from this tracing output:
For context, this is about
sub
being implemented asanalyzer/src/cdomain/value/cdomains/intDomain.ml
Line 3129 in 060004c
where
neg
involves amul
:analyzer/src/cdomain/value/cdomains/intDomain.ml
Line 3102 in 060004c
In fact, there are multiple problems visible here:
3ℤ - (1+3ℤ) = ℤ
(should be2+3ℤ
?) and3ℤ - 2 = 3ℤ
(should be1+3ℤ
?). I'm not sure if this is unsound because of the indirect definition ofsub
or what.4294967295 * 2 = 8589934590
. Seems like this result should also be reduced modulo 4294967296.There's a
normalize
function in the congruence domain, but the arithmetic operations don't seem to call it in all cases.