Ban ArrayToPointer and MutToConstPointer from runtime MIR by scottmcm · Pull Request #126308 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation23 Commits2 Checks6 Files changed
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
[ Show hidden characters]({{ revealButtonHref }})
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
(Banning them simplifies passes like GVN that no longer need to handle multiple cast possibilities.)
How that? Can't GVN use an "or" pattern to cover all these cases together?
To me, banning them seems more complicated than simply allowing them.
| @@ -506,6 +507,7 @@ fn run_analysis_cleanup_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { |
|---|
| /// Returns the sequence of passes that lowers analysis to runtime MIR. |
| fn run_runtime_lowering_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { |
| let passes: &[&dyn MirPass<'tcx>] = &[ |
| &coercions_to_casts::CoercionsToCasts, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not inside 'cleanup_post_borrowck' ?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about this more, and decided that it's easier to write the why comment when it's in cleanup_post_borrowck, so I moved it there, and adjusted them to be banned starting in Analysis(PostCleanup) rather than just in Runtime(..).
How that? Can't GVN use an "or" pattern to cover all these cases together?
I see it less as being about the code that's typed are more about "is it clear how you should think about things".
For example, today GVN groups these three here
| CastKind::PointerCoercion( |
|---|
| ty::adjustment::PointerCoercion::MutToConstPointer |
| | ty::adjustment::PointerCoercion::ArrayToPointer |
| | ty::adjustment::PointerCoercion::UnsafeFnPointer, |
| ) => { |
but then only has PtrToPtr and MutToConstPointer in
| if let PtrToPtr | PointerCoercion(MutToConstPointer) = kind |
|---|
| && let Value::Cast { kind: inner_kind, value: inner_value, from: inner_from, to: _ } = |
| *self.get(value) |
| && let PtrToPtr | PointerCoercion(MutToConstPointer) = inner_kind |
Is that right? Should the former have MutToConstPointer in the same arm as PtrToPtr? Should the latter have ArrayToPointer in it too? I don't know.
I think it's much simpler when the answer to those things is "as it says on CastKind and MirPhase::Runtime, you only need PtrToPtr here; ignore the rest".
I think it's much simpler when the answer to those things is "as it says on CastKind and MirPhase::Runtime, you only need PtrToPtr here; ignore the rest".
What about grouping all of these together in a single CastKind variant PtrToPtr(PtrCastKind), and declaring that operationally, the PtrCastKind makes no difference, it just exists for borrowck? Then it's easy to consistently match on it and ignore the irrelevant details in mir-opts.
This is not a strong opinion though, I just feel like this PR is using the MIR validation to work around CastKind just not being of the right shape for some of its consumers.
| @@ -143,6 +143,9 @@ pub enum RuntimePhase { |
|---|
| /// * [`TerminatorKind::CoroutineDrop`] |
| /// * [`Rvalue::Aggregate`] for any `AggregateKind` except `Array` |
| /// * [`PlaceElem::OpaqueCast`] |
| /// * [`CastKind::PointerCoercion`] with any of the following: |
| /// * [`PointerCoercion::ArrayToPointer`] |
| /// * [`PointerCoercion::MutToConstPointer`] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compiler/rustc_const_eval/src/interpret/cast.rs should then probably also explicitly panic on these casts.
Ah, we have have this inner enum, PointerCoercion... but that enum is a pretty poor fit for runtime MIR in general, it is designed for coercion generation way earlier in the compiler. I wonder if we'd be better off just not using it in MIR... but I have no idea how this is relevant for borrowck.
but I have no idea how this is relevant for borrowck.
I first tried just banning these from MIR altogether, but doing that meant there were some missing errors in one of the borrowck tests. I don't have the failure handy, but I think it was this one:
| fn mut_to_const<'a, 'b>(x: *mut &'a i32) -> *const &'b i32 { |
|---|
| x //~ ERROR |
| } |
So it's checks about lifetimes inside the pointee, I guess?
I would expect the lifetimes in that test to be a red herring -- mut_to_const is the key. Someone has to reject that as a coercion (but allow it as a cast).
Ah no never mind, I shouldn't write comments when I am too tired...
on a mut-to-const implicit coercion, the type needs to stay the same, and the lifetime equality part of "the same" is checked by borrowck.
but that enum is a pretty poor fit for runtime MIR in general, it is designed for coercion generation way earlier in the compiler. I wonder if we'd be better off just not using it in MIR...
So I agree with this, but I'm not sure how to action on it. I don't really want to change https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/thir/enum.ExprKind.html#variant.PointerCoercion, and if we're stuck representing the distinction in MIR anyway, I don't know how helpful restructuring it would be.
What I've grokked here is that early MIR does need this distinction for borrowchecking coercions, but that coercion vs cast distinction is irrelevant in runtime MIR. So it doesn't sound that bad to me to just ban the borrowck-parts in later MIR phases, as I'm doing here? Even if we rearranged these cases to different CastKinds, I think I'd still rather just normalize things so there's fewer things MIR consumers have to deal with.
I don't have a concrete proposal either, and it seems you considered the alternatives, so my concern isn't blocking. I feel like there's got to be a better way but don't feel strongly enough about it to try and find it. ;)
Some changes occurred in compiler/rustc_codegen_cranelift
cc @bjorn3
Some changes occurred to the CTFE / Miri engine
cc @rust-lang/miri
I'm okay with this change because it makes the runtime MIR dialect more sensible. I don't know how much this actually matters in practice, clearly it lets you clean up MIR transforms if you know what you're doing. But the fact that the enum variants still exist because all MIR dialects are the same type still makes implementing transforms confusing.
@bors r+
📌 Commit 64bba35 has been approved by saethlin
It is now in the queue for this repository.
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
jieyouxu added a commit to jieyouxu/rust that referenced this pull request
fmease added a commit to fmease/rust that referenced this pull request
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
Apparently MIR borrowck cares about at least one of these for checking variance.
In runtime MIR, though, there's no need for them as PtrToPtr does the same thing.
(Banning them simplifies passes like GVN that no longer need to handle multiple cast possibilities.)
Rebased and re-blessed.
@bors r=saethlin rollup=iffy (already failed in a rollup once)
📌 Commit e04e351 has been approved by saethlin
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
jieyouxu added a commit to jieyouxu/rust that referenced this pull request
bors added a commit to rust-lang-ci/rust that referenced this pull request
Rollup of 7 pull requests
Successful merges:
- rust-lang#124807 (Migrate
run-make/rustdoc-io-errortormake.rs) - rust-lang#126095 (Migrate
link-args-order,ls-metadataandlto-readonly-librun-maketests tormake) - rust-lang#126308 (Ban
ArrayToPointerandMutToConstPointerfrom runtime MIR) - rust-lang#126620 (Actually taint InferCtxt when a fulfillment error is emitted)
- rust-lang#126629 (Migrate
run-make/compressed-debuginfotormake.rs) - rust-lang#126644 (Rewrite
extern-flag-rename-transitive.debugger-visualizer-dep-info,metadata-flag-frobs-symbols,extern-overrides-distributionandforced-unwind-terminate-pofrun-maketests to rmake) - rust-lang#126650 (Rename a bunch of things in the new solver and
rustc_type_ir)
r? @ghost
@rustbot modify labels: rollup
jieyouxu added a commit to jieyouxu/rust that referenced this pull request
Finished benchmarking commit (3d5d7a2): comparison URL.
Overall result: ✅ improvements - no action needed
@rustbot label: -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) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | -0.3% | [-0.3%, -0.3%] | 1 |
| All ❌✅ (primary) | - | - | 0 |
Max RSS (memory usage)
Results (primary -2.4%, secondary 3.9%)
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 |
| Regressions ❌ (secondary) | 3.9% | [3.1%, 5.4%] | 3 |
| Improvements ✅ (primary) | -2.4% | [-2.4%, -2.4%] | 1 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | -2.4% | [-2.4%, -2.4%] | 1 |
Cycles
This benchmark run did not return any relevant results for this metric.
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 692.485s -> 691.149s (-0.19%)
Artifact size: 323.83 MiB -> 323.84 MiB (0.01%)
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.