Simple validation for unsize coercion in MIR validation #130735

compiler-errors · 2024-09-23T08:03:02Z

This adds the most basic validity check to unsize coercions in MIR. The src and target of an unsize cast must at least implement Src: CoerceUnsized<Target> for this to be valid.

This doesn't the second, more subtle validity check that is taken of advantage in codegen here, but I did leave a beefy FIXME for that explaining what it is.

As a consequence, this also fixes an ICE with GVN and invalid unsize coercions. This is somewhat coincidental, since MIR inlining will check that a body is valid before inlining it; so now that we determine it to be invalid, we don't inline it, and we don't encounter the GVN ICE. I'm not certain if the same GVN ICE is triggerable without the inliner, and perhaps instead with trivial where clauses or something.

cc @RalfJung

rustbot · 2024-09-23T08:03:10Z

r? @petrochenkov

rustbot has assigned @petrochenkov.
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-09-23T08:03:12Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler-errors · 2024-09-23T08:03:34Z

compiler/rustc_mir_transform/src/validate.rs

@@ -586,6 +589,22 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {

        crate::util::relate_types(self.tcx, self.param_env, variance, src, dest)
    }
+
+    /// Check that the given predicate definitely holds in the param-env of this MIR body.


Do we already have a "test this predicate holds" function somewhere in the validator? I couldn't find one when doing a simple search.

compiler/rustc_mir_transform/src/validate.rs

petrochenkov · 2024-09-23T11:35:39Z

r? @RalfJung

RalfJung · 2024-09-23T14:23:06Z

This is doing trait query stuff, I have no idea if that's done correctly...
r? types

rustbot · 2024-09-23T17:52:36Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

spastorino · 2024-09-24T21:21:41Z

@bors r+

bors · 2024-09-24T21:21:44Z

📌 Commit 6bdfd13 has been approved by spastorino

It is now in the queue for this repository.

Rollup of 7 pull requests Successful merges: - rust-lang#130234 (improve compile errors for invalid ptr-to-ptr casts with trait objects) - rust-lang#130752 (Improve assembly test for CMSE ABIs) - rust-lang#130764 (Separate collection of crate-local inherent impls from error tracking) - rust-lang#130788 (Pin memchr to 2.5.0 in the library rather than rustc_ast) - rust-lang#130789 (add InProgress ErrorKind gated behind io_error_inprogress feature) - rust-lang#130793 (Mention `COMPILETEST_VERBOSE_CRASHES` on crash test failure) - rust-lang#130798 (rustdoc: inherit parent's stability where applicable) Failed merges: - rust-lang#130735 (Simple validation for unsize coercion in MIR validation) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2024-09-25T04:56:04Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout validate-unsize (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self validate-unsize --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Removing tests/crashes/129219.rs
Auto-merging compiler/rustc_mir_transform/src/validate.rs
CONFLICT (content): Merge conflict in compiler/rustc_mir_transform/src/validate.rs
Automatic merge failed; fix conflicts and then commit the result.

bors · 2024-09-25T05:50:25Z

☔ The latest upstream changes (presumably #130807) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2024-09-25T06:54:50Z

compiler/rustc_codegen_ssa/src/base.rs

+                // and because higher-ranked equality now requires the binders are equal.
+                debug_assert_eq!(
+                    data_a.principal(),
+                    data_b.principal(),


That doesn't check that the projections are equal though, does it?

No but there is not an upcast for which this is valid. That would literally be unsound in the type system.

Like, checking projection validity seems more appropriate as an assertion on the Unsize trait goal in the trait system or something, but even that seems excessive.

Like, the only reason miri needs to check projections are compatible is because of transmutes.

And also because I don't trust the library/ and would prefer this to be double-checked in the compiler. ;)

But maybe that's overly paranoid, and anyway it's a different PR. I just wanted to understand what is and is not being checked here, and the comments should reflect that.

And also because I don't trust the library/ and would prefer this to be double-checked in the compiler. ;)

It's not really library/, it's just the soundness of the Unsize goal, and the fact that you can't impl impl Foo for Bar { type Assoc = A; } and impl Foo for Bar { type Assoc = B; }. That is, normalization is a function.

RalfJung · 2024-09-25T06:56:14Z

compiler/rustc_codegen_ssa/src/base.rs

                // A NOP cast that doesn't actually change anything, should be allowed even with
                // invalid vtables.


Suggested change

// A NOP cast that doesn't actually change anything, should be allowed even with

// invalid vtables.

// A NOP cast that doesn't actually change anything, let's avoid any unnecessary work.

// This relies on the assumption that if the principal traits are equal, then the associated type

// bounds (`dyn Trait<Assoc=T>`) are also equal, which is ensured by ....

Not sure what does at the end of this sentence :)

…pal trait def id are truly NOPs

RalfJung · 2024-09-25T17:17:54Z

Thanks. :)
@bors r=spastorino,RalfJung

bors · 2024-09-25T17:17:57Z

📌 Commit 3209943 has been approved by spastorino,RalfJung

It is now in the queue for this repository.

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#130735 (Simple validation for unsize coercion in MIR validation) - rust-lang#130781 (Fix up setting strip = true in Cargo.toml makes build scripts fail in…) - rust-lang#130811 (add link from random() helper fn to extensive DefaultRandomSource docs) - rust-lang#130819 (Add `must_use` attribute to `len_utf8` and `len_utf16`.) - rust-lang#130832 (fix some cfg logic around optimize_for_size and 16-bit targets) - rust-lang#130842 (Add tracking issue for io_error_inprogress) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#130735 - compiler-errors:validate-unsize, r=spastorino,RalfJung Simple validation for unsize coercion in MIR validation This adds the most basic validity check to unsize coercions in MIR. The src and target of an unsize cast must *at least* implement `Src: CoerceUnsized<Target>` for this to be valid. This doesn't the second, more subtle validity check that is taken of advantage in codegen [here](https://github.com/rust-lang/rust/blob/914193c8f40528fe82696e1054828de8c399882e/compiler/rustc_codegen_ssa/src/base.rs#L126), but I did leave a beefy FIXME for that explaining what it is. As a consequence, this also fixes an ICE with GVN and invalid unsize coercions. This is somewhat coincidental, since MIR inlining will check that a body is valid before inlining it; so now that we determine it to be invalid, we don't inline it, and we don't encounter the GVN ICE. I'm not certain if the same GVN ICE is triggerable without the inliner, and perhaps instead with trivial where clauses or something. cc `@RalfJung`

Kobzol · 2024-10-01T07:23:03Z

@rust-timer build 323f521

rust-timer · 2024-10-01T09:37:12Z

Finished benchmarking commit (323f521): comparison URL.

Overall result: ❌ regressions - 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)	0.6%	[0.2%, 0.9%]	12
Regressions ❌ (secondary)	0.5%	[0.2%, 1.7%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.2%, 0.9%]	12

Max RSS (memory usage)

Results (secondary -1.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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.9%	[-1.9%, -1.9%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 1.3%, secondary 7.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)	1.3%	[0.8%, 1.5%]	5
Regressions ❌ (secondary)	7.3%	[4.1%, 10.2%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.3%	[0.8%, 1.5%]	5

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 769.756s -> 773.205s (0.45%)
Artifact size: 340.86 MiB -> 340.87 MiB (0.00%)

Kobzol · 2024-10-01T09:55:14Z

This PR caused the small regression in #130847. I think it's fine, since it adds new functionality (new form of MIR validation), so marking as triaged.

@rustbot label: +perf-regression-triaged

rustbot assigned petrochenkov Sep 23, 2024

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

compiler-errors commented Sep 23, 2024

View reviewed changes

RalfJung reviewed Sep 23, 2024

View reviewed changes

compiler/rustc_mir_transform/src/validate.rs Outdated Show resolved Hide resolved

rustbot assigned RalfJung and unassigned petrochenkov Sep 23, 2024

rustbot added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Sep 23, 2024

rustbot assigned spastorino and unassigned RalfJung Sep 23, 2024

compiler-errors force-pushed the validate-unsize branch from bb7137a to 6bdfd13 Compare September 23, 2024 17:52

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 Sep 24, 2024

tgross35 mentioned this pull request Sep 24, 2024

Rollup of 7 pull requests #130807

Merged

bors added S-waiting-on-author 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 Sep 25, 2024

RalfJung reviewed Sep 25, 2024

View reviewed changes

compiler-errors added 2 commits September 25, 2024 11:10

Validate unsize coercion in MIR validation

8fc8e03

Add a debug assertion in codegen that unsize casts of the same princi…

3209943

…pal trait def id are truly NOPs

compiler-errors force-pushed the validate-unsize branch from 6bdfd13 to 3209943 Compare September 25, 2024 15:14

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 25, 2024

matthiaskrgr mentioned this pull request Sep 25, 2024

Rollup of 6 pull requests #130847

Merged

bors merged commit 0055895 into rust-lang:master Sep 25, 2024
6 checks passed

rustbot added this to the 1.83.0 milestone Sep 25, 2024

ehuss mentioned this pull request Sep 25, 2024

ICE: NOP unsize vtable changed principal trait ref #130855

Closed

This comment has been minimized.

Sign in to view

rustbot added the perf-regression Performance regression. label Oct 1, 2024

rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple validation for unsize coercion in MIR validation #130735

Simple validation for unsize coercion in MIR validation #130735

compiler-errors commented Sep 23, 2024

rustbot commented Sep 23, 2024

rustbot commented Sep 23, 2024

compiler-errors Sep 23, 2024

petrochenkov commented Sep 23, 2024

RalfJung commented Sep 23, 2024

rustbot commented Sep 23, 2024

spastorino commented Sep 24, 2024

bors commented Sep 24, 2024

bors commented Sep 25, 2024

bors commented Sep 25, 2024

RalfJung Sep 25, 2024

compiler-errors Sep 25, 2024

compiler-errors Sep 25, 2024

RalfJung Sep 25, 2024

compiler-errors Sep 25, 2024

RalfJung Sep 25, 2024

RalfJung commented Sep 25, 2024

bors commented Sep 25, 2024

Kobzol commented Oct 1, 2024

This comment has been minimized.

rust-timer commented Oct 1, 2024

Kobzol commented Oct 1, 2024

		// A NOP cast that doesn't actually change anything, should be allowed even with
		// invalid vtables.

-                // A NOP cast that doesn't actually change anything, should be allowed even with
-                // invalid vtables.
+                // A NOP cast that doesn't actually change anything, let's avoid any unnecessary work.
+                // This relies on the assumption that if the principal traits are equal, then the associated type
+                // bounds (`dyn Trait<Assoc=T>`) are also equal, which is ensured by ....

Simple validation for unsize coercion in MIR validation #130735

Simple validation for unsize coercion in MIR validation #130735

Conversation

compiler-errors commented Sep 23, 2024

rustbot commented Sep 23, 2024

rustbot commented Sep 23, 2024

Choose a reason for hiding this comment

petrochenkov commented Sep 23, 2024

RalfJung commented Sep 23, 2024

rustbot commented Sep 23, 2024

spastorino commented Sep 24, 2024

bors commented Sep 24, 2024

bors commented Sep 25, 2024

bors commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Sep 25, 2024

bors commented Sep 25, 2024

Kobzol commented Oct 1, 2024

This comment has been minimized.

rust-timer commented Oct 1, 2024

Overall result: ❌ regressions - ACTION NEEDED

Kobzol commented Oct 1, 2024