-
Notifications
You must be signed in to change notification settings - Fork 6
Symbolic abstractions #131
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
base: main
Are you sure you want to change the base?
Conversation
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.
Conceptually this is cool but I am not very convinced, because rn it adds a lot of code and signatures that are barely used, even where they seem to fit (Symex).
The dependency Symex -> Sym_data is quite unpractical, and I wonder if there is not a cleverer split to do, like:
Sym_valfor symbolic values, ie. just defining the shape of different sorts of values (withS_bool,S_int,S_eq,S_elt,S_range)Symex, defines symbolic processes using symbolic valuesSym_data, defines data structures using symbolic values processes.s_mapright now, but could include sets, maybe binary trees, idk.
We could just re-export Sym_val from Sym_data to only provide one module of symbolic things, but at least it would avoid the weird split we have rn, because Symex not reusing S_eq etc. is super weird to me
soteria-c/lib/globs.ml
Outdated
| let open Typed.Infix in | ||
| let* loc = Csymex.nondet Typed.t_loc in | ||
| let+ () = Symex.assume [ Typed.not (loc ==@ Typed.Ptr.null_loc) ] in | ||
| let+ () = Symex.assume [ Typed.S_bool.not (loc ==@ Typed.Ptr.null_loc) ] in |
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.
what I did for bv_values to avoid this, which is super unpractical, is that I include S_bool in Typed -- would make these simpler since bool operations are primitive enough that you always want them imo
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.
ah nvm you can't because the type is t so it'd override stuff.... hmm
| | _ -> Unop (Not, sv) <| TBool | ||
| let conj l = List.fold_left S_bool.and_ v_true l | ||
|
|
||
| let rec split_ands (sv : t) (f : t -> unit) : unit = |
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.
why not include split_ands and conj in S_bool? You're not restricting its type to the module type S_bool anyways so you can add stuff
| module S_bool = struct | ||
| module type S = sig | ||
| type +'a v | ||
| type t | ||
|
|
||
| val not : t v -> t v | ||
| val and_ : t v -> t v -> t v | ||
| val or_ : t v -> t v -> t v | ||
| val to_bool : t v -> bool option | ||
| val of_bool : bool -> t v | ||
| end | ||
|
|
||
| module Make_syntax (S_bool : S) = struct | ||
| let ( &&@ ) = S_bool.and_ | ||
| let ( ||@ ) = S_bool.or_ | ||
| let not = S_bool.not | ||
| end | ||
| end | ||
|
|
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.
Maybe I'm being picky but it was very weird seeing s_int.ml in sym_data but having to go here for S_bool
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.
ok it's because module sym_data depends on Symex but Symex depends on this.... :s
| (** Typed constructors *) | ||
| (** {3 Typed constructors} *) | ||
|
|
||
| module S_bool : Symex.Value.S_bool.S with type 'a v = 'a t and type t = sbool |
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.
here you could do sig include Symex.Value.S_bool.S ... val split_ands : ... end
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.
at least to me it makes a lot more sense if you package all bool operations together
soteria/lib/sym_states/tree_block.ml
Outdated
| open Sym_data.S_int.Make_syntax (MemVal.S_int) | ||
| open Make_bool_syntax (Symex.Value.S_bool) | ||
| module Range = Sym_data.S_range.Make (MemVal.S_int) |
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.
ok this is quite nice -- only thing is the weird mismatch in names/paths between Sym_data.S_int.Make_syntax and Make_bool_syntax but ok
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.
It's because Symex is overriden so I can't go and fetch Symex.Value.S_bool.Syntax :/
| val iter_vars : 'a t -> 'b ty Var.iter_vars | ||
| val subst : (Var.t -> Var.t) -> 'a t -> 'a t |
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.
again maybe i'm being slow but I find it confusing we still have this here despite defining the same functions is s_elt
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.
yeah :/ It's a bit messy I agree
|
The |
I don't think so, the state_monad you have is not necessarily symbolic. It's just a state monad transformer |
|
Gave it a thought and I agree: I should declare S_eq and S_elt in Value, and then declare that value is just include S_eq
include S_elt
module S_bool : S_bool
... |
Signed-off-by: Sacha Ayoun <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
Co-authored-by: N1ark <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
| S with type key = Key.t and module Symex = Symex = struct | ||
| module Symex = Symex | ||
| open Symex.Syntax | ||
| module Raw_map = Stdlib.Map.Make (Key) |
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.
looking back -- why don't you include Raw_map? or at least reexpose some of it, since e.g. one probably wants add, empty functions
Signed-off-by: Sacha Ayoun <[email protected]>
Signed-off-by: Sacha Ayoun <[email protected]>
N1ark
left a comment
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.
still not convinced tbh :/ (sorry if this wasnt ready for review)
| let add = Infix.( +!@ ) | ||
| let sub = Infix.( -!@ ) |
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.
you should mark these as checked since we assume no overflows within blocktrees
| let add = Infix.( +!@ ) | |
| let sub = Infix.( -!@ ) | |
| let add = Infix.( +!!@ ) | |
| let sub = Infix.( -!!@ ) |
| let add = Infix.( +!@ ) | ||
| let sub = Infix.( -!@ ) |
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.
same here, see original code
| let add = Infix.( +!@ ) | |
| let sub = Infix.( -!@ ) | |
| (* We assume addition/overflow within the range of an allocation may never overflow. | |
| This allows extremely good reductions around inequalities, which Tree_block relies on. *) | |
| let ( +@ ) = Infix.( +!!@ ) | |
| let ( -@ ) = Infix.( -!!@ ) |
soteria/lib/bv_values/bv_solver.ml
Outdated
| | Unop (Not, e) -> | ||
| let e' = simplify e in | ||
| if Svalue.equal e e' then fallback v else Svalue.Bool.not e' | ||
| if Svalue.equal e e' then v else Svalue.S_bool.not e' |
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.
So here fallback is basically Analyses.simplify. If you remove the fallback here you get less reductions, in particular in some corner cases.
e.g. if you have a state where x is in [3, INT_MAX] you cannot simplify x < 5 (that would be OX), but you can simplify !(x < 5) (which is x >= 5) into true.
(ofc this example in particular cannot happen since we'd reduce !(x < 5) to x >= 5 directly but i know there are cases where this applies)
i think i would rather keep all these inner fallbacks if possible, we can always do some benchmarking at some point to see how useful they are
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.
Yes, this is just a mismanaged merge conflict, thanks for catching this
| val v_true : [> sbool ] t | ||
| val v_false : [> sbool ] t | ||
| val bool : bool -> [> sbool ] t | ||
| val as_bool : 'a t -> bool option | ||
| val and_ : [< sbool ] t -> [< sbool ] t -> [> sbool ] t | ||
| val conj : [< sbool ] t list -> [> sbool ] t | ||
| val split_ands : [< sbool ] t -> ([> sbool ] t -> unit) -> unit | ||
| val or_ : [< sbool ] t -> [< sbool ] t -> [> sbool ] t | ||
| val not : sbool t -> sbool t |
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.
curiosity : why would you unexport some of these but not all ? maybe in this case it makes sense in the module S_bool : Symex.Value.S_bool.S declaration to instead have something like
module S_bool : sig
include Symex.Value.S_bool.S with ...
val v_true : ...
etc
endThere 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.
I really messed up that merge didn't I? 😆
| module type S_bool = sig | ||
| val v_true : t | ||
| val v_false : t | ||
| val as_bool : t -> bool option | ||
| val bool : bool -> t | ||
| val to_bool : t -> bool option | ||
| val of_bool : bool -> t |
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.
here you could do
module type S_bool = sig
include Symex.Value.S_bool.S with ...
val v_true : ...
etc
endSigned-off-by: Sacha Ayoun <[email protected]>
|
I think I won't merge this, but here's a list of things that are in this PR, and I will import them as needed in other PRs.
Opinions are welcome btw |
This PR introduces the
Sym_datafolder, which contains utilities for what I call (in my thesis) Symbolic Abstractions. Really... simple abstract domains.S_eq is an abstraction that implements equality, while S_int is an abstraction over integers (i.e. a symbolic "thing" that can be interpreted as an integer).
I also add S_range (used for TreeBlocks) and S_map (used for PMap) -- We really need a linter to enforce naming conventions 😭
I also extract more clearly what
sboolis and explicitly, i.e. an abstraction over booleans. Some of the resulting code is slightly more verbose but I think that's ok.Also, some of the code in TreeBlock is a bit verbose because of
MemVal.S_int.zero ()everywhere but #125 fixes this already :)