Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

detect dyn-unsized arguments before they cause ICEs #116115

Closed
wants to merge 1 commit into from

Conversation

RalfJung
Copy link
Member

"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
Copy link
Collaborator

rustbot commented Sep 24, 2023

r? @petrochenkov

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

@rustbot 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 24, 2023
@rustbot
Copy link
Collaborator

rustbot commented Sep 24, 2023

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
Copy link
Member Author

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

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 24, 2023
@bors
Copy link
Contributor

bors commented Sep 24, 2023

⌛ Trying commit 356b1c2 with merge f0a3533...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 24, 2023
detect dyn-unsized arguments before they cause ICEs

"Fixes" rust-lang#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".
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Sep 24, 2023

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@RalfJung
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Sep 24, 2023

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

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 24, 2023
detect dyn-unsized arguments before they cause ICEs

"Fixes" rust-lang#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".
@bors
Copy link
Contributor

bors commented Sep 24, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

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%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 24, 2023
@RalfJung
Copy link
Member Author

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
Copy link
Contributor

lcnr commented Sep 25, 2023

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
Copy link
Member Author

I have no idea. 🤷

#111175 is definitely a blocker in my view.

@RalfJung
Copy link
Member Author

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
Copy link
Contributor

lcnr commented Sep 25, 2023

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
Copy link
Member Author

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.

TaKO8Ki added a commit to TaKO8Ki/rust that referenced this pull request Dec 4, 2023
…iler-errors

more targeted errors when extern types end up in places they should not

Cc rust-lang#115709 -- this does not fix that bug but it makes the panics less obscure and makes it more clear that this is a deeper issue than just a little codegen oversight. (In rust-lang#116115 we decided we'd stick to causing ICEs here for now, rather than nicer errors. We can't currently show any errors pre-mono and probably we don't want post-mono checks when this gets stabilized anyway.)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 4, 2023
Rollup merge of rust-lang#118551 - RalfJung:extern-types-bugs, r=compiler-errors

more targeted errors when extern types end up in places they should not

Cc rust-lang#115709 -- this does not fix that bug but it makes the panics less obscure and makes it more clear that this is a deeper issue than just a little codegen oversight. (In rust-lang#116115 we decided we'd stick to causing ICEs here for now, rather than nicer errors. We can't currently show any errors pre-mono and probably we don't want post-mono checks when this gets stabilized anyway.)
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Dec 5, 2023
more targeted errors when extern types end up in places they should not

Cc rust-lang/rust#115709 -- this does not fix that bug but it makes the panics less obscure and makes it more clear that this is a deeper issue than just a little codegen oversight. (In rust-lang/rust#116115 we decided we'd stick to causing ICEs here for now, rather than nicer errors. We can't currently show any errors pre-mono and probably we don't want post-mono checks when this gets stabilized anyway.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants