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

[SMTChecker] Fix CHCSmtLib2Interface #11289

Merged
merged 9 commits into from
May 27, 2021
Merged

Conversation

leonardoalt
Copy link
Member

@leonardoalt leonardoalt commented Apr 22, 2021

Previously CHCSmtLib2Interface was using z3's the rule language sugar to describe Horn clauses. This PR changes it to the proper smtlib2 Horn logic which other solvers can also understand. That is the main fix and is coded in the first commit.
The other commits add other small fixes that I felt were small enough to be just single commits in this same PR:

  • implies is not smtlib2, => is the right operator
  • negative number literals cannot be represented by -10 in smtlib2. It needs to be either (- 10) or (0 - 10). Here I went with the latter to avoid overloading the minus operator.
  • In pure smtlib2, / is actually Real division, whereas we want Integer division which is div
  • gasleft() should be gasleft
  • Manual two's complement should use bvnat first

With these changes, the queries given by CHCSmtLib2Interface can actually be read by any Horn solver that reads smtlib2.
We don't have direct tests for that, because:

  • They are tested in the solc-js tests, which is part of t_ems BUT
  • Since we have z3 statically linked in soljson.js, the queries never actually run. This will be fixed next but I need the fixes here to go in first. I tested them locally and it works fine.

@leonardoalt leonardoalt force-pushed the smt_fix_chcsmtlib2interface branch from a36d55f to 8a444ba Compare April 29, 2021 12:56
@leonardoalt leonardoalt force-pushed the smt_fix_chcsmtlib2interface branch from 8a444ba to 981bca9 Compare May 18, 2021 17:47
@leonardoalt leonardoalt marked this pull request as ready for review May 18, 2021 17:48
Expression(s256 const& _number): Expression(_number.str(), {}, SortProvider::sintSort) {}
Expression(bigint const& _number): Expression(_number.str(), {}, SortProvider::sintSort) {}
Expression(s256 const& _number): Expression(_number.sign() >= 0 ? _number.str() : "-", _number.sign() >= 0 ? std::vector<Expression>{} : std::vector<Expression>{Expression(size_t(0)), u256(-_number)}, SortProvider::sintSort) {}
Expression(bigint const& _number): Expression(_number.sign() >= 0 ? _number.str() : "-", _number.sign() >= 0 ? std::vector<Expression>{} : std::vector<Expression>{Expression(size_t(0)), u256(-_number)}, SortProvider::sintSort) {}
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about the lines above. The fix is correct but looks kinda ugly. Open to suggestions.

@@ -118,7 +118,7 @@ SortPointer smtSort(frontend::Type const& _type)
else
tupleName = arrayType->baseType()->toString(true);

tupleName += "[]";
tupleName += "_array";
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed because smtlib2 doesn't like identifiers with []

libsmtutil/SolverInterface.h Outdated Show resolved Hide resolved
@@ -300,7 +300,7 @@ class Expression
friend Expression operator/(Expression _a, Expression _b)
{
auto intSort = _a.sort;
return Expression("/", {std::move(_a), std::move(_b)}, intSort);
return Expression("div", {std::move(_a), std::move(_b)}, intSort);
Copy link
Member

Choose a reason for hiding this comment

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

Aren't the changes in the commit redundant?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why redundant?

Copy link
Member

Choose a reason for hiding this comment

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

Because we end up converting "div" to / in both z3interface and cvc4interface. Why do we call it div?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because smtlib2 uses the name of the operator when creating the queries. Since we want it to use div, it makes sense for all solvers to read div

Copy link
Member Author

Choose a reason for hiding this comment

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

@hrkrshnn only this discussion left to finish I think

Copy link
Member

Choose a reason for hiding this comment

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

Do we currently use pure smt2 queries?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an option, via SmtLib2Interface or CHCSmtLib2Inteface

libsmtutil/SMTLib2Interface.h Outdated Show resolved Hide resolved
libsolidity/formal/CHC.cpp Show resolved Hide resolved
libsmtutil/CHCSmtLib2Interface.h Show resolved Hide resolved
libsmtutil/CHCSmtLib2Interface.cpp Show resolved Hide resolved
libsmtutil/CHCSmtLib2Interface.cpp Show resolved Hide resolved
@leonardoalt leonardoalt force-pushed the smt_fix_chcsmtlib2interface branch from dc919ba to c570b63 Compare May 20, 2021 16:08
Copy link
Member

@hrkrshnn hrkrshnn left a comment

Choose a reason for hiding this comment

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

Looks good. Some minor comments.

libsmtutil/SMTLib2Interface.h Show resolved Hide resolved
libsmtutil/SolverInterface.h Outdated Show resolved Hide resolved
@leonardoalt leonardoalt force-pushed the smt_fix_chcsmtlib2interface branch from c570b63 to f7b045b Compare May 26, 2021 21:03
@leonardoalt
Copy link
Member Author

@hrkrshnn updated & rebased

Copy link
Member

@hrkrshnn hrkrshnn left a comment

Choose a reason for hiding this comment

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

The last commit may be squashed, but okay otherwise too.

@leonardoalt
Copy link
Member Author

Yea the last commit has parts that would have to into different commits, so I would merge as is since it's not too much.

@leonardoalt leonardoalt merged commit a879984 into develop May 27, 2021
@leonardoalt leonardoalt deleted the smt_fix_chcsmtlib2interface branch May 27, 2021 09:50
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.

2 participants