Add assert_unsafe_precondition to unchecked_{add,sub,neg,mul,shl,shr} methods #121571

clarfonthey · 2024-02-24T22:20:40Z

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

rustbot · 2024-02-24T22:20:48Z

r? @cuviper

rustbot has assigned @cuviper.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-02-24T22:20:50Z

The Miri subtree was changed

cc @rust-lang/miri

clarfonthey · 2024-02-24T22:27:04Z

(Hastily added the commit I held off on and realised that tests are failing, so, I'll fix those soon and rebase as well.)

saethlin · 2024-02-25T00:49:34Z

Let's see if the ghosts followed us
@bors try @rust-timer queue

bors · 2024-02-25T00:50:43Z

⌛ Trying commit 949d1f9 with merge 4028cb5...

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

bors · 2024-02-25T02:21:40Z

☀️ Try build successful - checks-actions
Build commit: 4028cb5 (4028cb5d31411da55b86940756d037628ce2531b)

rust-timer · 2024-02-25T03:35:00Z

Finished benchmarking commit (4028cb5): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking 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 @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.2%	[0.2%, 3.0%]	39
Regressions ❌ (secondary)	1.8%	[0.5%, 3.8%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	1
All ❌✅ (primary)	1.2%	[0.2%, 3.0%]	39

Max RSS (memory usage)

Results

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.

	mean	range	count
Regressions ❌ (primary)	3.4%	[0.5%, 5.2%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.5%	[-7.7%, -0.9%]	8
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.8%	[-7.7%, 5.2%]	13

Cycles

Results

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.

	mean	range	count
Regressions ❌ (primary)	2.3%	[0.8%, 4.0%]	21
Regressions ❌ (secondary)	3.1%	[1.8%, 4.5%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.3%	[0.8%, 4.0%]	21

Binary size

Results

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.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.0%, 1.5%]	68
Regressions ❌ (secondary)	0.8%	[0.2%, 1.8%]	9
Improvements ✅ (primary)	-0.2%	[-0.7%, -0.0%]	30
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	6
All ❌✅ (primary)	0.3%	[-0.7%, 1.5%]	98

Bootstrap: 649.328s -> 651.207s (0.29%)
Artifact size: 311.03 MiB -> 311.02 MiB (-0.00%)

clarfonthey · 2024-02-25T03:38:44Z

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.

saethlin · 2024-02-25T04:15:01Z

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 assert_unsafe_precondition internally stamps out a new function so that it can outline the check logic. It might be useful to investigate which check(s) are hottest using the technique I posted in the last PR. Certainly it will be educational to poke through IR dumps, if you're into this sort of thing.

clarfonthey · 2024-02-25T05:36:40Z

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.

RalfJung · 2024-02-25T07:25:53Z

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.

saethlin · 2024-02-25T17:07:46Z

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.

clarfonthey · 2024-03-21T02:33:00Z

Going to mark this as blocked by #122520 since it seems clear that's going to be merged soon, and I'll have to fix the merge conflicts.

@rustbot blocked

rust-log-analyzer · 2024-05-25T04:59:50Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

 mingw-w64-x86_64-gcc-ada-13.2.0-3-any downloading...
 mingw-w64-x86_64-gcc-objc-13.2.0-3-any downloading...
error: failed retrieving file 'mingw-w64-x86_64-headers-git-11.0.0.r547.g4c8123efb-1-any.pkg.tar.zst' from mirror.msys2.org : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
warning: too many errors from mirror.msys2.org, skipping for the remainder of this transaction
error: failed retrieving file 'mingw-w64-x86_64-gcc-ada-13.2.0-3-any.pkg.tar.zst.sig' from mirror.umd.edu : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
error: failed to commit transaction (unexpected error)
 mingw-w64-x86_64-gcc-fortran-13.2.0-3-any downloading...
 mingw-w64-x86_64-libgccjit-13.2.0-3-any downloading...
 mingw-w64-x86_64-openssl-3.2.0-1-any downloading...

bors · 2024-05-25T05:00:26Z

💔 Test failed - checks-actions

clarfonthey · 2024-05-25T05:02:20Z

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.

saethlin · 2024-05-25T13:27:33Z

It looks like no PRs were merged for about 12 hours but one just got through.

@bors retry

…ions, r=saethlin Add assert_unsafe_precondition 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.

bors · 2024-05-25T15:04:40Z

⌛ Testing commit 18cb2fa with merge dce1275...

rust-log-analyzer · 2024-05-25T15:18:54Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

error: failed retrieving file 'mingw-w64-x86_64-headers-git-11.0.0.r547.g4c8123efb-1-any.pkg.tar.zst' from mirror.msys2.org : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
 mingw-w64-x86_64-gcc-13.2.0-3-any downloading...
error: failed retrieving file 'mingw-w64-x86_64-python-3.11.7-1-any.pkg.tar.zst' from mirror.msys2.org : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
warning: too many errors from mirror.msys2.org, skipping for the remainder of this transaction
error: failed retrieving file 'mingw-w64-x86_64-gdb-14.1-1-any.pkg.tar.zst.sig' from mirror.umd.edu : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
error: failed to commit transaction (unexpected error)
 mingw-w64-x86_64-python-3.11.7-1-any downloading...
 mingw-w64-x86_64-gcc-ada-13.2.0-3-any downloading...
 mingw-w64-x86_64-gcc-objc-13.2.0-3-any downloading...

bors · 2024-05-25T15:19:34Z

💔 Test failed - checks-actions

saethlin · 2024-05-25T15:20:24Z

That is bizarre...

saethlin · 2024-05-25T15:25:29Z

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

saethlin · 2024-05-25T18:03:34Z

The last 2 PRs have landed without error 🤞
@bors r+

bors · 2024-05-25T18:03:36Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
There's another pull request that is currently being tested, blocking this pull request: bootstrap: support target specific config overrides #125515

bors · 2024-05-25T18:03:37Z

📌 Commit 18cb2fa has been approved by saethlin

It is now in the queue for this repository.

bors · 2024-05-25T18:07:36Z

⌛ Testing commit 18cb2fa with merge 48f0011...

bors · 2024-05-25T20:14:05Z

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing 48f0011 to master...

clarfonthey · 2024-05-25T20:30:50Z

Thank you to everyone for helping get this merged!

rust-timer · 2024-05-25T21:28:33Z

Finished benchmarking commit (48f0011): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.5%	[0.2%, 3.4%]	27
Regressions ❌ (secondary)	1.8%	[0.4%, 3.8%]	5
Improvements ✅ (primary)	-0.9%	[-2.5%, -0.3%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[-2.5%, 3.4%]	32

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.

	mean	range	count
Regressions ❌ (primary)	8.8%	[4.7%, 14.6%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.2%	[-3.8%, -2.5%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.8%	[-3.8%, 14.6%]	6

Cycles

Results (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.

	mean	range	count
Regressions ❌ (primary)	2.1%	[0.7%, 4.2%]	20
Regressions ❌ (secondary)	3.3%	[2.8%, 4.1%]	4
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.9%	[-2.1%, 4.2%]	21

Binary size

Results (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.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.0%, 1.6%]	65
Regressions ❌ (secondary)	0.7%	[0.1%, 1.8%]	14
Improvements ✅ (primary)	-0.2%	[-0.8%, -0.0%]	29
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	3
All ❌✅ (primary)	0.3%	[-0.8%, 1.6%]	94

Bootstrap: 671.46s -> 671.029s (-0.06%)
Artifact size: 315.58 MiB -> 315.62 MiB (0.01%)

rustbot assigned cuviper Feb 24, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 24, 2024

clarfonthey mentioned this pull request Feb 24, 2024

Add debug_assert_nounwind to unchecked_{add,sub,neg,mul,shl,shr} methods (haunted PR) #117494

Closed

This comment has been minimized.

Sign in to view

clarfonthey force-pushed the unchecked-math-preconditions branch from 3a8f4fa to 0673ba1 Compare February 24, 2024 22:47

This comment has been minimized.

Sign in to view

clarfonthey force-pushed the unchecked-math-preconditions branch from 0673ba1 to 949d1f9 Compare February 24, 2024 23:35

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 25, 2024

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 25, 2024

clarfonthey changed the title ~~Add debug_assert_nounwind to unchecked_{add,sub,neg,mul,shl,shr} methods~~ [don't merge] Add debug_assert_nounwind to unchecked_{add,sub,neg,mul,shl,shr} methods Feb 25, 2024

rustbot assigned saethlin and unassigned cuviper Feb 25, 2024

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 25, 2024

jyn514 mentioned this pull request May 25, 2024

compiletest should note when test output is normalized #125537

Open

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 25, 2024

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 25, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 25, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label May 25, 2024

bors merged commit 48f0011 into rust-lang:master May 25, 2024
7 checks passed

rustbot added this to the 1.80.0 milestone May 25, 2024

clarfonthey deleted the unchecked-math-preconditions branch May 25, 2024 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add assert_unsafe_precondition to unchecked_{add,sub,neg,mul,shl,shr} methods #121571

Add assert_unsafe_precondition to unchecked_{add,sub,neg,mul,shl,shr} methods #121571

clarfonthey commented Feb 24, 2024

rustbot commented Feb 24, 2024

rustbot commented Feb 24, 2024

clarfonthey commented Feb 24, 2024

This comment has been minimized.

This comment has been minimized.

saethlin commented Feb 25, 2024

This comment has been minimized.

bors commented Feb 25, 2024

bors commented Feb 25, 2024

This comment has been minimized.

rust-timer commented Feb 25, 2024

clarfonthey commented Feb 25, 2024

saethlin commented Feb 25, 2024 •

edited

clarfonthey commented Feb 25, 2024

RalfJung commented Feb 25, 2024

saethlin commented Feb 25, 2024

clarfonthey commented Mar 21, 2024

rust-log-analyzer commented May 25, 2024

bors commented May 25, 2024

clarfonthey commented May 25, 2024

saethlin commented May 25, 2024

bors commented May 25, 2024

rust-log-analyzer commented May 25, 2024

bors commented May 25, 2024

saethlin commented May 25, 2024

saethlin commented May 25, 2024

saethlin commented May 25, 2024

bors commented May 25, 2024

bors commented May 25, 2024

bors commented May 25, 2024

bors commented May 25, 2024

clarfonthey commented May 25, 2024

rust-timer commented May 25, 2024

Add assert_unsafe_precondition to unchecked_{add,sub,neg,mul,shl,shr} methods #121571

Add assert_unsafe_precondition to unchecked_{add,sub,neg,mul,shl,shr} methods #121571

Conversation

clarfonthey commented Feb 24, 2024

rustbot commented Feb 24, 2024

rustbot commented Feb 24, 2024

clarfonthey commented Feb 24, 2024

This comment has been minimized.

This comment has been minimized.

saethlin commented Feb 25, 2024

This comment has been minimized.

bors commented Feb 25, 2024

bors commented Feb 25, 2024

This comment has been minimized.

rust-timer commented Feb 25, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

clarfonthey commented Feb 25, 2024

saethlin commented Feb 25, 2024 • edited

clarfonthey commented Feb 25, 2024

RalfJung commented Feb 25, 2024

saethlin commented Feb 25, 2024

clarfonthey commented Mar 21, 2024

rust-log-analyzer commented May 25, 2024

bors commented May 25, 2024

clarfonthey commented May 25, 2024

saethlin commented May 25, 2024

bors commented May 25, 2024

rust-log-analyzer commented May 25, 2024

bors commented May 25, 2024

saethlin commented May 25, 2024

saethlin commented May 25, 2024

saethlin commented May 25, 2024

bors commented May 25, 2024

bors commented May 25, 2024

bors commented May 25, 2024

bors commented May 25, 2024

clarfonthey commented May 25, 2024

rust-timer commented May 25, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

saethlin commented Feb 25, 2024 •

edited