const-eval: detect more pointers as definitely not-null #133700
Conversation
|
r? @davidtwco rustbot has assigned @davidtwco. Use |
rustbot
added
S-waiting-on-review
|
Some changes occurred to the CTFE machinery cc @rust-lang/wg-const-eval Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri |
RalfJung
added
T-lang
|
r? @lcnr |
|
r=me after lang approval (idk if it needs a full FCP, it is observable by users after all) |
|
@RalfJung @JakobDegen Could you elaborate on the motivation? I agree it would be nice if #133523 compiled but I find myself asking "how clever is clever enough" for these checks. Do you feel comfortable with writing this behavior into the language spec? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
rfcbot
added
proposed-final-comment-period
|
Oh dear. @rfcbot cancel @labels -T-compiler |
|
what am I doing :) |
rustbot
removed
the
T-compiler
|
@nikomatsakis proposal cancelled. |
rfcbot
removed
proposed-final-comment-period
|
@rfcbot fcp merge Based on discussion in the lang-team meeting we felt this needed an FCP. We discussed a few points we'd like to see clarified
but it is still complicating the spec, and it's not obvious when this function (or any other) will be "smart enough", so @tmandry was looking for better motivation than an issue (does this represent a real-world pattern?). The other question came from @pnkfelix who was wondering if the logic could be invalidated by people casting unaligned pointers or doing other things that don't respect alignment. |
|
Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
rfcbot
added
proposed-final-comment-period
|
@rustbot labels +T-compiler |
|
@joshtriplett @scottmcm @tmandry have the answers above resolved your concerns, or is the plan that this will be discussed at some future lang team meeting before we proceed with FCP? |
|
Thanks for the discussion. Reading #133523 (comment) (which I missed before) was helpful for understanding a real-world use case. Making const eval less willing to give up in a case where we know a pointer can't be null makes sense to me. @rfcbot reviewed Do you think the behavior should be documented in the reference somewhere? I guess it stays out of detailed CTFE mechanics like this? |
rfcbot
added
the
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rfcbot
removed
the
proposed-final-comment-period
|
AFAIK we don't say anything about |
|
Looking at how it works, I feel good about doing this. Adding the modulo check is straight-forward to explain, and fits into the existing data well. If it took a whole bunch of new complicated tracking I'm be more skeptical, but this check seems fine. |
traviscross
removed
the
I-lang-nominated
rfcbot
added
finished-final-comment-period
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
RalfJung
force-pushed
the
const-non-null
branch
from
b438e46 to
e1dda10
Compare
|
🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened. |
bors
added
S-waiting-on-bors
|
@bors r=lcnr |
|
🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened. |
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#133700 (const-eval: detect more pointers as definitely not-null) - rust-lang#135290 (Encode constraints that hold at all points as logical edges in location-sensitive polonius) - rust-lang#135478 (Run clippy for rustc_codegen_gcc on CI) - rust-lang#135583 (Move `std::pipe::*` into `std::io`) - rust-lang#135612 (Include x scripts in tarballs) - rust-lang#135624 (ci: mirror buildkit image to ghcr) - rust-lang#135661 (Stabilize `float_next_up_down`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#133700 - RalfJung:const-non-null, r=lcnr const-eval: detect more pointers as definitely not-null This fixes rust-lang#133523 by making the `scalar_may_be_null` check smarter: for instance, an odd offset in any 2-aligned allocation can never be null, even if it is out-of-bounds. More generally, if an allocation with unknown base address B is aligned to alignment N, and a pointer is at offset X inside that allocation, then we know that `(B + X) mod N = B mod N + X mod N = X mod N`. Since `0 mod N` is definitely 0, if we learn that `X mod N` is *not* 0 we can deduce that `B + X` is not 0. This is immediately visible on stable, via `ptr.is_null()` (and, more subtly, by not raising a UB error when such a pointer is used somewhere that a non-null pointer is required). Therefore nominating for `@rust-lang/lang.`
This fixes #133523 by making the
scalar_may_be_nullcheck smarter: for instance, an odd offset in any 2-aligned allocation can never be null, even if it is out-of-bounds.More generally, if an allocation with unknown base address B is aligned to alignment N, and a pointer is at offset X inside that allocation, then we know that
(B + X) mod N = B mod N + X mod N = X mod N. Since0 mod Nis definitely 0, if we learn thatX mod Nis not 0 we can deduce thatB + Xis not 0.This is immediately visible on stable, via
ptr.is_null()(and, more subtly, by not raising a UB error when such a pointer is used somewhere that a non-null pointer is required). Therefore nominating for @rust-lang/lang.