Add overflow_checks intrinsic
#128666
Open
Add overflow_checks intrinsic
#128666
+317
−102
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt This PR changes Stable MIR cc @oli-obk, @celinval, @ouz-a Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
rustbot
added
the
T-libs
|
Given how invasive and boilerplatey the implementation of the intrinsic is, I strongly prefer we not merge this unless it comes with well-motivated use in the standard library implementation. |
|
Just to be clear, do you mean the existing motivation (better debug behavior for |
|
Oh wow that wording I left was very unclear. I'll try to explain myself better this time. The implementation strategy that is currently used for the runtime UB checks (intrinsic which lowers to a new NullOp that's branched on) is concerningly heavy on both compiler implementation complexity and compile-time overhead. There are 4 places where the runtime UB checks are cfg'd out or not merged yet because their compile-time overhead isn't justifiable ( Before we merge the compiler complexity in this PR, I want evidence that actually using the new intrinsic for its intended purpose has tolerable compile-time overhead. If the compile-time overhead of the approach in this PR is so high that its use must be avoided in often-instantiated code paths, we should find another strategy. So what would make me happy is a draft PR based on this one (or just modifying this PR) that swaps in the new range types and uses this new I've mostly put up with the UB checks implementation because I have finite time to investigate things, and it has so far been a mostly-successful way to deliver a feature we've been missing for years. My leading theory on a better way to do this is to have a magic |
|
☔ The latest upstream changes (presumably #128707) made this pull request unmergeable. Please resolve the merge conflicts. |
BoxyUwU
added
S-blocked
|
It's worth noting that like the UB checks flag, if this gets added we need to ensure that ctfe always runs with either the flag enabled or disabled and is not affected by any kind of rustc flag as otherwise it would be unsound for const generics (#129552). I think what @saethlin said about making sure this is actually usable in practice is a good point so marked this as S-blocked until that is resolved. |
|
I'm wondering if these cases where we want to enable extra checks in the standard library should be fixed by improving and stabilizing the cargo option to build the standard library together with the target crate. |
|
I too would love to see all of the issues addressed: https://github.com/rust-lang/wg-cargo-std-aware/issues?q=is%3Aissue+label%3A%22stabilization+blocker%22+is%3Aopen but it's a huge engineering and political effort. If you'd like to push forward such work, a PR is not the right place to advocate for it. |
I'm just trying to understand the motivation behind the chosen approach, which I think is a fair point to ask. What are the trade-offs and why this has to be solved by adding more logic to the compiler backend so the std library behaves like any other cargo dependency. From what I understand, building the standard library in cargo is out of the table given its current state. So thank you for clarifying. |
|
I'm gonna close this since it's not had any activity for ~2 months and making progress seems somewhat blocked. Feel free to re-open this at some future point. |
tests will fail, but this is meant for checking how it affects compile times, which shouldn't be impacted by the broken logic
pitaj
force-pushed
the
intrinsic-overflow_checks
branch
from
803f841 to
a7c9eb6
Compare
|
Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred to the CTFE machinery The Miri subtree was changed cc @rust-lang/miri Some changes occurred to constck cc @fee1-dead |
|
Reopening this because I noticed the recent addition of the Running perf on this PR should give us some idea of how much impact this kind of check would have on compile times if the new |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds an intrinsic which allows code in a pre-built library to inherit the overflow checks option from a crate depending on it. This enables code in the standard library to explicitly change behavior based on whether
overflow_checksare enabled, regardless of the setting used when standard library was compiled.This is very similar to the
ub_checksintrinsic, and refactors the two to use a common mechanism.The primary use case for this is to allow the new
RangeFromiterator to yield the maximum element before overflowing, as requested here. I already have a workingIterRangeFromimplementation based on this new intrinsic that exhibits the desired behavior.Prior discussion on Zulip