-
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
Eliminate a source of code bloat in the kani library #2033
Conversation
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.
I'm slightly concerned about playing whack-a-mole with this.
With assess, I think we also see a lot of code bloat and caller_location. I haven't looked into it yet, but it'd be nice if we could integrate a hook or something in Kani that fixed all this everywhere, and then we could revert these back to being basic unreachable!()
or whatever.
/// produce unnecessarily large MIR. This function is handled via a hook | ||
/// anyway, so we just need to put a body that rustc does not complain about. An | ||
/// infinite loop works out nicely. | ||
const fn __kani_dummy_never() -> ! { |
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.
#[doc(hidden)]
for things we don't want customer-facing.
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.
Is this needed for a private function?
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.
Oh, good point. Nevermind then :)
I don't think hooks will help here, since it would run after the reachability analysis. We could potentially plug hooks into the reachability analysis or use stubs here. For the caller location warning, we should probably just fix it. |
Also, I mentioned this to @zhassan-aws. Fixing our library is not really going to fix the problem in real user code, since any call to the std library that may eventually panic will bring in all that code. E.g.: |
True. One interesting thing though is that
I'm not sure what else These numbers are using c135fa9. |
I evaluated the impact of this change on a few customer tests and found the reduction in the MIR size to be minimal. Abandoning this PR. |
Description of changes:
Same trick applied in #2032, but this time to the
kani::panic
function. In addition to eliminating thecaller_location
warning on a small program like this one:it also results in a speedup and a significant reduction in the MIR size.
Before:
$ /usr/bin/time -p kani simp_panic.rs ... real 3.92 user 3.54 sys 0.37 $ RUSTFLAGS="--emit mir" kani simp_panic.rs $ wc simp_panic.kani.mir 33248 237545 4173632 simp_panic.kani.mir
After:
$ /usr/bin/time -p kani simp_panic.rs ... real 0.15 user 0.11 sys 0.04 $ RUSTFLAGS="--emit mir" kani simp_panic.rs $ wc simp_panic.kani.mir 86 530 5399 simp_panic.kani.mir
Resolved issues:
Resolves #2010
Related RFC:
Optional #ISSUE-NUMBER.
Call-outs:
Testing:
How is this change tested? Couldn't find a way to test this.
Is this a refactor change? No
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.