-
Notifications
You must be signed in to change notification settings - Fork 97
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
Adopt Rust's source-based code coverage instrumentation #3119
Adopt Rust's source-based code coverage instrumentation #3119
Conversation
952c406
to
0ae802b
Compare
981b46b
to
187f5ea
Compare
3cfef90
to
e010665
Compare
Ready for review again - I've taken the chance to reorganize the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I haven't looked at the CBMC result parsing part yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits. Thanks @adpaco-aws!
Thank you, @zhassan-aws ! I will hold the merge of this PR until #3143 is merged. Also, the other we talked about this feature breaking the coverage feature in the VS Code extension. I recall agreeing to keeping the line-based coverage option but, on a second thought, I think it'd just be easier to remove it here and integrate the VS Code extension with the source-based feature. I'm meeting @jaisnan soon to make sure that's the case and coordinate on the integration. |
These are the auto-generated release notes: ## What's Changed * Update CBMC build instructions for Amazon Linux 2 by @tautschnig in #3431 * Handle intrinsics systematically by @artemagvanian in #3422 * Bump tests/perf/s2n-quic from `445f73b` to `ab9723a` by @dependabot in #3434 * Automatic cargo update to 2024-08-12 by @github-actions in #3433 * Actually apply CBMC patch by @tautschnig in #3436 * Update features/verify-rust-std branch by @feliperodri in #3435 * Add test related to issue 3432 by @celinval in #3439 * Implement memory initialization state copy functionality by @artemagvanian in #3350 * Bump tests/perf/s2n-quic from `ab9723a` to `80b93a7` by @dependabot in #3453 * Make points-to analysis handle all intrinsics explicitly by @artemagvanian in #3452 * Automatic cargo update to 2024-08-19 by @github-actions in #3450 * Add loop scanner to tool-scanner by @qinheping in #3443 * Avoid corner-cases by grouping instrumentation into basic blocks and using backward iteration by @artemagvanian in #3438 * Re-enabled hierarchical logs in the compiler by @celinval in #3449 * Fix ICE due to mishandling of Aggregate rvalue for raw pointers to `str` by @celinval in #3448 * Automatic cargo update to 2024-08-26 by @github-actions in #3459 * Bump tests/perf/s2n-quic from `80b93a7` to `8f7c04b` by @dependabot in #3460 * Update deny action by @zhassan-aws in #3461 * Basic support for memory initialization checks for unions by @artemagvanian in #3444 * Adjust test patterns so as not to check for trivial properties by @tautschnig in #3464 * Clarify comment in RFC Template by @carolynzech in #3462 * RFC: Source-based code coverage by @adpaco-aws in #3143 * Adopt Rust's source-based code coverage instrumentation by @adpaco-aws in #3119 * Upgrade toolchain to 08/28 by @jaisnan in #3454 * Extra tests and bug fixes to the delayed UB instrumentation by @artemagvanian in #3419 * Upgrade Toolchain to 8/29 by @carolynzech in #3468 * Automatic toolchain upgrade to nightly-2024-08-30 by @github-actions in #3469 * Extend name resolution to support qualified paths (Partial Fix) by @celinval in #3457 * Partially integrate uninit memory checks into `verify_std` by @artemagvanian in #3470 * Update Toolchain to 9/1 by @carolynzech in #3478 * Automatic cargo update to 2024-09-02 by @github-actions in #3480 * Bump tests/perf/s2n-quic from `8f7c04b` to `1ff3a9c` by @dependabot in #3481 * Automatic toolchain upgrade to nightly-2024-09-02 by @github-actions in #3479 * Automatic toolchain upgrade to nightly-2024-09-03 by @github-actions in #3482 * RFC for List Subcommand by @carolynzech in #3463 * Add tests for fixed issues. by @carolynzech in #3484 **Full Changelog**: kani-0.54.0...kani-0.55.0 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.
This PR replaces the line-based coverage instrumentation we introduced in #2609 with the standard source-based code coverage instrumentation performed by the Rust compiler.
As a result, we now insert code coverage checks in the
StatementKind::Coverage(..)
statements produced by the Rust compiler during compilation. These checks include coverage-relevant information1 such as the coverage counter/expression they represent 2. Both the coverage metadata (kanimap
) and coverage results (kaniraw
) are saved into files after the verification stage.Unfortunately, we currently have a chicken-egg problem with this PR and #3121, where we introduce a tool named
kani-cov
to postprocess coverage results. As explained in #3143,kani-cov
is expected to be an alias for thecov
subcommand and provide most of the postprocessing features for coverage-related purposes. But, the tool will likely be introduced after this change. Therefore, we propose to temporarily print a list of the regions in each function with their associated coverage status (i.e.,COVERED
orUNCOVERED
).Source-based code coverage: An example
The main advantage of source-based coverage results is their precision with respect to the source code. The Source-based Code Coverage documentation explains more details about the LLVM coverage workflow and its different options.
For example, let's take this Rust code:
Compiling and running the program with
rustc
and the-C instrument-coverage
flag, and using the LLVM tools can get us the following coverage result:In contrast, the
cargo kani --coverage -Zsource-coverage
command currently generates:which is a verification-based coverage result almost equivalent to the runtime coverage results.
Benchmarking
We have evaluated the performance impact of the instrumentation using the
kani-perf.sh
suite (14 benchmarks). For each test, we compare the average time to run standard verification against the average time to run verification with the source-based code coverage feature enabled3.The evaluation has been performed on an EC2
m5a.4xlarge
instance running Ubuntu 22.04. The experimental data has been obtained by running thekani-perf.sh
script 10 times for each version (only verification
andverification + coverage
), computing the average and standard deviation. We've split this data intosmall
(tests taking 60s or less) andlarge
(tests taking more than 60s) and drawn the two graphs below.Performance comparison -
small
benchmarksPerformance comparison -
large
benchmarksComments on performance
Looking at the small tests, the performance impact seems negligible in such cases. The difference is more noticeable in the large tests, where the time to run verification and coverage can take 2x or even more. It wouldn't be surprising that, as programs become larger, the complexity of the coverage checking grows exponentially as well. However, since most verification jobs don't take longer than 30min (1800s), it's OK to say that coverage checking represents a 100-200% slowdown in the worst case w.r.t. standard verification.
It's also worth noting a few other things:
Call-outs
tests/coverage/
to test this feature. Since this technique is simpler, we don't need that many test cases. However, it's possible I've left some test cases which don't contribute much. Please let me know if you want to add/remove a test case.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.
Footnotes
The coverage mappings can't be accessed through the StableMIR interface so we retrieve them through the internal API. ↩
The instrumentation replaces certain counters with expressions based on other counters when possible to avoid a part of the runtime overhead. More details can be found here. Unfortunately, we can't avoid instrumenting expressions at the moment. ↩
We have not compared performance against the line-based code coverage feature because it doesn't seem worth it. The line-based coverage feature is guaranteed to include more coverage checks than the source-based one for any function. In addition, source-based results are more precise than line-based ones. So this change represents both a quantitative and qualitative improvement. ↩