Skip to content

Conversation

esteffin
Copy link
Contributor

This PR adds support for byte_extract_exprt and byte_update_exprt for the new incremental SMT backend.

  • Each commit message has a non-empty body, explaining why the change was made.
  • Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@esteffin esteffin force-pushed the esteffin/byte-extraction-update-2 branch 6 times, most recently from 10eac99 to 6fdc582 Compare November 3, 2022 12:49
@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Base: 78.24% // Head: 78.28% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (9ec53d1) compared to base (6938faa).
Patch coverage: 99.61% of modified lines in pull request are covered.

❗ Current head 9ec53d1 differs from pull request most recent head 424bed5. Consider uploading reports for the commit 424bed5 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7285      +/-   ##
===========================================
+ Coverage    78.24%   78.28%   +0.03%     
===========================================
  Files         1642     1642              
  Lines       189752   189976     +224     
===========================================
+ Hits        148479   148714     +235     
+ Misses       41273    41262      -11     
Impacted Files Coverage Δ
src/solvers/smt2/smt2_conv.cpp 66.38% <93.33%> (+0.02%) ⬆️
...c/solvers/smt2_incremental/convert_expr_to_smt.cpp 82.91% <100.00%> (-0.14%) ⬇️
...ncremental/smt2_incremental_decision_procedure.cpp 96.81% <100.00%> (+0.09%) ⬆️
...t/solvers/smt2_incremental/convert_expr_to_smt.cpp 99.69% <100.00%> (-0.02%) ⬇️
...ncremental/smt2_incremental_decision_procedure.cpp 98.75% <100.00%> (+0.65%) ⬆️
src/solvers/floatbv/float_bv.cpp 53.45% <0.00%> (-0.65%) ⬇️
src/goto-programs/goto_trace.h 100.00% <0.00%> (ø)
... and 15 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@esteffin esteffin force-pushed the esteffin/byte-extraction-update-2 branch from 6fdc582 to 48a85bd Compare November 3, 2022 14:58
Copy link
Contributor

@thomasspriggs thomasspriggs left a comment

Choose a reason for hiding this comment

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

This a partial review. I haven't reviewed the tests yet.

@esteffin esteffin force-pushed the esteffin/byte-extraction-update-2 branch 6 times, most recently from 5e3004e to 8f62a0e Compare November 7, 2022 17:07
@esteffin esteffin marked this pull request as ready for review November 7, 2022 17:08
@esteffin esteffin force-pushed the esteffin/byte-extraction-update-2 branch from 8f62a0e to 8b3820f Compare November 7, 2022 17:15
Copy link
Contributor

@thomasspriggs thomasspriggs left a comment

Choose a reason for hiding this comment

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

I think we should get the following sorted before merging -

  1. Fixing the one unit test which is squashed into the wrong commit.
  2. Adding commit message bodies. The commits now (mostly) say what they do, which is great. But some of the pre-requisite fixes are still missing the context as to why they are needed. I am happy to go through adding these with you.

smt_bit_vector_theoryt::make_or(
smt_bit_vector_theoryt::make_and(
original_int_term,
smt_bit_vector_constant_termt{18446744073709486335UL, 64}),
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ I think this mask would be easier to interpret as a hex literal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

smt_bit_vector_theoryt::make_and(
original_int_term,
smt_bit_vector_constant_termt{18446744073709486335UL, 64}),
smt_bit_vector_constant_termt{2816UL, 64}));
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ I think the literal 2816UL may be easier to read as 11UL << 8. This would make it clearer that it corresponds to the 11 in the input expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

}
}

TEST_CASE(
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Looks like a good test to me.

@esteffin esteffin force-pushed the esteffin/byte-extraction-update-2 branch from 8b3820f to 9ec53d1 Compare November 8, 2022 17:35
Enrico Steffinlongo added 2 commits November 8, 2022 17:39
As `with_exprt` can be used as a multy-ary operatior (with more than 3
arguments) we need to introduce an index smt_identifier_termt for each
index used by the expression.
Lowering of the byte extraction and byte update expressions can yield
non-integer bitvector typed operands.
Because we do not require a numeric interpretation of them we can relax
the requirement of integer-typed operands for bitwise operators.
@esteffin esteffin force-pushed the esteffin/byte-extraction-update-2 branch from 9ec53d1 to 6ad772c Compare November 8, 2022 17:50
Enrico Steffinlongo added 2 commits November 8, 2022 18:02
When the input program includes non-aligned memory accesses the decision
procedure is passed expressions including byte_extract and byte_update
operators.
To support such programs in the decision procedure we need to lower such
operators to operators we support.
@esteffin esteffin force-pushed the esteffin/byte-extraction-update-2 branch from 6ad772c to 424bed5 Compare November 8, 2022 18:03
@thomasspriggs thomasspriggs merged commit 5b95d2b into diffblue:develop Nov 8, 2022
@thomasspriggs thomasspriggs deleted the esteffin/byte-extraction-update-2 branch November 8, 2022 20:01
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.

3 participants