Skip to content

Add workaround for issue described in #6168. #6340

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
Sep 15, 2021

Conversation

NlightNFotis
Copy link
Contributor

This fixes the infinite unwinding by adding calls to do_indirect_call_and_rtti_removal()
before the contracts code invocation in goto-instrument/.

This is not yet finished, as it will need a test-case attached, but this is pushed so that
it can garner some initial comments regarding the approach.

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

@NlightNFotis
Copy link
Contributor Author

Hi @SaswatPadhi, would it be possible to have a look at this PR? It works-around the issue described in #6168 using Martin's suggestion.

However, I couldn't get it to work without introducing a call to apply_loop_contracts() in enforce_contracts(). I'm not sure about this step, but this is what was preventing the change in latest develop from working (it worked cleanly against the version described in the issue, cbmc-5.32.0).

Besides, I'm not sure if that addition is changing the semantics of your contract code. It seems that in 9cdca48#diff-1ff6abca57de19a68456666ed5c25ce09fcaae20fdc9c2b109cb60cb311c183aL1208 the call to apply_loop_contracts() is removed from a branch that could be exercised, without an equivalent call being reachable elsewhere, but that's just my understanding. Please let me know if there are issues with the current approach.

@SaswatPadhi
Copy link
Contributor

Thanks for implementing the workaround, @NlightNFotis!

We separated the function contract flags and loop contracts flags recently, so --enforce-all-contracts only enforces function contracts (for all functions). Loop contracts don't have separate enforcement and replacement steps, just a single apply step using the flag --apply-loop-contracts.

So you could replace the --enforce-all-contracts flag with --apply-loop-contracts and undo the new call to apply_loop_contracts(). Thanks!

@SaswatPadhi SaswatPadhi added aws Bugs or features of importance to AWS CBMC users bugfix Code Contracts Function and loop contracts labels Sep 14, 2021
This fixes the infinite unwinding by adding calls to `do_indirect_call_and_rtti_removal()`
before the contracts code invocation in `goto-instrument/`.
This also demonstrates the issue being fixed.
@codecov
Copy link

codecov bot commented Sep 15, 2021

Codecov Report

Merging #6340 (de9e891) into develop (c06ccf1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #6340   +/-   ##
========================================
  Coverage    75.89%   75.90%           
========================================
  Files         1515     1515           
  Lines       163971   163990   +19     
========================================
+ Hits        124452   124469   +17     
- Misses       39519    39521    +2     
Impacted Files Coverage Δ
.../goto-instrument/goto_instrument_parse_options.cpp 68.29% <100.00%> (+0.03%) ⬆️
src/goto-instrument/contracts/contracts.cpp 93.87% <0.00%> (-0.92%) ⬇️
src/goto-instrument/contracts/assigns.cpp 96.66% <0.00%> (+4.71%) ⬆️

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 14d35cc...de9e891. Read the comment docs.

@NlightNFotis NlightNFotis marked this pull request as ready for review September 15, 2021 12:25
@NlightNFotis
Copy link
Contributor Author

Thanks @SaswatPadhi, the latest suggestions have now been implemented, and I moved the test to the regression/contracts folder.

This patch should now fix the issue you are facing in #6168 if you call goto-instrument --apply-loop-contracts.

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.

Awesome! Thank you so much, @NlightNFotis.

Looks like that resolved all of the CI errors too.

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 bugfix Code Contracts Function and loop contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants