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

Add support for STP and Yices2 #273

Merged
merged 30 commits into from
Dec 14, 2023
Merged

Conversation

gussmith23
Copy link
Contributor

@gussmith23 gussmith23 commented Dec 13, 2023

Coauthored by @vcanumalla.

TODO before converting from draft:

  • Freeze STP at specific checkout
  • Make sure tests pass locally

@gussmith23 gussmith23 marked this pull request as ready for review December 13, 2023 17:33
@gussmith23
Copy link
Contributor Author

Ready for review @sorawee

"../solver.rkt"
(prefix-in base/ "base-solver.rkt"))

(provide (rename-out [make-yices-smt2 yices-smt2]) yices-smt2? yices-smt2-available?)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this called yices-smt2 rather than e.g. yices2 (or yices)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See here: https://github.com/SRI-CSL/yices2/blob/e27cf308cffb0ecc6cc7165c10e81ca65bc303b3/doc/NOTES#L1182

Apparently they make different binaries for each frontend (rather than changing the frontend with a flag, as other solvers do)

Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be called yices, for consistency with other Rosette solvers.

@sorawee
Copy link
Collaborator

sorawee commented Dec 13, 2023

Please also add documentation here. And please do that for cvc5 and Bitwuzla as well (perhaps as a separate PR). I overlooked this in #260, which was my bad.

If I understand correctly, on Mac, the brew installation of stp doesn't work, and it must be compiled from source. If that's the case, it would be good to add the version requirement somewhere. Perhaps the documentation might be a good place? I'm open to other possibilities too if you have an idea.

@gussmith23
Copy link
Contributor Author

gussmith23 commented Dec 13, 2023

Ready for another pass @sorawee

Please also add documentation here. And please do that for cvc5 and Bitwuzla as well (perhaps as a separate PR). I overlooked this in #260, which was my bad.

Done. I added docs for all four solvers in this PR, hope you don't mind. LMK if you want me to move the CVC5/Bitwuzla stuff to a different PR, but it's mostly copy-paste and find-and-replace.

If I understand correctly, on Mac, the brew installation of stp doesn't work, and it must be compiled from source. If that's the case, it would be good to add the version requirement somewhere. Perhaps the documentation might be a good place? I'm open to other possibilities too if you have an idea.

I added a specific note in the STP docs about this!

@gussmith23
Copy link
Contributor Author

gussmith23 commented Dec 14, 2023 via email

@gussmith23
Copy link
Contributor Author

Ready for another review, tests are running now.

BTW: Yices2 tests didn't actually run before, because of the confusion about binary names. (you can see that CI ran STP tests and then finished!) Will have to explicitly check for the Yices2 tests in the logs once CI completes.


@defproc[(bitwuzla-available?) boolean?]{
Returns true if the Bitwuzla solver is available for use (i.e., Rosette can locate a @tt{bitwuzla} binary).
If this returns @racket[#f], @racket[(bitwuzla)] will not succeed
Copy link
Collaborator

Choose a reason for hiding this comment

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

To make these hyperlinked correctly, you need to add rosette/solver/smt/bitwuzla, rosette/solver/smt/cvc5, etc. under for-label at the top of this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Rosette specifically uses the @tt{yices-smt2} executable, which is the Yices2
solver with its SMTLIB2 frontend enabled.
Note that just building (without installing) Yices2 will produce an executable
named @tt{yices_smt2}. This executable can safely be renamed or symlinked to
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add a sentence before "This executable can safely be renamed" to indicate that the installation step will give the correct executable name. Then, say "Alternatively, the built (but not installed) yices_smt2 executable can safely be renamed ..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@sorawee
Copy link
Collaborator

sorawee commented Dec 14, 2023

I'm sorry. I missed that you also need to change declare-exporting at the top. Right now the links are red underlined because of that.

Screenshot 2023-12-14 at 12 26 36 PM

@gussmith23
Copy link
Contributor Author

Done. Sorry, could have checked that locally myself. Looks fine now, locally.

@sorawee
Copy link
Collaborator

sorawee commented Dec 14, 2023

This looks good to me, thanks!

@sorawee sorawee merged commit edf682d into emina:master Dec 14, 2023
3 checks passed
@gussmith23 gussmith23 deleted the vcanumalla/add-stp branch December 14, 2023 21:44
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.

4 participants