-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Port all viable contracts from verify-rust-std #147148
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
base: main
Are you sure you want to change the base?
Port all viable contracts from verify-rust-std #147148
Conversation
This comment has been minimized.
This comment has been minimized.
| #[core::contracts::requires(!self.overflowing_mul(rhs).1)] | ||
| pub const unsafe fn unchecked_mul(self, rhs: Self) -> Self { | ||
| assert_unsafe_precondition!( | ||
| check_language_ub, |
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.
Many contracts are duplicating existing assert_unsafe_precondition!(). How should we handle this duplication?
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.
My personal opinion is that all uses of assert_unsafe_precondition should be replaced by contracts::requires clauses as contracts provide a more general framework.
| + _7 = &_4; | ||
| + _6 = {closure@$SRC_DIR/core/src/num/uint_macros.rs:LL:COL} { 0: copy _7 }; | ||
| + StorageDead(_7); | ||
| + _5 = contract_check_requires::<{closure@core::num::<impl u16>::unchecked_shl::{closure#0}}>(move _6) -> [return: bb1, unwind continue]; |
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.
All these extra checks would slow down debug builds a lot in terms of runtime performance and likely a bit in build times, right? Can we check by how much exactly?
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.
With #144438 merged there should not be any runtime impact (not even in debug builds), unless contract checks are enabled. Build times, however, may indeed be affects. Would you have any suggestion on what experiments we should perform to obtain numbers?
|
☔ The latest upstream changes (presumably #142771) made this pull request unmergeable. Please resolve the merge conflicts. |
fc56734 to
f47406e
Compare
This comment has been minimized.
This comment has been minimized.
fa3bb5c to
b38b44c
Compare
This comment has been minimized.
This comment has been minimized.
Ports over all contracts (other than those for `Alignment`, see the separate PR) that can be expressed using the current, experimental contracts syntax. (Notably, this excludes all contracts that refer to pointer validity.)
Updated via `./x.py test mir-opt --bless --stage 1`.
These are now expressible thanks to aeae085.
2daa184 to
357743f
Compare
|
cc @rust-lang/wg-const-eval The Miri subtree was changed cc @rust-lang/miri Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
|
@tautschnig: 🔑 Insufficient privileges: not in try users |
|
@bors try @rust-timer queue You can always ask for a perf run here: https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/perf.20run/with/543397764 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…r=<try> Port all viable contracts from verify-rust-std
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (2c09cd9): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.6%, secondary 2.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 4.5%, secondary 4.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.3%, secondary 0.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 475.886s -> 478.145s (0.47%) |
|
☔ The latest upstream changes (presumably #148412) made this pull request unmergeable. Please resolve the merge conflicts. |
| // #[core::contracts::ensures( | ||
| // move |result: &Layout| | ||
| // result.size() >= self.size() && | ||
| // result.align() == self.align() && | ||
| // result.size() % result.align() == 0 && | ||
| // self.size() + self.padding_needed_for(self.align()) == result.size())] |
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.
Would something like this work?
#[core::contracts::ensures({
let old_self = *self; // Copy self value.
move |result: &Layout|
result.size() >= old_self.size() &&
result.align() == old_self.align() &&
result.size() % result.align() == 0 &&
old_self.size() + old_self.padding_needed_for(old_self.align()) == result.size())}]|
So I like contracts, but it seems like we shouldn't be getting rustc-perf impacts this high to add them when they're not even being used to do anything? Is there anything feasible to do about that? @rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
I don't have the expertise to interpret the runs of #145229 was proposed to completely disable macro expansion of contracts when those are disabled, but was rejected as having contracts not be type checked was considered undesirable. Maybe we can revisit this and implement some sort of hybrid solution, giving the option to opt-in or opt-out of contract macro expansion as a new compiler flag. Then e.g. the flag could be enabled in CI, checking the well-typedness of contracts, whereas for fast iterations to build Any thoughts on this approach? Should contract expansion be opt-in or opt-out? I would image opt-out is a better fit, as by default having type-checking on contracts is nice for the general user, and we can set the opt-out flag in the |
|
The issue here is not the performance regression of compiling libstd, but that other crates have regressions when calling those functions |
Wouldn't the flag still do the job? We can expose the stdlib for consumption by crates with the contracts macro expansion disabled, and then users can override that with |
Ports over all contracts (other than those for
Alignment, see the separate PR) that can be expressed using the current, experimental contracts syntax. (Notably, this excludes all contracts that refer to pointer validity.)