Add assert_unsafe_precondition to unchecked_{add,sub,neg,mul,shl,shr} methods#121571
Conversation
|
The Miri subtree was changed cc @rust-lang/miri |
|
(Hastily added the commit I held off on and realised that tests are failing, so, I'll fix those soon and rebase as well.) |
This comment has been minimized.
This comment has been minimized.
3a8f4fa to
0673ba1
Compare
This comment has been minimized.
This comment has been minimized.
0673ba1 to
949d1f9
Compare
|
Let's see if the ghosts followed us |
This comment has been minimized.
This comment has been minimized.
…ions, r=<try>
Add debug_assert_nounwind to unchecked_{add,sub,neg,mul,shl,shr} methods
(Old PR is haunted, opening a new one. See rust-lang#117494 for previous discussion.)
This ensures that these preconditions are actually checked in debug mode, and hopefully should let people know if they messed up. I've also replaced the calls (I could find) in the code that use these intrinsics directly with those that use these methods, so that the asserts actually apply.
More discussions on people misusing these methods in the tracking issue: rust-lang#85122.
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (4028cb5): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 649.328s -> 651.207s (0.29%) |
|
So, I'm guessing that the answer is "about what we expected," although I'm not sure what percentage of those benchmarks are running under debug versus release mode and would have to do some more digging. |
|
Click on "comparison URL", that will take you to the page with all the useful data on it. Click around, explore. I think the opt regressions might be addressed by #121421, for sure that PR is supposed to eliminate the extra IR that would be generated. The debug build regressions, well, keeping those down are why |
|
I'm really inexperienced when it comes to debugging the compiler, so, I definitely do think that poking through the IR dumps would be cool, but I'm not sure how effective I'll be at it. I think that this is blocked anyway on a couple optimisations anyway, so, at least I'll have time. I do still think that it'd be good to have a review that okays the code as-is regardless, even though we won't merge it until we get the perf situation sorted out, since that way we can be ready with the PR when that happens. I'll add a "don't merge" to the PR title in an effort to dissuade people from actually approving it before we sort out the blockers, but I don't want to actually add an S-blocked so folks still get around to the reviews. |
|
Also carrying over this potential blocker from the other thread: I think adding more of these checks needs to be blocked on having a way to turn them off separately from debug assertions, to avoid having more issues like this. (This is about runtime overhead, not compiletime overhead.) I love having these checks but I worry about the unintended side-effects of making even optimized debug-assertion builds a lot slower. |
|
r? saethlin I'm basically reviewing/mentoring this PR already, and there would be a lot of history for a randomly-selected libs reviewer to go through. |
|
I applied this diff and I get exactly the same run-make libtest-padding test failure: -ENV RUST_CONFIGURE_ARGS --build=i686-unknown-linux-gnu
+ENV RUSTFLAGS_NOT_BOOTSTRAP="-Zmir-opt-level=0"
+ENV RUST_CONFIGURE_ARGS \
+ --build=i686-unknown-linux-gnu \
+ --set rust.optimize=0
# Skip some tests that are unlikely to be platform specific, to speed up
# this slow job.
-ENV SCRIPT python3 ../x.py --stage 2 test \
- --skip src/bootstrap \
- --skip tests/rustdoc-js \
- --skip src/tools/error_index_generator \
- --skip src/tools/linkchecker
+ENV SCRIPT python3 ../x.py --stage 2 test run-makeSo to me that indicates that this is not a buggy optimization, because I think I've turned them all off. |
|
I have more things to try, don't worry :) |
The body of these benchmarks is close to empty but not literally empty. This was making the runtime of the benchmarks (which are compiled without optimizations!) flicker between 9 ns and 10 ns runtime, which changes the padding and breaks the test. Recent changes to the standard library have pushed the runtime closer to 10 ns when unoptimized, which is why we haven't seen such failures before in CI. Contributors can also induce such failures before this PR by running the run-make tests while the system is under heavy load.
|
Some changes occurred in run-make tests. cc @jieyouxu |
|
A job failed! Check out the build log: (web) (plain) Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
|
@bors retry |
|
A job failed! Check out the build log: (web) (plain) Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
|
This PR just really doesn't want to be merged, I guess. Might have to wait until tomorrow if it's just some of the dependencies just not downloading. |
|
It looks like no PRs were merged for about 12 hours but one just got through. @bors retry |
|
A job failed! Check out the build log: (web) (plain) Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
|
That is bizarre... |
|
Ah I see a number of recent PRs are bouncing off this. https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/Numerous.20errors.20downloading.20mingw/near/440643904 |
|
The last 2 PRs have landed without error 🤞 |
|
💡 This pull request was already approved, no need to approve it again.
|
|
☀️ Test successful - checks-actions |
|
Thank you to everyone for helping get this merged! |
|
Finished benchmarking commit (48f0011): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 2.8%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 1.9%, secondary 3.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary 0.3%, secondary 0.6%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 671.46s -> 671.029s (-0.06%) |
(Old PR is haunted, opening a new one. See #117494 for previous discussion.)
This ensures that these preconditions are actually checked in debug mode, and hopefully should let people know if they messed up. I've also replaced the calls (I could find) in the code that use these intrinsics directly with those that use these methods, so that the asserts actually apply.
More discussions on people misusing these methods in the tracking issue: #85122.