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

Z3 fixes and enhancements #658

Merged
merged 11 commits into from
Jun 1, 2017
Merged

Conversation

delcypher
Copy link
Contributor

This upstreams a bunch of enhancements for working with Z3 from my fork.

@ccadar This includes the Z3 API logging I mentioned to you (see -z3-log-interaction option).

It also starts to address #653. I won't continue doing that until this PR is merged.

@delcypher delcypher mentioned this pull request May 24, 2017
Copy link
Contributor

@andreamattavelli andreamattavelli left a comment

Choose a reason for hiding this comment

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

@delcypher thanks for your work!
Besides some small changes I would apply, looks good to me.

if (z3LogInteractionFileArg)
this->z3LogInteractionFile = std::string(z3LogInteractionFileArg);
if (z3LogInteractionFile.length() > 0) {
llvm::errs() << "Logging Z3 API interaction to \"" << z3LogInteractionFile
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use klee_message for consistency with other messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Z3Solver is also used in the kleaver tool. Are you sure you want this change? Same applies for the other comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, if I'm not wrong we already use klee_warning in both STPSolver.cpp and Z3Solver.cpp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we do. I didn't put them there though...
I'll go ahead and make the changes then.

#include <string>

namespace klee {
llvm::raw_fd_ostream *klee_open_file(std::string &path, std::string &error);
Copy link
Contributor

Choose a reason for hiding this comment

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

I belive that we can now use this new functionality in many other places.

Copy link
Contributor

Choose a reason for hiding this comment

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

@delcypher this is just a comment btw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying.

std::string error;
dumpedQueriesFile = klee_open_file(Z3QueryDumpFile, error);
if (!error.empty()) {
llvm::errs() << "Error creating file for dumping Z3 queries:" << error
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use klee_error for consistency.

@andreamattavelli
Copy link
Contributor

@delcypher I forgot to mention that 84464e9 and bcb498a should me squashed together in my opinion.

@delcypher
Copy link
Contributor Author

@andreamattavelli

@delcypher I forgot to mention that 84464e9 and bcb498a should me squashed together in my opinion

Agreed. I'll do that once we've reached a decision on your other suggested changes.

`klee_open_output_file()` function so that it can be used by
the Z3Solver.
@delcypher
Copy link
Contributor Author

@andreamattavelli I've made your suggested changes. I also renamed klee_open_file() to klee_open_output_file() to make it clear the opened file is for writing, not reading.

@andreamattavelli
Copy link
Contributor

@delcypher Great! I don't have any more comments. I give @ccadar the opportunity to comment before merging.

@ccadar
Copy link
Contributor

ccadar commented May 25, 2017

This looks ok, but it would be useful to describe the new options in the documentation before merging.

@delcypher
Copy link
Contributor Author

@ccadar Not all of these options are permanent. -z3-query-dump= for example is a hack that will be removed if the work I started and gave up on in #629 ever gets accepted.

@ccadar
Copy link
Contributor

ccadar commented May 25, 2017

If they change, the documentation will change with them. I don't see this as a reason not to document them. Also, the unwritten convention is to name validation/debugging techniques using the prefix debug-, would you be happy with this?

@delcypher
Copy link
Contributor Author

delcypher commented May 25, 2017

If they change, the documentation will change with them. I don't see this as a reason not to document them.

I don't want people to rely on something that is likely to change quite soon.

Also, the unwritten convention is to name validation/debugging techniques using the prefix debug-, would you be happy with this?

Not really but for selfish reasons

  • The -z3-* options appear at the end of -help which makes finding them super convenient.
  • Changing the names will cause me problems the next time I rebase on master.

If you insist I can change them.

@delcypher
Copy link
Contributor Author

Documentation written: klee/klee.github.io#85

It documents options as they are now and also a bunch of options for the other solver backends too.

Dan Liew added 7 commits May 26, 2017 08:47
is useful for getting access to the constraints being stored in the Z3
solver in the SMT-LIBv2.5 format.
This can be enabled by passing the command line option `-debug-z3-validate-models`.
Although Z3 has a global parameter `model_validate` (off by default) I don't trust it
so do the validation manually. This also means we can potentially do
validation on a per Z3Solver instance basis rather than globally.

When failing to validate a Z3 model the solver state and model are
dumped to standard error.
Add `-debug-z3-log-api-interaction` option to allow Z3 API calls to be
logged to a file. The files logged by this option can be replayed by the
`z3` binary (using its `-log` option). This is incredibly useful because
it allows to exactly replay Z3's behaviour outside of KLEE.
solver. This is to avoid tampering with the cache of the builder the
solver is using.
My discussions [1] with the Z3 team have revealed that
`Z3_mk_simple_solver()` is the wrong solver to use. That solver
basically runs the `simplify` tactic and then the `smt` tactic.
This by-passes Z3's attempt to probe for different logics and
apply its own specialized tactic.

Using `Z3_mk_solver()` should be closer to the behaviour of the
Z3 binary.

This partially addresses klee#653. We still need to try rolling our
own custom tactic.

[1] Z3Prover/z3#1035
…:<N>` option.

This lets us see what Z3 is doing execution (e.g. which tactic is being
applied) which is very useful for debugging.
delcypher pushed a commit to delcypher/klee.github.io that referenced this pull request May 26, 2017
@delcypher
Copy link
Contributor Author

@ccadar Options have been renamed.

delcypher pushed a commit to delcypher/klee.github.io that referenced this pull request May 31, 2017
@andreamattavelli
Copy link
Contributor

@delcypher @ccadar I think we are ready to merge as I don't see any reason to not do it.

@ccadar
Copy link
Contributor

ccadar commented Jun 1, 2017

This LGTM too. I think @delcypher wanted to squash some commits, otherwise we're ready to merge.

@delcypher
Copy link
Contributor Author

@ccadar I've already done all the squashing I wanted. Merging now.

@delcypher delcypher merged commit 641bc46 into klee:master Jun 1, 2017
delcypher pushed a commit to delcypher/klee.github.io that referenced this pull request Jun 1, 2017
ccadar pushed a commit to ccadar/klee.github.io that referenced this pull request Sep 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants