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

Fix address set meet #924

Merged
merged 10 commits into from
Nov 23, 2022
Merged

Fix address set meet #924

merged 10 commits into from
Nov 23, 2022

Conversation

jerhard
Copy link
Member

@jerhard jerhard commented Nov 22, 2022

The meet of addresses is currently unsound when different integer domains may be activated within an analysis run. This is demonstrated by this example. Here, the abstract representation of the address pointed to by slotvec0.val contains an offset where all int domains are active, as all int domains are activated for global variables with the annotation.int.enabled setting. At the point of the comparison however, only def_exc is active. Therefore the abstract addresses are kept in different buckets and the meet that is performed yields bottom.

This issue also appears on sv-comp when the congruence auto-tuning is enabled, and for some functions the congruence domain is activated, and for others not.

This PR fixes this issue by not keeping the index information in the representative that is used to split address sets.

(Closes #925)

jerhard and others added 3 commits November 22, 2022 14:00
The representative should not keep any information about the values of the index offset. Previously, the top() offset led to offsets being kept apart (i.e. the corresponding addresses in different buckets) in the case that different integer domains were active in the index offset.

Co-authored-by: Michael Schwarz <michael.schwarz93@gmail.com>
Co-authored-by: Sarah Tilscher <66023521+stilscher@users.noreply.github.com>
@michael-schwarz
Copy link
Member

I'll start a run of this together with #911.

@sim642
Copy link
Member

sim642 commented Nov 22, 2022

This PR fixes this issue by not keeping the index information in the representative that is used to split address sets.

But that is already the case:

(** Representatives for lvalue sublattices as defined by {!NormalLat}. *)
module R: DisjointDomain.Representative with type elt = t =
struct
include Normal (Idx)
type elt = t
let rec of_elt_offset: Offs.t -> Offs.t =
function
| `NoOffset -> `NoOffset
| `Field (f,o) -> `Field (f, of_elt_offset o)
| `Index (_,o) -> `Index (Idx.top (), of_elt_offset o) (* all indices to same bucket *)

Isn't this just another instance of #822?

@michael-schwarz
Copy link
Member

michael-schwarz commented Nov 22, 2022

The issue is Idx.top () that depends on the activated int domains.
In the example the two address values for which the meet is \bot are identical except for the number of activated int domains.

@jerhard jerhard marked this pull request as ready for review November 22, 2022 13:51
Rename the variants in NormalLatRepr, and the NormalLatRepr.Offset module.
@sim642
Copy link
Member

sim642 commented Nov 22, 2022

The issue is Idx.top () that depends on the activated int domains.

That seems like a problem in itself because it's supposed to be a real top, not top_of. Having multiple incomparable "tops" defies the definition of top.

Also, this is the index domain, which is a separate thing from the intdomain that's actually used for integer values.

In the example the two address values for which the meet is \bot are identical except for the number of activated int domains.

It still seems to me that something deeper is wrong here that such situation can arise at all. Aren't the intdomain projections that are used for converting integer abstractions between different precisions supposed to ensure that we don't have such mixture?
There might be a projection missing somewhere. If it's a problem here, it's a problem elsewhere as well because AFAIK our intdomain operations aren't meant to be used between tuples with different precisions. The fact that such mismatches arise at all is the bug.
If integers with mismatching precisions coexist, then other operations on them could break precisely in the same way.

src/cdomains/lval.ml Show resolved Hide resolved
@jerhard
Copy link
Member Author

jerhard commented Nov 22, 2022

@michael-schwarz started a run with a version of this PR, so I would suggest that if the results for that run are well, we merge this hot-fix PR for the sv-comp version. We would implement a follow-up PR that addresses your remark, @sim642, to be merged after submission for sv-comp.

Co-authored-by: Michael Schwarz <michael.schwarz93@gmail.com>
@sim642 sim642 merged commit bf9e0be into master Nov 23, 2022
@sim642 sim642 deleted the fix_address_meet branch November 23, 2022 08:07
jerhard added a commit that referenced this pull request Nov 23, 2022
Revert changes to lval.ml from #924 to allow for cleaner implementation.

Revert "Rename ReprMyStrPtr to ReprStrPtr."

This reverts commit a754b7d.

Revert "Rename NormalLatRepr variants and inner Offset module"

This reverts commit 0c02495.

Revert "Add comment explaining why separate type for offsets without abstract value for index offsets is introduced."

This reverts commit 18ebb35.

Revert "Imporve Lval.R.show output for strings."

This reverts commit a0f48e6.

Revert "Lval.R: Implement show and related print functions."

This reverts commit d5bdc96.

Revert "Rename type to r"

This reverts commit 21bd2cd.

Revert "Addressets: Remove index-offset information from representative."

This reverts commit d35bc54.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Address sets may contain offsets with mismatching int domain precisions
3 participants