Skip to content
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

Function Contracts: Support for defining and checking requires and ensures clauses #2655

Merged
merged 41 commits into from
Sep 7, 2023

Conversation

JustusAdam
Copy link
Contributor

@JustusAdam JustusAdam commented Aug 3, 2023

Description of changes:

Adds simple function contracts in the form of requires and ensures clauses to Kani.

A contract for e.g. a function f can be checked by creating a harness and adding the proof_for_contract(f) annotation.

This is the first PR of a series that implement the function contact RFC which is under review (#2620) and can be seen rendered here

Contracts as implemented here cannot satisfactorily reason about unsafe memory manipulations. It cannot reason about write sets or mutation of values (history variables). To communicate this to the user any attempt to use a contract on a function which receives *mut pointers or returning any kind of pointer (*const or *mut) will result in a compile time error.
The error is restricted to pointers, because memory validity of references is enforced by the Rust type system. The lack of capability to reason about history is an issue for &mut references, however this only makes certain policies inexpressible, whereas raw pointer manipulation causes unsoundness, since the bounds of the manipulation are not checked by this implementation.

References can technically also be abused by casting them to raw pointers inside the function, which is not checked. This would require an pass over the actual body of the function and all functions it calls. Such a check might be desirable also to catch whether static globals are manipulated.

Resolved issues:

Resolves #2596
Resolves #2595

Related RFC:

Tracking issue for function contracts #2652

Call-outs:

There are some auxiliary changes here that are not strictly necessary.

  • The testing framework compiletest has been changed so that multiple "expected" tests can be located in the same directory. In this case the runner expects there to be for each testfile.rs a testfile.expected file
  • KaniAttributes have been encapsulated into a struct that is exported but opaque. This is not strictly necessary for this PR, but in later contract work the attributes will need to be requeried later in the compiler which would be inefficient with the way they were previously used. This leaves two options: Expose the internal attribute map, or expose the attribute map in encapsulated fashion (which is what is done here).
  • The sysroot module in kani_macros has been refactored into a new file. This is because it is significantly larger now and mostly because my editor chokes on scrolling through it otherwise (because it grays it out which I guess is bad for the rendering engine).

Testing:

  • How is this change tested? A suite of "expected" test cases is provided in the "function-contract" directory

  • Is this a refactor change? No

Checklist

  • Each commit message has a non-empty body, explaining why the change was made
  • Methods or procedures are documented
  • Regression or unit tests are included, or existing tests cover the modified code
  • My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@JustusAdam JustusAdam requested a review from a team as a code owner August 3, 2023 00:37
@JustusAdam JustusAdam changed the title Everything needed for requires/ensures checking Function Contracts: Support for defining and checking requires and ensures clauses Aug 3, 2023
@JustusAdam JustusAdam added this to the Function Contracts MVP milestone Aug 3, 2023
@JustusAdam JustusAdam self-assigned this Aug 3, 2023
Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

library/kani/src/lib.rs Outdated Show resolved Hide resolved
library/kani_macros/src/lib.rs Outdated Show resolved Hide resolved
library/kani_macros/src/sysroot.rs Outdated Show resolved Hide resolved
library/kani_macros/src/sysroot.rs Outdated Show resolved Hide resolved
library/kani_macros/src/sysroot.rs Outdated Show resolved Hide resolved
kani-driver/src/args/mod.rs Outdated Show resolved Hide resolved
kani-driver/src/args/mod.rs Outdated Show resolved Hide resolved
kani-driver/src/args/mod.rs Outdated Show resolved Hide resolved
kani-compiler/src/kani_compiler.rs Outdated Show resolved Hide resolved
kani-compiler/src/codegen_cprover_gotoc/overrides/hooks.rs Outdated Show resolved Hide resolved
@JustusAdam JustusAdam marked this pull request as draft August 4, 2023 01:43
@JustusAdam
Copy link
Contributor Author

Changing this back to a draft because I've noticed an issue with the "functions wrapping" approach. Will have to change the macro code gen a bit.

@JustusAdam JustusAdam marked this pull request as ready for review August 4, 2023 04:52
@JustusAdam
Copy link
Contributor Author

Okay the pivot has happened. The generation algorithm has been changed and should now not exhibit the weird drop issue.

There is still a lingering drop issue where if you move a value with heap data such that it's drop gets called early it causes a double-free.

Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

@JustusAdam can you please revert the changes to kani_macro/lib.rs and move the refactoring of attributes to a separate PR. It will make reviewing this PR much easier.

Even though the compiletest changes should've be a different PR, at least the changes are fairly independent from everything else, and small enough. I've reviewed them already, so I'm OK keeping them here.

library/kani_macros/src/lib.rs Show resolved Hide resolved
library/kani_macros/src/sysroot.rs Outdated Show resolved Hide resolved
kani-driver/src/args/mod.rs Outdated Show resolved Hide resolved
kani-compiler/src/kani_middle/attributes.rs Outdated Show resolved Hide resolved
kani-compiler/src/kani_middle/attributes.rs Show resolved Hide resolved
tools/compiletest/src/runtest.rs Outdated Show resolved Hide resolved
library/kani_macros/src/sysroot.rs Outdated Show resolved Hide resolved
library/kani_macros/src/sysroot.rs Outdated Show resolved Hide resolved
library/kani_macros/src/sysroot.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

I was reviewing the tests. They are looking good, thanks for putting the effort.

I think there are a few things that we are not covering yet, but we can leave them for a follow up PR. Things like:

  • Multiple requires and ensures
  • Contract doesn't get checked in regular harnesses
  • Only target contract is checked in the proof_for_contract harnesses.

kani-compiler/src/kani_middle/attributes.rs Show resolved Hide resolved
library/kani/src/lib.rs Outdated Show resolved Hide resolved
@JustusAdam
Copy link
Contributor Author

JustusAdam commented Aug 23, 2023

That last failing check is an out-of-disk error that I don't believe these changes are responsible for.

@celinval
Copy link
Contributor

That last failing check is an out-of-disk error that I don't believe these changes are responsible for.

I'm looking into that. I'll probably fix that as part of #2703

Copy link
Contributor

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

Well done! Only minor comments over documentation and clean ups.

kani-compiler/src/codegen_cprover_gotoc/overrides/hooks.rs Outdated Show resolved Hide resolved
kani-compiler/src/codegen_cprover_gotoc/overrides/hooks.rs Outdated Show resolved Hide resolved
kani-compiler/src/kani_middle/attributes.rs Outdated Show resolved Hide resolved
kani-compiler/src/kani_middle/attributes.rs Show resolved Hide resolved
kani-compiler/src/kani_middle/attributes.rs Outdated Show resolved Hide resolved
tests/expected/function-contract/gcd_failure_code.expected Outdated Show resolved Hide resolved
tests/expected/function-contract/out.txt Outdated Show resolved Hide resolved
JustusAdam and others added 2 commits September 6, 2023 15:12
Co-authored-by: Felipe R. Monteiro <rms.felipe@gmail.com>
Copy link
Contributor Author

@JustusAdam JustusAdam left a comment

Choose a reason for hiding this comment

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

@feliperodri Thanks for the review. I mostly just adopted your changes but a few I voiced objections to in the comments.

@JustusAdam JustusAdam merged commit f834be7 into model-checking:main Sep 7, 2023
12 checks passed
@JustusAdam JustusAdam deleted the simple-contacts-checking branch September 7, 2023 17:30
@JustusAdam JustusAdam mentioned this pull request Sep 7, 2023
4 tasks
JustusAdam added a commit that referenced this pull request Sep 27, 2023
### Description of changes: 

Expands the contract features introduced in #2655 by completing the
checking with stubbing/replacement capabilities.

### Resolved issues:

Resolves #2621 

### Related RFC: [0009 Function
Contracts](https://model-checking.github.io/kani/rfc/rfcs/0009-function-contracts.html)

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.

---------

Co-authored-by: Celina G. Val <celinval@amazon.com>
Co-authored-by: Felipe R. Monteiro <rms.felipe@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: In Progress
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Unstable gating for function contracts Refactor contract attribute validation into check_attributes
3 participants