Skip to content

Check inferred loop assigns clauses instead of blindly trusting them #6509

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

Conversation

remi-delmas-3000
Copy link
Collaborator

@remi-delmas-3000 remi-delmas-3000 commented Dec 7, 2021

When no assigns clause is specified by the user for a loop, we infer one by inspecting assignment present in the loop body.

These inferred targets were havocked but not checked for correctness. This PR uses the inferred targets to instrument the loop as if it were user-provided.

  1. In some tests the inferred assigns clause did not pass verification so an explicit assigns clause was added;
  2. major performance regression on regression/contracts/quantifiers-loop-02;
  3. For remaining tests, we add the assigned targets in the tests.
  • 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.

@remi-delmas-3000 remi-delmas-3000 self-assigned this Dec 7, 2021
@remi-delmas-3000 remi-delmas-3000 added aws Bugs or features of importance to AWS CBMC users aws-high enhancement labels Dec 7, 2021
@codecov
Copy link

codecov bot commented Dec 7, 2021

Codecov Report

Merging #6509 (ef52bb9) into develop (5f33908) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #6509   +/-   ##
========================================
  Coverage    75.98%   75.98%           
========================================
  Files         1578     1578           
  Lines       180910   180920   +10     
========================================
+ Hits        137467   137477   +10     
  Misses       43443    43443           
Impacted Files Coverage Δ
src/goto-instrument/contracts/contracts.cpp 95.11% <100.00%> (+0.07%) ⬆️

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 e783d13...ef52bb9. Read the comment docs.

@remi-delmas-3000 remi-delmas-3000 force-pushed the dont-trust-inferred-loop-assigns-clauses branch 2 times, most recently from ff2ddce to 245edcf Compare December 7, 2021 14:49
Comment on lines +293 to +296
// TODO: Should we add the automatically detected local static variables
// too ? (they are present in loop_assigns but not in assigns_clause, and
// they are not necessarily touched by the loop).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Who might be able to answer that question, or how would we find out the answer?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should track this in a Github issue as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This needs careful analysis, keeping it here.

@tautschnig
Copy link
Collaborator

One question about the PR note:

major performance regression on regression/contracts/quantifiers-loop-02;

Can you provide more details on this?

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.

Only minor comments.

Comment on lines +293 to +296
// TODO: Should we add the automatically detected local static variables
// too ? (they are present in loop_assigns but not in assigns_clause, and
// they are not necessarily touched by the loop).
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should track this in a Github issue as well.

@remi-delmas-3000
Copy link
Collaborator Author

remi-delmas-3000 commented Dec 11, 2021

One question about the PR note:

major performance regression on regression/contracts/quantifiers-loop-02;

Can you provide more details on this?

It seems that instrumenting the body of the loop adds a lot of extra complexity that the SMT backend cannot handle combined with the quantifier (arithmetic and pointer reasoning) (had to ctrl-c the analysis after 20+ minutes). I reduced the MAX_ARRAY_SIZE to make it pass.

@remi-delmas-3000 remi-delmas-3000 force-pushed the dont-trust-inferred-loop-assigns-clauses branch from 245edcf to ef52bb9 Compare December 11, 2021 01:07
@tautschnig tautschnig merged commit eeddb3f into diffblue:develop Dec 11, 2021
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 aws-high enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants