Remove support for dynamic allocas #142911

mejrs · 2025-06-23T14:34:51Z

Followup to #141811

rustbot · 2025-06-23T14:34:56Z

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 · 2025-06-23T14:34:59Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

petrochenkov · 2025-06-23T14:48:41Z

The removal part LGTM, the type checker part I don't know.
r? @compiler-errors as the reviewer of #141811.

bjorn3 · 2025-06-23T15:54:07Z

compiler/rustc_codegen_ssa/src/mir/mod.rs

    /// `UnsizedPlace(p)`: `p` itself is a thin pointer (indirect place).
    /// `*p` is the wide pointer that references the actual unsized place.
-    /// Every time it is initialized, we have to reallocate the place
-    /// and update the wide pointer. That's the reason why it is indirect.
+    ///
+    /// Rust has no alloca and thus no ability to move the unsized place.


Could this be made a direct place in the future? If so maybe add a FIXME?

I don't know anything about codegen, so I don't know about that. There are quite a few occasions of UnsizedPlace(_) => bug!() though, so it does look like something that could be refactored.

compiler-errors · 2025-06-23T17:15:03Z

tests/ui/unsized-locals/scope_unsized.rs

+//! `#![feature(unsized_fn_params)]` lets you use unsized function parameters. In particular this
+//! is load bearing for `Box<dyn FnOnce()>: FnOnce()`. To do that, borrowck relaxes the requirement
+//! that certain places must be `Sized`. That means when `unsized_fn_params` is enabled, we must
+//! explicitly check for the sizedness of various expressions to not ICE at codegen. Note that it is
+//! specifically the blocks and scopes that are problematic; `udrop::<[u8]>(*foo());` is just fine.
+//! Also see tests/ui/unsized_locals/unsized-exprs-rpass.rs


I'm a bit skeptical about this.

I don't think the fact that there's a block or a "scope" (not sure what actually is mean by this) is problematic. Instead, I think in order to get arguments that don't require alloca's, they must be unsized place expressions. Does that make sense, and might that seem more consistent with the desugaring?

My understanding is that when we convert the unsized arg back to a pointer for the fn ABI, we just take a ref of the place if it's a place expression, rather than having to alloca storage for the value if it's a temporary.

Specifically, nothing strikes me as particularly unique about block expressions. They're, AFAICT, just temporaries that evaluate to their final expression. They obviously have some special behavior with promotion, but that shouldn't have any impact here.

For example, the difference between udrop(*foo()); and udrop({ *foo() }) is that the arg in the latter example is not a place.

I'm not sure about the wording either. I appreciate the help.

Instead, I think in order to get arguments that don't require alloca's, they must be unsized place expressions. Does that make sense, and might that seem more consistent with the desugaring?

That is a much nicer and principled way to couch it. What is the best way to implement that? I don't see any obvious "is this a place" functionality in borrowck. or am I better off staying in typeck and just checking for all ExprKinds that I know are (not) place expressions like not a Deref/Index/Field etc expression?

Specifically, nothing strikes me as particularly unique about block expressions. They're, AFAICT, just temporaries that evaluate to their final expression. They obviously have some special behavior with promotion, but that shouldn't have any impact here.

Using them as the final operand of a block expression causes a move, and you don't have to bind them to anything to do that which is why it doesn't trigger existing Sized checks like assigning to a variable would do. That and the loop break example are the only way to do it as far as I can tell.

What is the best way to implement that?

Well, two things I'd change here:

Use is_place_expr. Look through typeck for other uses of this function to use it correctly. This is the most consistent way to check that something is a place in HIR typeck.

Move this check into argument checking, since this AFAICT only matters for unsized args.

Does the case where this is a bare expression actually matter? Like, the { *foo(); } case? In that case, I'd still really rather keep this out of the general check_expr_kind function, and instead check the types of unsized statement-expressions. In that case, this may be duplicated in two places -- argument checking, and the StmtKind::Semi branch of check_stmt.

That and the loop break example are the only way to do it as far as I can tell.

Match expressions too?

Oh yeah, match foo() { x => *x } is an dynamic alloca as well. (can't unsize in the scrutinee, that's an error)

Does the case where this is a bare expression actually matter?

Yeah, you will overflow the stack with a program like this (with any of the "block like" expressions discussed so far)

#![feature(unsized_fn_params)] use std::hint::black_box; fn udrop(t: [u8]) { black_box(&t); } fn foo() -> Box<[u8]> { Box::new([0; 1000]) } fn main() { for _ in 0..100000 { udrop({ *foo() }); } }

Hm, it's not just about call arguments. If you have

pub fn foo() -> Box<[u8]> { Box::new([0;1000]) } fn main() { { *foo() }; }

then that doesn't compile. But if you enable unsized_fn_params, it starts compiling. I think that should always be an error. At least it doesn't end up alloca'ing, I guess.

That's why I said:

I'd still really rather keep this out of the general check_expr_kind function, and instead check the types of unsized statement-expressions. In that case, this may be duplicated in two places -- argument checking, and the StmtKind::Semi branch of check_stmt.

If you implemented that, then the expression { *foo() }; should fail.

mejrs · 2025-06-24T21:50:15Z

I've implemented the checks. Maybe the error messages could use some work, but I'm not too fussed about it given they should only pop up if you incorrectly use an internal feature...

@rustbot ready

compiler-errors

Thanks. I do have one nit that I was going to let slide but it's pretty pervasive through the PR so I'd really prefer if it was fixed up really quick.

Right now you mention "because we can't move them" as the justification for why we require non-place (i.e. temp) expressions to be sized, in comments and in the ObligationCauseCode message -- that's not totally accurate, since in the callee of an unsized arg, the arg will be moved from (like, that's why we consume the Box in <Box<dyn FnOnce> as FnOnce>).

I'd really prefer if we say "because we can't store them in MIR locals as temporaries" or something, since it's more precise. Could you fix that, and also perhaps link to this PR somewhere, or add additional justification that this check is specifically because we've removed support for dynamic alloca's and/or unsized locals?

Thanks. r=me after.

compiler-errors · 2025-06-25T01:57:51Z

@bors delegate+

bors · 2025-06-25T01:57:55Z

✌️ @mejrs, you can now approve this pull request!

If @compiler-errors told you to "r=me" after making some further change, please make that change, then do @bors r=@compiler-errors

workingjubilee

"alloca" is technically also an idea in LLVM that is used with statically-known arguments and creates the expected known-sized stack storage, so we should be clear what we're interacting with here.

workingjubilee · 2025-07-07T23:52:00Z

compiler/rustc_codegen_ssa/src/mir/mod.rs

-    /// Every time it is initialized, we have to reallocate the place
-    /// and update the wide pointer. That's the reason why it is indirect.
+    ///
+    /// Rust has no alloca anymore, so we cannot reallocate from the place.


Suggested change

/// Rust has no alloca anymore, so we cannot reallocate from the place.

/// MIR only supports unsized args, not dynamically-sized locals, so

/// new unsized temps don't exist and we must reuse the referred-to place.

mejrs · 2025-07-09T10:56:58Z

@bors r=compiler-errors

bors · 2025-07-09T10:57:02Z

📌 Commit 25eb382 has been approved by compiler-errors

It is now in the queue for this repository.

Remove support for dynamic allocas Followup to rust-lang#141811

Rollup of 9 pull requests Successful merges: - #141996 (Fix `proc_macro::Ident`'s handling of `$crate`) - #142911 (Remove support for dynamic allocas) - #142950 (mbe: Rework diagnostics for metavariable expressions) - #143270 (tests/codegen/enum/enum-match.rs: accept negative range attribute) - #143298 (`tests/ui`: A New Order [23/N]) - #143398 (tidy: add support for `--extra-checks=auto:` feature) - #143632 (fix: correct parameter names in LLVMRustBuildMinNum and LLVMRustBuildMaxNum FFI declarations) - #143644 (Add triagebot stdarch mention ping) - #143651 (Win: Use exceptions with empty data for SEH panic exception copies instead of a new panic) r? `@ghost` `@rustbot` modify labels: rollup

tgross35 · 2025-07-10T07:22:18Z

Possible failure at #143715 (comment) since nothing else touched codegen

@bors r-
@bors2 try jobs=i686-gnu-nopt-2

rust-bors · 2025-07-10T07:22:22Z

⌛ Trying commit 25eb382 with merge 25e4170…

To cancel the try build, run the command @bors2 try cancel.

Remove support for dynamic allocas Followup to #141811 try-job: i686-gnu-nopt-2

rust-bors · 2025-07-10T13:27:07Z

💥 Test timed out

tgross35 · 2025-07-10T18:46:34Z

Trying to get a concrete answer...
@bors2 try jobs=i686-gnu-nopt-2

Remove support for dynamic allocas Followup to #141811 try-job: i686-gnu-nopt-2

rust-bors · 2025-07-10T18:46:39Z

⌛ Trying commit 25eb382 with merge 72a248e…

To cancel the try build, run the command @bors2 try cancel.

rust-bors · 2025-07-10T21:24:31Z

☀️ Try build successful (CI)
Build commit: 72a248e (72a248e538208fba511e98c115e3ec34915e1c0d, parent: 78a6e132984dba0303ebad7dcfd1305c93ad5835)

workingjubilee · 2025-07-11T00:31:53Z

seems fine.

@bors r=compiler-errors rollup=never

bors · 2025-07-11T00:31:56Z

📌 Commit 25eb382 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2025-07-11T05:27:35Z

⌛ Testing commit 25eb382 with merge 855e0fe...

bors · 2025-07-11T08:29:15Z

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

github-actions · 2025-07-11T08:32:48Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing cdac44e (parent) -> 855e0fe (this PR)

Test differences

Show 7 test diffs

Stage 1

[ui] tests/ui/unsized-locals/unsized-non-place-exprs.rs: [missing] -> pass (J1)

Stage 2

[ui] tests/ui/unsized-locals/unsized-non-place-exprs.rs: [missing] -> pass (J0)

Additionally, 5 doctest diffs were found. These are ignored, as they are noisy.

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 855e0fe46e68d94e9f6147531b75ac2d488c548e --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

dist-apple-various: 6820.5s -> 5118.5s (-25.0%)
dist-x86_64-apple: 9535.3s -> 7593.2s (-20.4%)
pr-check-1: 1544.4s -> 1856.3s (20.2%)
x86_64-apple-2: 6111.4s -> 5055.8s (-17.3%)
x86_64-apple-1: 6454.6s -> 7454.3s (15.5%)
tidy: 71.4s -> 64.9s (-9.1%)
dist-arm-linux-gnueabi: 4745.3s -> 5155.0s (8.6%)
aarch64-apple: 4749.7s -> 4373.7s (-7.9%)
x86_64-gnu-llvm-19-2: 5944.2s -> 6414.6s (7.9%)
dist-loongarch64-linux: 6088.4s -> 5661.9s (-7.0%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-07-11T09:40:06Z

Finished benchmarking commit (855e0fe): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary -3.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.5%	[-4.6%, -1.9%]	4
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -0.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.5%	[3.5%, 3.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.3%	[-2.5%, -2.0%]	2
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 464.73s -> 464.604s (-0.03%)
Artifact size: 374.55 MiB -> 374.58 MiB (0.01%)

rustbot assigned petrochenkov Jun 23, 2025

rustbot assigned compiler-errors and unassigned petrochenkov Jun 23, 2025

mejrs mentioned this pull request Jun 23, 2025

Unimplement unsized_locals #141811

Merged

bjorn3 reviewed Jun 23, 2025

View reviewed changes

compiler-errors requested changes Jun 23, 2025

View reviewed changes

rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 23, 2025

mejrs force-pushed the unsized branch from 930c08d to c62e18f Compare June 24, 2025 20:16

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 24, 2025

compiler-errors approved these changes Jun 25, 2025

View reviewed changes

Remove support for dynamic allocas

49421d1

mejrs force-pushed the unsized branch 3 times, most recently from f9f886c to f972fbf Compare July 7, 2025 22:28

This comment has been minimized.

Sign in to view

workingjubilee reviewed Jul 7, 2025

View reviewed changes

mejrs force-pushed the unsized branch from f972fbf to 52de720 Compare July 8, 2025 11:58

This comment has been minimized.

Sign in to view

Error on moving unsized values rather than ICE'ing

25eb382

mejrs force-pushed the unsized branch from 52de720 to 25eb382 Compare July 8, 2025 20:37

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 Jul 9, 2025

tgross35 added a commit to tgross35/rust that referenced this pull request Jul 10, 2025

Rollup merge of rust-lang#142911 - mejrs:unsized, r=compiler-errors

9e95e4b

Remove support for dynamic allocas Followup to rust-lang#141811

tgross35 mentioned this pull request Jul 10, 2025

Rollup of 9 pull requests #143715

Closed

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 Jul 10, 2025

rust-bors bot added a commit that referenced this pull request Jul 10, 2025

Auto merge of #142911 - mejrs:unsized, r=<try>

25e4170

Remove support for dynamic allocas Followup to #141811 try-job: i686-gnu-nopt-2

rust-bors bot added a commit that referenced this pull request Jul 10, 2025

Auto merge of #142911 - mejrs:unsized, r=<try>

72a248e

Remove support for dynamic allocas Followup to #141811 try-job: i686-gnu-nopt-2

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 Jul 11, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 11, 2025

bors merged commit 855e0fe into rust-lang:master Jul 11, 2025
13 checks passed

rustbot added this to the 1.90.0 milestone Jul 11, 2025

	/// Rust has no alloca anymore, so we cannot reallocate from the place.
	/// MIR only supports unsized args, not dynamically-sized locals, so
	/// new unsized temps don't exist and we must reuse the referred-to place.

Remove support for dynamic allocas #142911

Remove support for dynamic allocas #142911

Conversation

mejrs commented Jun 23, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 23, 2025

Uh oh!

rustbot commented Jun 23, 2025

Uh oh!

petrochenkov commented Jun 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mejrs Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mejrs Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mejrs commented Jun 24, 2025

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Jun 25, 2025

Uh oh!

bors commented Jun 25, 2025

Uh oh!

This comment has been minimized.

workingjubilee left a comment

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

mejrs commented Jul 9, 2025

Uh oh!

bors commented Jul 9, 2025

Uh oh!

tgross35 commented Jul 10, 2025

Uh oh!

rust-bors bot commented Jul 10, 2025

Uh oh!

rust-bors bot commented Jul 10, 2025

Uh oh!

tgross35 commented Jul 10, 2025

Uh oh!

rust-bors bot commented Jul 10, 2025

Uh oh!

rust-bors bot commented Jul 10, 2025

Uh oh!

mejrs commented Jun 23, 2025 •

edited by rustbot

Loading

compiler-errors Jun 23, 2025 •

edited

Loading

compiler-errors Jun 23, 2025 •

edited

Loading

mejrs Jun 23, 2025 •

edited

Loading

mejrs Jun 23, 2025 •

edited

Loading

compiler-errors Jun 23, 2025 •

edited

Loading

workingjubilee Jul 7, 2025 •

edited

Loading