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

Make meet in AddressDomain more precise #1468

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/analyses/base.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1579,7 +1579,7 @@ struct
let set ~(ctx: _ ctx) ?(invariant=false) ?(blob_destructive=false) ?lval_raw ?rval_raw ?t_override (st: store) (lval: AD.t) (lval_type: Cil.typ) (value: value) : store =
let update_variable x t y z =
if M.tracing then M.tracel "set" ~var:x.vname "update_variable: start '%s' '%a'\nto\n%a" x.vname VD.pretty y CPA.pretty z;
let r = update_variable x t y z in (* refers to defintion that is outside of set *)
let r = update_variable x t y z in (* refers to definition that is outside of set *)
if M.tracing then M.tracel "set" ~var:x.vname "update_variable: start '%s' '%a'\nto\n%a\nresults in\n%a" x.vname VD.pretty y CPA.pretty z CPA.pretty r;
r
in
Expand Down
6 changes: 6 additions & 0 deletions src/cdomain/value/cdomains/addressDomain.ml
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,11 @@ struct
| StrPtr _, UnknownPtr -> None
| _, _ -> Some false

let amenable_to_meet x y = match x,y with
| StrPtr _, StrPtr _ -> true
| Addr x, Addr y when Mval.equal (Mval.top_indices x) (Mval.top_indices y) -> true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't the address set buckets already guarantee this to only be called in such cases because Mval.top_indices are the representatives?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if the buckets guarantee this already, this function can be called with two arbitrary addresses, so we can not just return true.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, although it could just be an implementation detail (like the buckets themselves are), which isn't exposed to the outside by the interface. Then such misuse would be impossible.

| _ -> false

let leq x y = match x, y with
| StrPtr s1, StrPtr s2 -> SD.leq s1 s2
| Addr x, Addr y -> Mval.leq x y
Expand Down Expand Up @@ -178,6 +183,7 @@ struct
struct
include SetDomain.Joined (Addr)
let may_be_equal a b = Option.value (Addr.semantic_equal a b) ~default:true
let amenable_to_meet = Addr.amenable_to_meet
end
module OffsetSplit = DisjointDomain.ProjectiveSetPairwiseMeet (Addr) (J) (Addr.UnitOffsetRepr)

Expand Down
4 changes: 4 additions & 0 deletions src/cdomain/value/cdomains/addressDomain_intf.ml
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ sig
(** Check semantic equality of two addresses.

michael-schwarz marked this conversation as resolved.
Show resolved Hide resolved
@return [Some true] if definitely equal, [Some false] if definitely not equal, [None] if unknown. *)

val amenable_to_meet: t -> t -> bool
(** Whether two addresses are amenable to meet operation, i.e., their lattice meet overapproximates the intersection
of concretizations. If true, meet is used instead of semantic_equal *)
end

(** Address lattice with sublattice representatives for {!DisjointDomain}. *)
Expand Down
14 changes: 12 additions & 2 deletions src/domain/disjointDomain.ml
Original file line number Diff line number Diff line change
Expand Up @@ -182,16 +182,26 @@ module type MayEqualSetDomain =
sig
include SetDomain.S
val may_be_equal: elt -> elt -> bool
val amenable_to_meet: elt -> elt -> bool
Comment on lines 184 to +185
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if the two predicates could be combined into one more meaningful thing. Because I guess now there's supposed to be some kind of relationship between them, given that we only check them in certain order.

Would this be the correct description?

val same_sublattice: elt -> elt -> bool option
(** @return [Some true] if definitely in same sublattice, [Some false] if definitely not, [None] if unknown. *)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm skeptical here: may_be_equal is a strange semantic property for which there is no real definition in terms of lattices.

So I think using lattice terminology here will be only confusing. For example what is difference between Some false and None in your proposed definition above:

  • Even if they are definitely not from the same sublattice, they may still be equal or not
  • Same if I can't determine whether they are in the same sublattice.

So it once again boils down to the two properties above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  type eqinfo = Maybe | No | Meet
  val may_be_equal: elt -> elt -> eqinfo

One could do something like this but I don't think that is particularly nice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm skeptical here: may_be_equal is a strange semantic property for which there is no real definition in terms of lattices.

I agree and that's why I'm wondering if there is a less ad-hoc way to view these things. same_sublattice might not be the right thing, but possibly something else.

There are now two predicates who have 4 possible combined results for a pair of arguments, but only three cases are distinguished where the predicates are used.
Suppose some addresses x and y had may_be_equal x y = false before. Then neither was added to the result.
Now if amenable_to_meet x y = true, then meet x y is added to the result, which makes the meet of the entire sets larger (i.e. less precise) than before.

This makes me wonder about the order of these checks. If the previous implementation was sound, then checking may_be_equal first (to add nothing) would still be sound and cannot become less precise due to amenable_to_meet, which could then be checked second to determine whether meet x y is added or both x and y. This would be more precise than the previous implementation by construction.

Or if the case may_be_equal x y = false and amenable_to_meet x y = true should be impossible, then why? That would indicate there being some implicit connection between the two predicates (a semantic and a lattice-theoretic) after all.

end

module ProjectiveSetPairwiseMeet (E: Printable.S) (B: MayEqualSetDomain with type elt = E.t) (R: Representative with type elt = E.t): SetDomain.S with type elt = E.t = struct
module ProjectiveSetPairwiseMeet (E: Lattice.S) (B: MayEqualSetDomain with type elt = E.t) (R: Representative with type elt = E.t): SetDomain.S with type elt = E.t = struct
include ProjectiveSet (E) (B) (R)

let meet m1 m2 =
let meet_buckets b1 b2 acc =
B.fold (fun e1 acc ->
B.fold (fun e2 acc ->
if B.may_be_equal e1 e2 then
if B.amenable_to_meet e1 e2 then
try
let m = E.meet e1 e2 in
if not (E.is_bot m) then
add m acc
else
acc
with Lattice.Uncomparable ->
failwith (GobPretty.sprintf "amenable_to_meet %a %a returned true, but meet throws!" E.pretty e1 E.pretty e2)
else if B.may_be_equal e1 e2 then
add e1 (add e2 acc)
else
acc
Expand Down
30 changes: 30 additions & 0 deletions tests/regression/13-privatized/89-write-lacking-precision.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// PARAM: --set ana.base.privatization write
#include<pthread.h>
struct a {
char* b;
};

struct a *c;
struct a h = {""};
struct a i = {"string"};

void* d(void* args) {
struct a r;
if (c->b) {
__goblint_check(strlen(h.b) == 0); // Should also work for write!
}
}

int main() {
int top;

if(top) {
c = &h;
} else {
c = &i;
}

pthread_t t;
pthread_create(&t, 0, d, 0);
return 0;
}
51 changes: 51 additions & 0 deletions tests/regression/27-inv_invariants/22-meet-ptrs.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
//PARAM: --enable ana.int.interval
#include <stdio.h>
#include <stdlib.h>
#include <time.h>

int more_intricate() {
int arr[20];

int top;

int i = 2;
int j = 8;
if(top) {
i = 8;
j = 9;
}

int* imprecise1 = &arr[i]; // &arr[2..8]
int* imprecise2 = &arr[j]; // &arr[8..9]

if(imprecise1 == imprecise2) {
__goblint_check(imprecise1 == &arr[8]);
__goblint_check(imprecise2 == &arr[8]); //TODO (Refinement should happen in both directions!)
}

if(imprecise1 == &arr[j]) {
__goblint_check(imprecise1 == &arr[8]);
}

}


int main() {
int arr[20];

int top;

int i = 2;
if(top) {
i = 8;
}

int* imprecise = &arr[i];

if(imprecise == &arr[2]) {
__goblint_check(imprecise == &arr[2]);
sim642 marked this conversation as resolved.
Show resolved Hide resolved
}

more_intricate();
return 0;
}
Loading