-
Notifications
You must be signed in to change notification settings - Fork 14
WIP: Smtlib Prelude Injection #340
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: master
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.
This is super neat! I have a few questions:
-
Is the prelude added by default if the user specifies a precondition or postcondition via smtlib string? For the comparative case, are we duplicating the constraints that equate registers to each other between the original and modified binaries?
-
I wonder if we should be more consistent with the uses of dashes vs underscores in the prelude names. For example, we have some functions that use dashes and some that use underscores.
(define-fun mem-equal () Bool (= mem_orig mem_mod))
(define-fun init_main_argc_orig () (_ BitVec 64) init_RDI0)
- In the potential predicates that you mentioned, what does the
pto
inpto-stack
andpto-heap
mean?
let define_fun name args retsort body = | ||
let args = List.map ~f:(fun (v,sort) -> Sexp.List [v; sort]) args in | ||
Sexp.List [Sexp.Atom "define-fun"; Sexp.Atom name; Sexp.List args; retsort; body] | ||
let synonym name ret body = define_fun name [] ret body |
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.
Would it be possible to annotate the types of the arguments to these functions?
let sexp_of_sort (s : Z3.Sort.sort) : Sexp.t = Sexp.of_string (Z3.Sort.to_string s) | ||
|
||
(* [make_arg_synonyms] constructs smtlib formula for the arguments of a subroutine. | ||
This make synonyms that for high level c-like names ot low level locations *) |
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.
Is ot
supposed to be at
?
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.
I have to admit I still don't really get this sentence.
Maybe an example here?
(define-sort BV64 () (_ BitVec 64)) | ||
(define-sort char () (_ BitVec 8)) | ||
(define-sort byte () (_ BitVec 8)) | ||
(define-sort int16 () (_ BitVec 16)) |
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's the difference between a BV16 and an int16 in this case?
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.
Nothing. I thought it was reasonable or at least unharmful to have C-esque looking types and something that is just a nice shorthand evocative of (_ BitVec 64). Maybe I should use int16_t
instead
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.
Definitely int16_t
!
define_sort "memsort" (sexp_of_sort (Z3.Expr.get_sort z3_mem)) | ||
|
||
let compare_prelude_smtlib2 main_sub1 main_sub2 (env1 : Env.t) (env2 : Env.t) : string = | ||
let std_prelude = " |
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.
I wonder if we should have some sort of file that contains the prelude that we read in over here rather than having a hard-coded string.
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.
I understand the impulse, but I'd rather not. A large amount of the string is programmatically generated and going to an external file adds a lot of complexity in terms of finding the file in question from wherever the wp is invoked. I think going to an external file was a mistake we made in vibes.
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.
I stand by the vibes decision though.
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.
In this case, I feel like the string is short enough to allow minor sins.
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.
We added an unnecessary and weird installation step that can easily go wrong or get stale just to avoid recompiling when we as developers edit the string. No user is ever going to edit this string and they can achieve the same functionality by writing define-fun
in their own smtlib. There is the possibility of adding another command line feature of --wp-user-lib=mylib.smt2
to avoid duplication between the pre and post condition. I'm not entirely sure we have to have both pre and post condition flags anyway, since this can be achieved via =>
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 so?
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.
(=> (preconds referring to init_ variables ) (post-conds referring to both init and non-init))
. I feel like this is how I've seen Chris write specs in vibes
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.
True. I guess we have some redundancy between init_var
and the pre
flag.
|
(** [args_prelude_compare] builds smtlib synonyms in a comparison for the arguments of | ||
two compared subroutines. *) | ||
let args_prelude_compare | ||
(main_sub1 : Sub.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.
This might need to change on rebase: there's a new type 'a code
which represents a pair of an 'a term
and an env
.
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 in compare.mli
)
Cool! This answers all of my questions. I think we should document when the prelude gets added and a list of the pre-defined predicates somewhere. |
I'm good with this, as soon as it's rebased. |
Initial pass at adding a smtlib prelude. Ready for discussion at least I think.
Currently on the test example, it prepends the following string to both the precond and postcond
Note that bap prepends the name of the subroutine to the variable names. Perhaps we want to trim this off. We also want to inject the prelude for single binary mode.
Features that could still be added to the prelude: