Skip to content

Parser support for assigns contracts on loops #6196

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

Merged
merged 1 commit into from
Jul 13, 2021

Conversation

aalok-thakkar
Copy link

@aalok-thakkar aalok-thakkar commented Jun 23, 2021

  1. Created cprover_loop_contract that has cprover_contract_assigns_opt and loop_invariant_opt.

This adds support for parsing assigns clauses for loops.

  • 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.

@SaswatPadhi SaswatPadhi added aws Bugs or features of importance to AWS CBMC users Code Contracts Function and loop contracts labels Jun 23, 2021
@SaswatPadhi SaswatPadhi requested a review from feliperodri June 23, 2021 19:04
@SaswatPadhi SaswatPadhi changed the title Created cprover_loop_contract Parser support for assigns contracts on loops Jun 23, 2021
@aalok-thakkar aalok-thakkar force-pushed the loop-contract branch 2 times, most recently from ec43ea3 to 83a39db Compare June 23, 2021 20:10
@codecov
Copy link

codecov bot commented Jun 23, 2021

Codecov Report

Merging #6196 (e190b7f) into develop (141f3ce) will increase coverage by 0.16%.
The diff coverage is 86.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #6196      +/-   ##
===========================================
+ Coverage    75.30%   75.47%   +0.16%     
===========================================
  Files         1458     1458              
  Lines       161436   161437       +1     
===========================================
+ Hits        121575   121838     +263     
+ Misses       39861    39599     -262     
Impacted Files Coverage Δ
src/ansi-c/parser.y 79.24% <86.66%> (+16.85%) ⬆️
src/util/simplify_expr.cpp 78.53% <0.00%> (+0.07%) ⬆️
src/ansi-c/c_typecheck_expr.cpp 61.56% <0.00%> (+0.33%) ⬆️
src/ansi-c/parser_static.inc 96.58% <0.00%> (+2.56%) ⬆️
src/util/floatbv_expr.h 73.58% <0.00%> (+2.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 665abf7...e190b7f. Read the comment docs.

Copy link
Contributor

@SaswatPadhi SaswatPadhi left a comment

Choose a reason for hiding this comment

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

Please rebase against current develop and resolve the merge conflict -- it's due to the decreases contract that Long's PR introduced. That should be added to cprover_loop_contract and should be removed from individual iteration statements.

@aalok-thakkar aalok-thakkar force-pushed the loop-contract branch 3 times, most recently from 990341f to c50fb3a Compare June 24, 2021 21:16
Copy link
Collaborator

@feliperodri feliperodri left a comment

Choose a reason for hiding this comment

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

LGTM.

@aalok-thakkar aalok-thakkar force-pushed the loop-contract branch 3 times, most recently from 36419cc to c07518e Compare June 25, 2021 19:18
@SaswatPadhi SaswatPadhi marked this pull request as draft June 26, 2021 04:28
@SaswatPadhi
Copy link
Contributor

Marking this as draft. There are several issues with commits, including merge conflicts.
@aalok-thakkar please fix these.

Copy link
Collaborator

@martin-cs martin-cs left a comment

Choose a reason for hiding this comment

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

Please rebase as there seems to be at least one unrelated commit in this PR.

Copy link
Contributor

@SaswatPadhi SaswatPadhi left a comment

Choose a reason for hiding this comment

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

LGTM, but one minor issue.

@SaswatPadhi SaswatPadhi marked this pull request as ready for review June 28, 2021 17:36
@SaswatPadhi SaswatPadhi requested a review from martin-cs June 28, 2021 17:36
Copy link
Collaborator

@martin-cs martin-cs left a comment

Choose a reason for hiding this comment

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

The patch looks OK but I really don't think we should be adding syntax extensions without test cases that at least demonstrate how to use them.

@aalok-thakkar aalok-thakkar force-pushed the loop-contract branch 4 times, most recently from e166702 to 64b5182 Compare July 1, 2021 18:38
@aalok-thakkar aalok-thakkar requested a review from a team as a code owner July 8, 2021 18:26
@aalok-thakkar aalok-thakkar force-pushed the loop-contract branch 2 times, most recently from dbe9e64 to 5717865 Compare July 8, 2021 18:48
Comment on lines 12 to 13
This test shows that the current implementation only supports
parsing and still uses alias analysis. Fixing this is a part
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you mean by "current implementation only supports parsing and still uses alias analysis"? Could you elaborate?

Copy link
Author

Choose a reason for hiding this comment

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

This PR only adds parsing to it, and assertion statements are yet not generated. Therefore, this proof does not go through.

Copy link
Contributor

Choose a reason for hiding this comment

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

What Aalok is saying is that, he added this test just to check the parsing support (and address Martin's comment regarding usage of assigns clause). But it's not considered during instrumentation, so this test which he obtained from my bug report, still fails as before.

@aalok-thakkar aalok-thakkar force-pushed the loop-contract branch 4 times, most recently from 6d7db3d to 1868741 Compare July 9, 2021 15:17
Copy link
Collaborator

@feliperodri feliperodri left a comment

Choose a reason for hiding this comment

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

LGTM.

1. Renamed cprover_contract_assigns_opt to cprover_contract_assigns.
2. Created cprover_contract_assigns_opt which has a unit production and cprover_contract_assigns.
3. Renamed cprover_decreases_opt to cprover_contract_decreases_opt.
4. Renamed loop_invariant to cprover_contract_loop_invariant.
5. Renamed loop_invariant_list to cprover_contract_loop_invariant_list.
6. Renamed loop_invariant_list_opt to cprover_contract_loop_invariant_list_opt.
7. Added cprover_contract_assigns_opt to iteration statement.
8. Created the regression test invar_assigns_alias_analysis (KNOWNBUG).
9. Created the regression test invar_assigns_opt (CORE).
10. Created the regression test invar_assigns_empty (CORE).

This adds parser support for assigns contracts on loops.
@feliperodri
Copy link
Collaborator

@martin-cs could you give another review on this PR?

Copy link
Collaborator

@martin-cs martin-cs left a comment

Choose a reason for hiding this comment

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

Sure @feliperodri . There are now tests so no longer blocking.

@feliperodri feliperodri merged commit 9f19902 into diffblue:develop Jul 13, 2021
SaswatPadhi added a commit to aalok-thakkar/cbmc that referenced this pull request Nov 19, 2021
In this commit we implement the parsing actions, type checking and
conversion routines for assigns clauses on loops. The syntax rules along
with regressions tests were introduced in an earlier PR: diffblue#6196.
SaswatPadhi added a commit to aalok-thakkar/cbmc that referenced this pull request Nov 19, 2021
In this commit we implement the parsing actions, type checking and
conversion routines for assigns clauses on loops. The syntax rules along
with regressions tests were introduced in an earlier PR: diffblue#6196.
SaswatPadhi added a commit to aalok-thakkar/cbmc that referenced this pull request Nov 23, 2021
In this commit we implement the parsing actions, type checking and
conversion routines for assigns clauses on loops. The syntax rules along
with regressions tests were introduced in an earlier PR: diffblue#6196.
vmihalko pushed a commit to vmihalko/cbmc that referenced this pull request Jan 24, 2022
In this commit we implement the parsing actions, type checking and
conversion routines for assigns clauses on loops. The syntax rules along
with regressions tests were introduced in an earlier PR: diffblue#6196.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws Bugs or features of importance to AWS CBMC users C Front End Code Contracts Function and loop contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants