Temporarily bring back Rvalue::Len

[lqd]

r? @compiler-errors as requested in #135671 (comment)

However, in the mean time, I'd rather we not crunch trying to find and more importantly validate the soundness of a solution 🤔

Agreed. To fix the IMO P-critical #135671 for which we somehow didn't have test coverage, this PR temporarily reverts:

• Bounds-check with PtrMetadata instead of Len in MIR #133734
• its bugfix Check for array lengths that aren't actually usize #134371
• Delete Rvalue::Len 🎉 #134330

cc @scottmcm

I added the few samples from that issue as a test, but we can add more in the future, in particular it seems @steffahn will work on that.

Fixes #135671. And if we want to land this, it should also be nominated for beta backport.

[rustbot]

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @rust-lang/wg-const-eval

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

This PR changes MIR

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

[compiler-errors]

cc @scottmcm @RalfJung

Let me know if y'all would like to brainstorm how to fix this properly, and let's maybe do another crater run before fully removing the operand again. Crater runs should be really quick these days. I think the right solution would be to emit some sort of special copy operation to clone the slice pointer but allow borrowck and miri to understand that we're only copying the ref to read its metadata, similarly to how we have CopyForDeref for deref'ing which has special semantics. Or perhaps augment the RawRef rvalue operand to pass along the fact that it should act like a fake borrow.

Sorry that the test coverage was lacking; I could not have expected anyone to catch this without a failing test, but thank goodness we have beta backports :)

I'll leave this open for a day or so b/c it's the weekend, but I'll likely r+ this soon thereafter. Also open to just backporting this and trying to find a fix-forward on master, though that comes with all the normal complications of that approach that I think we're all aware of.

[scottmcm]

No worries from my end about reverting these. Definitely better to get the regression fixed.

I can take a couple of simpler smaller steps on the way too, like getting Len out of runtime MIR while still using it for borrowck, while we figure out how best to represent this in earlier phases.

Glad to hear there's a plan to get more tests here!

[compiler-errors]

well if scott is ok, then ill r+ it now

@bors r+ rollup=never

@rustbot label: +beta-nominated

[bors]

📌 Commit c69dea9 has been approved by compiler-errors

It is now in the queue for this repository.

[bors]

⌛ Testing commit c69dea9 with merge c62b732...

[bors]

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing c62b732 to master...

[rust-timer]

Finished benchmarking commit (c62b732): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

• If the regression was expected or you think it can be justified,
  please write a comment with sufficient written justification, and add
  @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
• If you think that you know of a way to resolve the regression, try to create
  a new PR with a fix for the regression.
• If you do not understand the regression or you think that it is just noise,
  you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
  were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.3%, 0.3%]	1
Regressions ❌ (secondary)	0.4%	[0.2%, 0.5%]	6
Improvements ✅ (primary)	-0.7%	[-1.3%, -0.4%]	3
Improvements ✅ (secondary)	-0.9%	[-1.7%, -0.2%]	2
All ❌✅ (primary)	-0.4%	[-1.3%, 0.3%]	4

Max RSS (memory usage)

Results (primary -2.3%, secondary -1.6%)

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)	5.6%	[5.6%, 5.6%]	1
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-3.2%	[-11.9%, -0.7%]	9
Improvements ✅ (secondary)	-2.9%	[-3.7%, -2.1%]	3
All ❌✅ (primary)	-2.3%	[-11.9%, 5.6%]	10

Cycles

Results (primary -1.1%, secondary 1.7%)

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)	4.2%	[4.0%, 4.6%]	3
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-2.0%	[-2.2%, -1.9%]	2
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

Binary size

Results (primary 0.0%, secondary 0.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)	0.2%	[0.0%, 0.6%]	18
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	3
Improvements ✅ (primary)	-0.1%	[-0.7%, -0.0%]	16
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.7%, 0.6%]	34

Bootstrap: 765.901s -> 765.96s (0.01%)
Artifact size: 326.07 MiB -> 325.96 MiB (-0.04%)

[lqd]

Revert

@rustbot label: +perf-regression-triaged

[RalfJung]

