Skip to content

Cleanup regression tests for quantified contracts #6145

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 2 commits into from
May 25, 2021

Conversation

SaswatPadhi
Copy link
Contributor

@SaswatPadhi SaswatPadhi commented May 24, 2021

Cleaned up some the quantified contracts, changed some KNOWNBUG cases to CORE, added regex patterns in descriptions to make sure we catch expected failures (and don't ignore unexpected ones) etc.

Please review individual commits. The second commit is just a renaming: *-01 -> *-enforce and *-02 -> *-replace. Some tests didn't adhere to this convention (and were double testing the same context, enforcement / replacement), so I changed the context appropriately and now I think we are reasonably exhaustive on corner cases.

  • Each commit message has a non-empty body, explaining why the change was made.
  • n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • n/a 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).
  • n/a My commit message includes data points confirming performance improvements (if claimed).
  • n/a My PR is restricted to a single feature or bugfix.
  • n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@SaswatPadhi SaswatPadhi added Tests aws Bugs or features of importance to AWS CBMC users labels May 24, 2021
@SaswatPadhi SaswatPadhi requested a review from feliperodri May 24, 2021 20:52
@SaswatPadhi SaswatPadhi self-assigned this May 24, 2021
@SaswatPadhi SaswatPadhi force-pushed the quantifiers-tests-fix branch from a503b87 to 15e3336 Compare May 24, 2021 20:57
@SaswatPadhi SaswatPadhi force-pushed the quantifiers-tests-fix branch from 15e3336 to 303bcb5 Compare May 24, 2021 21:23
@SaswatPadhi
Copy link
Contributor Author

@feliperodri Please review the first commit. The second one is just for renaming.

Comment on lines 32 to 36
for(int i = 0; i <= MAX_LEN; i++)
{
if(i < len)
found_zero |= (arr[i] == 0);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are not proving these programs are correct, because we're not unrolling these loops. Thus, we have two options here:

  1. Add invariants to these loops;
  2. Update our regression-test script to also call CBMC with the correct bounds and --unwinding-assertions;

I'd prefer option (1). In any case, we can create a GitHub issue for this now and tackle it on a separate PR (for all test cases).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But all loops in CBMC are always unrolled. This one is too, you can see in the CBMC output.

With symbolic bounds the unrolling just never stops, so we give --unwind to limit unrolling and then --unwinding-assertions to make sure our limited unrolling is sound.

Another way to stop the unrolling is to provide an invariant, and then we just reason about a single arbitrary iteration.

Here, since the loop bounds are constants (0 and 128), the unrolling stops automatically, and is always sound since there is nothing more to unroll (i.e. we didn't limit anything).

range for i. However, in the current implementation of quantifier handling,
the (0 <= i && i < 10) statement is not interpreted as an explicit range, but
instead, as part of a logic formula, which causes verification to succeed.
This is fully supported (without requiring full unrolling) with the SAT backend.
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 without requiring full unrolling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be there is a better way to phrase this. I was being lazy.

I meant to say that exists x . (L <= x <= U) && P(x) is not unrolled to P(L) \/ P(L+1) \/ ... \/ P(U-1) \/ P(U) in a negative context.

In a negative context symbolic or even no explicit bounds at all would work for exists, e.g. assume(exists x . P(x)) works fine because CBMC just introduces a temporary variable x and then assume(P(x)).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@SaswatPadhi SaswatPadhi force-pushed the quantifiers-tests-fix branch from 303bcb5 to ea71d66 Compare May 24, 2021 22:56
@SaswatPadhi SaswatPadhi requested a review from feliperodri May 24, 2021 22:56
@SaswatPadhi SaswatPadhi marked this pull request as ready for review May 24, 2021 23: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.

@feliperodri feliperodri merged commit f82fea4 into diffblue:develop May 25, 2021
@SaswatPadhi SaswatPadhi deleted the quantifiers-tests-fix branch May 25, 2021 13:51
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 Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants