Skip to content
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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sim642
Copy link
Member

@sim642 sim642 commented Oct 1, 2024

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:

%%% congruence: mul : 4294967295 1+3ℤ -> ℤ 
%%% congruence: add : 3ℤ ℤ -> ℤ
%%% congruence: sub : 3ℤ 1+3ℤ -> ℤ
%%% congruence: mul : 4294967295 2 -> 8589934590 
%%% congruence: add : 3ℤ 8589934590 -> 3ℤ
%%% congruence: sub : 3ℤ 2 -> 3ℤ
%%% congruence: meet 2 ℤ -> 2
%%% congruence: meet 1+3ℤ 3ℤ -> ⟂

For context, this is about sub being implemented as

let sub ?(no_ov=false) ik x y = add ~no_ov ik x (neg ~no_ov ik y)

where neg involves a mul:
| Some _ -> mul ~no_ov ik (of_int ik (Z.of_int (-1))) x

In fact, there are multiple problems visible here:

  1. The subtractions are obviously unsound: 3ℤ - (1+3ℤ) = ℤ (should be 2+3ℤ?) and 3ℤ - 2 = 3ℤ (should be 1+3ℤ?). I'm not sure if this is unsound because of the indirect definition of sub or what.
  2. The negation via multiplication by -1 (which is 4294967295 by unsigned wrap-around) gives a constant with is out of bounds of the type: 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.

@sim642 sim642 added bug unsound sv-comp SV-COMP (analyses, results), witnesses labels Oct 1, 2024
@sim642 sim642 added this to the SV-COMP 2025 milestone Oct 1, 2024
@michael-schwarz
Copy link
Member

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.

The subtractions are obviously unsound: 3ℤ - (1+3ℤ) = ℤ (should be 2+3ℤ?) and 3ℤ - 2 = 3ℤ (should be 1+3ℤ?).

Iirc, there is some problem to be considered for when things become negative... But I would have to check that again.

@sim642
Copy link
Member Author

sim642 commented Oct 1, 2024

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:

  1. Things like 5 - 3 which remain non-negative.
  2. Things like 5 - 10 which go negative and wrap around with unsigned types, additionally modulo some power of 2.

Since congruences are unaware of intervals, they cannot distinguish the two (this has also come up in #1156) and must account for both. Thus, 3ℤ - (1+3ℤ) = ℤ should be fine and sound (it's just top). But 3ℤ - 2 = 3ℤ still is unsound, even for no-wrap cases.

Although addition should also have two cases: normal and wrap modulo power of 2. I wonder if add is even sound then.
It's starting to look like our congruences won't be able to find out anything like this, unless the values are constants (but then we don't need congruences in the first place).

@michael-schwarz
Copy link
Member

It's starting to look like our congruences won't be able to find out anything like this

We might want to think about feeding back interval information somehow.

@jerhard jerhard self-assigned this Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug sv-comp SV-COMP (analyses, results), witnesses unsound
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants