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

Enable contracts in associated functions #3363

Merged
merged 21 commits into from
Aug 1, 2024

Conversation

celinval
Copy link
Contributor

Contracts could not be used with associated function unless they used Self. This is because at a proc-macro level, we cannot determine if we are inside a associated function or not, except in cases where Self is used. In cases where the contract generation could not identify an associated function, it would generate incorrect call, triggering a compilation error.

Another problem with our encoding is that users could not annotate trait implementations with contract attributes. This would try to create new functions inside the trait implementation context, which is not allowed.

In order to solve these issues, we decided to wrap contract logic using closures instead of functions. See the discussion in the original issue (#3206) for more details.

The new solution is still split in two:

  1. The proc-macro will now expand the code inside the original function to encode all possible scenarios (check, replace, recursive check, and original body).
  2. Instead of using stub, we introduced a new transformation pass that chooses which scenario to pick according to the target harness configuration, and cleanup unused logic.

The expanded function will have the following structure:

#[kanitool::recursion_check = "__kani_recursion_check_modify"]
#[kanitool::checked_with = "__kani_check_modify"]
#[kanitool::replaced_with = "__kani_replace_modify"]
#[kanitool::inner_check = "__kani_modifies_modify"]
fn name_fn(ptr: &mut u32) {
    #[kanitool::fn_marker = "kani_register_contract"]
    pub const fn kani_register_contract<T, F: FnOnce() -> T>(f: F) -> T {
        kani::panic("internal error: entered unreachable code: ")
    }
    let kani_contract_mode = kani::internal::mode();
    match kani_contract_mode {
        kani::internal::RECURSION_CHECK => {
            #[kanitool::is_contract_generated(recursion_check)]
            let mut __kani_recursion_check_name_fn = || { /* recursion check body */ };
            kani_register_contract(__kani_recursion_check_modify)
        }
        kani::internal::REPLACE => {
            #[kanitool::is_contract_generated(replace)]
            let mut __kani_replace_name_fn = || { /* replace body */ };
            kani_register_contract(__kani_replace_name_fn)
        }
        kani::internal::SIMPLE_CHECK => {
            #[kanitool::is_contract_generated(check)]
            let mut __kani_check_name_fn = || { /* check body */ };
            kani_register_contract(__kani_check_name_fn)
        }
        _ => { /* original body */ }
    }
}

In runtime, kani::internal::mode() will return kani::internal::ORIGINAL, which runs the original body. The transformation will replace this call by a different assignment in case the function needs to be replaced. The body of the unused closures will be replaced by a UNREACHABLE statement to avoid unnecessary code to be analyzed.

This is still fairly hacky, but hopefully we can cleanup this logic once Rust adds contract support. 🤞

Resolves #3206

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

Initial setup works with only one annotation. Next steps:

1. Adjust compiler to search for the closures.
2. Handle multiple annotations.
This will simplify transformation quite a bit. We should consider only
adding the recursion closure if `#[recursion]` tag is added to the
function.
Just over 50% of tests is passing. Better than before. :)

 - Need to handle `self`
Still need to fix Receiver handling and closure transformation.
62 passing, 16 failing
- TODO: Remove all the debug stuff
Conflicts:
    - kani-compiler/src/codegen_cprover_gotoc/codegen/contract.rs
    - kani-compiler/src/kani_middle/attributes.rs
    - kani-compiler/src/kani_middle/transform/contracts.rs
    - library/kani/src/internal.rs
    - library/kani_macros/src/sysroot/contracts/check.rs
@celinval celinval requested a review from a team as a code owner July 20, 2024 02:11
@github-actions github-actions bot added the Z-BenchCI Tag a PR to run benchmark CI label Jul 20, 2024
…ethod

Conflicts:
    library/kani/src/lib.rs
    library/kani_core/src/lib.rs
Also disable the Arc harness.
- Simplify the Pointer structure a bit more
Stable constant functions in the standard library can only invoke stable
constant functions. Instead of adding the wrapper in the Kani library,
expand it as an internal function which inherit the attributes of the
function being annotated.
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.

First batch of reviews. So far the test looks good and we have confidence in the pre-existing tests that the refactoring did not break any current behavior. I'm gonna give another pass in the instrumentation.

Copy link
Contributor

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

I wonder if we couldn't vastly simplify the generated code by just inlining the closures into the match and pick a branch based on mode? Since we're already replacing the mode call with a constant, can't we just run like rustc's folding + dead code removal to select the branch for us?

@celinval
Copy link
Contributor Author

celinval commented Jul 31, 2024

I wonder if we couldn't vastly simplify the generated code by just inlining the closures into the match and pick a branch based on mode? Since we're already replacing the mode call with a constant, can't we just run like rustc's folding + dead code removal to select the branch for us?

Actually, we cannot do that today unless if we want to restrict contract of constant functions to only contain constant code. I remember bumping into this issue while trying to verify the copy intrinsics.

@feliperodri that is also the reason why we need the kani_register_contract function, i.e., we cannot invoke closures from constant functions.

This restriction is documented in the bootstrap.rs file:

// Dummy function used to force the compiler to capture the environment.
// We cannot call closures inside constant functions.
// This function gets replaced by `kani::internal::call_closure`.
#[inline(never)]
#[kanitool::fn_marker = "kani_register_contract"]
const fn kani_register_contract<T, F: FnOnce() -> T>(f: F) -> T {
    unreachable!()
}

@feliperodri feliperodri added this to the Function Contracts milestone Jul 31, 2024
@tautschnig tautschnig added the Z-Contracts Issue related to code contracts label Aug 1, 2024
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.

🚢 it!

…ethod

Conflicts:
   - kani-compiler/src/kani_middle/attributes.rs
   - kani-driver/src/call_goto_instrument.rs
   - library/kani_macros/src/sysroot/contracts/mod.rs
@celinval celinval enabled auto-merge (squash) August 1, 2024 19:20
@celinval celinval merged commit e980aa2 into model-checking:main Aug 1, 2024
27 checks passed
tautschnig added a commit to tautschnig/kani that referenced this pull request Aug 5, 2024
* Remove `gen_function_local_variable` and `initializer_fn_name`, the
  last uses of both of which were removed in model-checking#3305.
* Remove `arg_count`, which was introduced in model-checking#3363, but not actually
  used.
* Remove `insert_bb`, which was introduced in model-checking#3382, but not actually
  used.

The toolchain upgrade to 2024-08-04 includes several bugfixes to
dead-code analysis in rustc, explaining why we the recent PRs as listed
above weren't flagged before for introducing dead code.

Resolves: model-checking#3411
github-merge-queue bot pushed a commit that referenced this pull request Aug 5, 2024
* Remove `gen_function_local_variable` and `initializer_fn_name`, the
last uses of both of which were removed in #3305.
* Mark `arg_count`, which was introduced in #3363, as `allow(dead_code)`
as it will soon be used.
* Mark `insert_bb`, which was introduced in #3382, as `allow(dead_code)`
as it will soon be used.

The toolchain upgrade to 2024-08-04 includes several bugfixes to
dead-code analysis in rustc, explaining why we the recent PRs as listed
above weren't flagged before for introducing dead code.

Resolves: #3411

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.
github-merge-queue bot pushed a commit that referenced this pull request Aug 9, 2024
## 0.54.0

### Major Changes
* We added support for slices in the `#[kani::modifies(...)]` clauses
when using function contracts.
* We introduce an `#[safety_constraint(...)]` attribute helper for the
`Arbitrary` and `Invariant` macros.
* We enabled support for concrete playback for harness that contains
stubs or function contracts.
* We added support for log2*, log10*, powif*, fma*, and sqrt*
intrisincs.

### Breaking Changes
* The `-Z ptr-to-ref-cast-checks` option has been removed, and pointer
validity checks when casting raw pointers to references are now run by
default.

## What's Changed
* Make Kani reject mutable pointer casts if padding is incompatible and
memory initialization is checked by @artemagvanian in
#3332
* Fix visibility of some Kani intrinsics by @artemagvanian in
#3323
* Function Contracts: Modify Slices by @pi314mm in
#3295
* Support for disabling automatically generated pointer checks to avoid
reinstrumentation by @artemagvanian in
#3344
* Add support for global transformations by @artemagvanian in
#3348
* Enable an `#[safety_constraint(...)]` attribute helper for the
`Arbitrary` and `Invariant` macros by @adpaco-aws in
#3283
* Fix contract handling of promoted constants and constant static by
@celinval in #3305
* Bump CBMC Viewer to 3.9 by @tautschnig in
#3373
* Update to CBMC version 6.1.1 by @tautschnig in
#2995
* Define a struct-level `#[safety_constraint(...)]` attribute by
@adpaco-aws in #3270
* Enable concrete playback for contract and stubs by @celinval in
#3389
* Add code scanner tool by @celinval in
#3120
* Enable contracts in associated functions by @celinval in
#3363
* Enable log2*, log10* intrinsics by @tautschnig in
#3001
* Enable powif* intrinsics by @tautschnig in
#2999
* Enable fma* intrinsics by @tautschnig in
#3002
* Enable sqrt* intrinsics by @tautschnig in
#3000
* Remove assigns clause for ZST pointers by @carolynzech in
#3417
* Instrumentation for delayed UB stemming from uninitialized memory by
@artemagvanian in #3374
* Unify kani library and kani core logic by @jaisnan in
#3333
* Stabilize pointer-to-reference cast validity checks by @artemagvanian
in #3426
* Rust toolchain upgraded to `nightly-2024-08-07` by @jaisnan @qinheping
@tautschnig @feliperodri

## New Contributors
* @carolynzech made their first contribution in
#3387

**Full Changelog**:
kani-0.53.0...kani-0.54.0

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

Signed-off-by: Felipe R. Monteiro <felisous@amazon.com>
github-merge-queue bot pushed a commit that referenced this pull request Sep 18, 2024
Kani v0.55 no longer has the overly broad span issue reported in #3009.
I suspect that our shift (#3363) from functions to closures for
contracts allows rustc to produce better error messages. Add tests to
prevent regression in the future.

Resolves #3009 

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.
github-merge-queue bot pushed a commit that referenced this pull request Oct 24, 2024
I'm adding a few fix-me tests that I bumped into while working on #3363.
Most of them will be fixed by #3363, except the one related to #3370.

The original PR is already quite large, so I decided to just push all of
these as fixme tests for now. This is now ready for review!

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: Felipe R. Monteiro <felisous@amazon.com>
Co-authored-by: Zyad Hassan <88045115+zhassan-aws@users.noreply.github.com>
Co-authored-by: Carolyn Zech <cmzech@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-BenchCI Tag a PR to run benchmark CI Z-Contracts Issue related to code contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when annotating an associated function with a contract
4 participants