detect dyn-unsized arguments before they cause ICEs

[RalfJung]

"Fixes" #115709 in a crude way, with a post-mono check. This is entirely caused by us not having a trait to express "type can have its size determined at runtime".

[rustbot]

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

The Miri subtree was changed

cc @rust-lang/miri

[RalfJung]

I am quite concerned that this will be way too expensive perf-wise, since "post-mono" means "runs a lot for functions that get monomorphized a lot"... I also don't have any good ideas for how to make this check any more efficient, so let's just see what happens.

@bors try @rust-timer queue

[bors]

⌛ Trying commit 356b1c2 with merge f0a3533...

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[RalfJung]

@bors try

[bors]

⌛ Trying commit 0389d09 with merge 6e5c4b6...

[bors]

☀️ Try build successful - checks-actions
Build commit: 6e5c4b6 (6e5c4b60c8be7747ac4f1db1d6ea8d5298e9aac2)

[rust-timer]

Finished benchmarking commit (6e5c4b6): 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)	-	-	0
Regressions ❌ (secondary)	6.1%	[4.1%, 8.1%]	6
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	2
Improvements ✅ (secondary)	-0.5%	[-0.5%, -0.4%]	3
All ❌✅ (primary)	-0.5%	[-0.5%, -0.5%]	2

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)	-	-	0
Regressions ❌ (secondary)	2.7%	[2.7%, 2.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.2%	[-2.4%, -0.6%]	5
All ❌✅ (primary)	-	-	0

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)	-	-	0
Regressions ❌ (secondary)	6.8%	[6.5%, 7.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 629.884s -> 632.427s (0.40%)
Artifact size: 317.26 MiB -> 317.13 MiB (-0.04%)

[RalfJung]

Wait, there's speedup for some tests? That's got to be noise. 😂

For the slowdown, the good news it it's only the CTFE stress test. That one is pretty badly affected though. struct_tail_with_normalize already avoids normalizing unless it is actually required, so I don't see any way to make this any faster.

So... I guess the question is whether it's worth the perf hit to avoid ICEs in the interaction of two unstable features. I don't have a strong opinion on that matter.
Cc @rust-lang/wg-const-eval

[lcnr]

what's the actual status of unsized function arguments right now? if they're still fairly far from getting stabilized, I feel like this check may not be worth it 🤔

[RalfJung]

I have no idea. 🤷

#111175 is definitely a blocker in my view.

[RalfJung]

I can probably disable this check in the interpreter. Miri already doesn't ICE here. Since only the ctfe stress test is affected, it seems like checking this from the codegen backends is actually fine.

[lcnr]

hmm, skimming over the discussions here it feels like unsized function arguments may want to wait on an RFC to detect this before monomorphization

rust-lang/rfcs#2984 and rust-lang/rfcs#3396 are open RFCs that attempt to address this.

so maybe it's better to keep this ICE around and just make it a blocker for stabilization to handle this. Assuming that there's a path forward which eagerly detects these without post-mono errors, that feels preferable to me. I lean towards closing this PR in that case. I may be misinterpreting something here

[RalfJung]

Yeah we probably don't want to stabilize this with a post-mono check.

I'm a bit worried someone might encounter the ICE and try to fix it locally without realizing that this is doomed, but it's probably not worth defending against that. So yeah closing is fine for me.