I think the right solution would be to emit some sort of special copy operation to clone the slice pointer but allow borrowck and miri to understand that we're only copying the ref to read its metadata, similarly to how we have CopyForDeref for deref'ing which has special semantics. Or perhaps augment the RawRef rvalue operand to pass along the fact that it should act like a fake borrow.

Those sound like promising approaches. Adding this info to Rvalue::RawRef does seem a bit hacky, but if both borrowck and Miri need this info then maybe it's not so bad after all? I hope Miri can stop needing this in the future, but that needs more work -- ironically also related to s.len() calls; specifically we need a way for the implicit borrow of s here to not lead to a retag.

[cuviper]

Returning this to pending backports because it somehow caused a failure in #136017 (comment).

@rustbot label +beta-nominated
(and still beta-accepted)

[cuviper]

Responding to @lqd from #136017 (comment):

The fact this only appears at stage 2 sounds bad.

Yes, and only on some targets -- smells like UB or similar codegen trouble...

The parent commit of #133734 expectedly doesn't trigger the issue either. I've looked for other PRs that may be related and missing from backports, there was one related to PtrMetadata but is not related to this issue — and other than that nothing jumped at me (but may have missed it, there are 450 PRs).

As I said, I can reproduce the stack overflow, so if anyone has ideas on how to identify what's going on on beta and doesn't on nightly, via complex git bisections or else, I can test them out. Other than that I'm not sure how to make tangible progress.

I tried a complex git bisection between here and beta, cherry-picking this PR at each step and running tests/ui/issues/issue-74564-if-expr-stack-overflow.rs. I landed on #135046, commit ac9cb90, as the first base where these backports pass that test; the parent bf6f8a4 passes on its own but overflows with these backports.

I'm not sure what to make of that, because it doesn't seem like this should have anything to do with Box::new.

Here's the backtrace I get on the overflow, demangled by rustfilt:

--- stderr -------------------------------
error: rustc interrupted by SIGSEGV, printing backtrace

/home/gh-cuviper/rust/build/aarch64-unknown-linux-gnu/stage2/lib/librustc_driver-8ea34122c6730781.so(+0xc8b7bc)[0xe9a629a8b7bc]
linux-vdso.so.1(__kernel_rt_sigreturn+0x0)[0xe9a632b418f8]
/home/gh-cuviper/rust/build/aarch64-unknown-linux-gnu/stage2/lib/librustc_driver-8ea34122c6730781.so(+0x4b75f40)[0xe9a62d975f40]

### cycle encountered after 3 frames with period 4
/home/gh-cuviper/rust/build/aarch64-unknown-linux-gnu/stage2/lib/librustc_driver-8ea34122c6730781.so(<rustc_mir_build::check_tail_calls::TailCallCkVisitor as rustc_middle::thir::visit::Visitor>::visit_expr+0x334)[0xe9a62d94e258]
/home/gh-cuviper/rust/build/aarch64-unknown-linux-gnu/stage2/lib/librustc_driver-8ea34122c6730781.so(<rustc_mir_build::check_tail_calls::TailCallCkVisitor as rustc_middle::thir::visit::Visitor>::visit_expr+0x334)[0xe9a62d94e258]
/home/gh-cuviper/rust/build/aarch64-unknown-linux-gnu/stage2/lib/librustc_driver-8ea34122c6730781.so(+0x4b76448)[0xe9a62d976448]
/home/gh-cuviper/rust/build/aarch64-unknown-linux-gnu/stage2/lib/librustc_driver-8ea34122c6730781.so(<rustc_mir_build::check_tail_calls::TailCallCkVisitor as rustc_middle::thir::visit::Visitor>::visit_expr+0x334)[0xe9a62d94e258]
### recursed 61 times

I'm not sure what tail calls have to do with these changes or that test either.

[cuviper]

I admit I was assuming that this was an infinite recursion, but on a whim I actually followed the hint:

help: you can increase rustc's stack size by setting RUST_MIN_STACK=16777216

This works, and even 9MB is enough for this test. Further, tweaking it down to 4MB also makes the current beta and nightly compilers fail in the same TailCallCkVisitor::visit_expr. So it seems that this PR is probably increasing the size of stack frames somewhere to reveal an existing hazard.

Maybe the box_new change is doing a better job of avoiding temporaries than before? That could be a reason why it helps avoid this too.

[bjorn3]

Can we make rustc's segfault handler detect stack overflows too just like libstd's segfault handler?

[cuviper]

It does, approximately:

rust/compiler/rustc_driver_impl/src/signal_handler.rs

Lines 98 to 101 in a1d7676

	// technically speculation, but assert it with confidence anyway.
	// rustc only arrived in this signal handler because bad things happened
	// and this message is for explaining it's not the programmer's fault
	raw_errln!("note: rustc unexpectedly overflowed its stack! this is a bug");

[cuviper]

This is enough to avoid the TailCallCkVisitor::visit_expr overflow in that test:

--- a/compiler/rustc_mir_build/src/check_tail_calls.rs
+++ b/compiler/rustc_mir_build/src/check_tail_calls.rs
@@ -52,2 +52,3 @@ struct TailCallCkVisitor<'a, 'tcx> {
 impl<'tcx> TailCallCkVisitor<'_, 'tcx> {
+    #[cold]
     fn check_tail_call(&mut self, call: &Expr<'_>, expr: &Expr<'_>) {

(#[inline(never)] works too)

Would anyone object to me adding that to the beta backport?

[lqd]

I don't think anyone would, but maybe it can be discussed in tomorrow's t-compiler meeting if you'd like?

[cuviper]

Sure, please do mention it, even if only to raise awareness of the recursion hazard there. I'll wait until after that meeting before I start another backport round anyway, in case there's anything else new.

[RalfJung]

Sounds like a missing stacker guard somewhere? rustc is supposed to grow its stacks on-demand.

[lqd]

The PR that replaced the original ones in this revert also has apparently landed, and maybe that can come into the backport discussion so I’ll mention both tomorrow. cc @apiraino for the agenda

[lqd]

This was discussed in today's t-compiler meeting:

• since this isn't only about apple and can be reproduced on aarch64-linux, @saethlin volunteered to investigate the stack usage & overflow, and the appropriate fix 🙏
• there's no pressure, we have the #[cold]/#[inline(never)] workaround we can land if there's no time or we don't find anything
• we'll reassess this in the next meeting or the one after that, to make sure sure that we land something before the reverted PRs would hit stable

[cuviper]

Here's a quick example of the stack overflow with nightly on aarch64-linux:

gh-cuviper@dev-desktop-us-1:~/rust$ rustc +nightly -V
rustc 1.86.0-nightly (ae5de6c75 2025-01-29)

gh-cuviper@dev-desktop-us-1:~/rust$ rustc +nightly tests/ui/issues/issue-74564-if-expr-stack-overflow.rs
warning: function `banana` is never used
 --> tests/ui/issues/issue-74564-if-expr-stack-overflow.rs:5:4
  |
5 | fn banana(v: &str) -> u32 {
  |    ^^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

warning: 1 warning emitted

gh-cuviper@dev-desktop-us-1:~/rust$ RUST_MIN_STACK=$((1024*1024*4)) rustc +nightly tests/ui/issues/issue-74564-if-expr-stack-overflow.rs
error: rustc interrupted by SIGSEGV, printing backtrace

/home/gh-cuviper/.rustup/toolchains/nightly-aarch64-unknown-linux-gnu/lib/librustc_driver-6c17675a58d30cbe.so(+0x2fd4254)[0xfdd9073d4254]
linux-vdso.so.1(__kernel_rt_sigreturn+0x0)[0xfdd9086b08f8]
/home/gh-cuviper/.rustup/toolchains/nightly-aarch64-unknown-linux-gnu/lib/librustc_driver-6c17675a58d30cbe.so(_RNvXs_NtCskpTNR5DnBHI_15rustc_mir_build16check_tail_callsNtB4_17TailCallCkVisitorNtNtNtCsgG0pwfDM3lG_12rustc_middle4thir5visit7Visitor10visit_expr+0x0)[0xfdd9063fc074]
[...]

So currently nightly only fails if you tighten the stack, but beta with this PR backported will even fail with the default stack.

[lqd]

(fun fact, the unsafety visitor has the same issue at that min stack size) (edit: couldn't sleep, #136352 should fix the issues and make the backport succeed)

[lqd]

(cross-posting for traceability) With #136352 and its accepted beta-backport, we should also be good to go for this backport here.