Don't ICE in `might_permit_raw_init` if reference is polymorphic #108012

compiler-errors · 2023-02-13T21:10:27Z

Emitting optimized MIR for a polymorphic function may require computing layout of a type that isn't (yet) known. This happens in the instcombine pass, for example. Let's fail gracefully in that condition.

cc @saethlin
fixes #107999

rustbot · 2023-02-13T21:10:33Z

r? @oli-obk

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

tests/ui/lint/invalid_value-polymorphic.rs

compiler-errors · 2023-02-14T00:04:38Z

compiler/rustc_const_eval/src/util/might_permit_raw_init.rs

+        let Ok(pointee) = cx.layout_of(pointee.ty) else {
+            // Reference is too polymorphic, it has a layout but the pointee does not.
+            assert!(pointee.ty.needs_subst());
+            return false;


oh, lol, this is totally wrong. this is might_permit_raw_init_lax, not known_to_permit_raw_init_lax (or something worded like that) :/ this probably should return true in that case...

@saethlin, are you sure that it's valid to instcombine away an uninit validity assertion in this case? really we should be returning something ternary -- yes, no, or "too generic" (or at least, "unknown layout"), and in the case of the last option, we don't do anything in the optimization.

Not sure what you're asking, but I'll guess.

If you are asking me what the correct thing to do instead of ICEing here is, no idea. The upstream effect here should be not applying the optimization- we don't know if T has alignment > 1, in which case the 0x1-filling is invalid.

If you're asking about the code I added to InstCombine, I intended to nab that directly from what codegen does when it decides to emit a panic or not for that intrinsic. Now it's a fair question as to whether what codegen does just behaves nonsensically when given a generic, if code is supposed to be monomorphized at that stage.

What I'm saying is that it's probably not correct to return either false (like I did) or true (like I wanted to change when I first pointed out this comment) if we can't get the layout of the pointee here.

CI failure(edited link) shows that false is bad because we're optimizing away perfectly fine polymorphic functions in the stdlib, and true is also probably not great, since we're dropping this assertion which may be false later once things are monomorphized. Added a mir-opt test to show the latter behavior in the first commit in this PR.

^^^ edit: I'm not actually sure that CI failure is due to this? It's confusing why that's happening, though. We should always be going from ICE -> dropping assertions

Second PR in this stack makes the queries you added fallible, so we can handle the LayoutErrors gracefully and not perform the optimization in that case. It duplicates a bit of work, since we may have to call the layout query twice on some code paths, but oh well.

and true is also probably not great, since we're dropping this assertion

If this function is not allowed to have false positives maybe it shouldn't be called might_

The function is intended to definitely answer whether we will emit a panic or not. Optimizations shouldn't change that behavior.

We don't always emit panics fully aggressively (that depends on compiler flags), so in a sense there are false negatives allowed.

rustbot · 2023-02-14T01:03:46Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler/rustc_middle/src/query/mod.rs

compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs

saethlin · 2023-02-14T22:53:07Z

Just been musing about this... I'm pretty sure that all of the MIR optimization value derived here is from deleting the inhabited assertion. Initially I was concerned that the ICE here pointed at a deeper issue in the compiler.

But if the solution that preserves the optimizations of the zero init and mem_uninit init paths costs complexity elsewhere in the compiler, or especially if it makes compilation slower due to the additional queries, we should probably just delete the InstCombine for those intrinsics, and only keep the one that checks for inhabited types.

compiler-errors · 2023-02-14T22:54:56Z

But if the solution that preserves the optimizations of the zero init and mem_uninit init paths costs complexity elsewhere in the compiler, or especially if it makes compilation slower due to the additional queries

I don't think that this implementation is significantly more complex, and I guess we can see if compilation is slower with

@bors try @rust-timer queue

bors · 2023-02-14T22:55:06Z

⌛ Trying commit b096f0e with merge ff2eaadf96941a226289d1d89cb63f9e7d7ef475...

bors · 2023-02-15T01:13:01Z

☀️ Try build successful - checks-actions
Build commit: ff2eaadf96941a226289d1d89cb63f9e7d7ef475 (ff2eaadf96941a226289d1d89cb63f9e7d7ef475)

rust-timer · 2023-02-15T03:03:39Z

Finished benchmarking commit (ff2eaadf96941a226289d1d89cb63f9e7d7ef475): comparison URL.

Overall result: no relevant changes - no 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.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

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)	-	-	0
Improvements ✅ (primary)	-3.5%	[-3.5%, -3.5%]	1
Improvements ✅ (secondary)	-2.1%	[-2.9%, -0.9%]	15
All ❌✅ (primary)	-3.5%	[-3.5%, -3.5%]	1

Cycles

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

oli-obk · 2023-02-15T08:39:28Z

@bors r+

bors · 2023-02-15T08:39:30Z

📌 Commit b096f0e has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-02-15T15:53:21Z

⌛ Testing commit b096f0e with merge f6cc158db71213b9eb346875598970a4205d964c...

bors · 2023-02-15T15:56:48Z

💔 Test failed - checks-actions

compiler-errors · 2023-02-15T16:47:47Z

Spurious it seems

@bors retry

bors · 2023-02-15T20:56:10Z

⌛ Testing commit b096f0e with merge c528357...

bors · 2023-02-16T00:12:48Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing c528357 to master...

bors · 2023-02-16T00:12:48Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing c528357 to master...

rust-log-analyzer · 2023-02-16T02:03:29Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

curl: (22) The requested URL returned error: 404

error: failed to download llvm from ci

    help: old builds get deleted after a certain time
    help: if trying to compile an old commit of rustc, disable `download-ci-llvm` in config.toml:

    [llvm]
    download-ci-llvm = false
Build completed unsuccessfully in 0:00:00

rust-timer · 2023-02-16T02:16:48Z

Finished benchmarking commit (c528357): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)	4.8%	[3.9%, 5.6%]	2
Improvements ✅ (primary)	-3.3%	[-3.4%, -3.2%]	2
Improvements ✅ (secondary)	-2.4%	[-4.8%, -0.6%]	50
All ❌✅ (primary)	-3.3%	[-3.4%, -3.2%]	2

Cycles

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

Don't ICE in `might_permit_raw_init` if reference is polymorphic Emitting optimized MIR for a polymorphic function may require computing layout of a type that isn't (yet) known. This happens in the instcombine pass, for example. Let's fail gracefully in that condition. cc `@saethlin` fixes rust-lang#107999

Upstream PRs that require local changes: - Don't ICE in might_permit_raw_init if reference is polymorphic rust-lang/rust#108012 - Use target instead of machine for mir interpreter integer handling rust-lang/rust#108047 - Optimize mk_region rust-lang/rust#108020 Co-authored-by: Qinheping Hu <qinhh@amazon.com>

rustbot assigned oli-obk Feb 13, 2023

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 Feb 13, 2023

compiler-errors commented Feb 13, 2023

View reviewed changes

tests/ui/lint/invalid_value-polymorphic.rs Outdated Show resolved Hide resolved

compiler-errors force-pushed the issue-107999 branch 2 times, most recently from 6c854dc to 132ee4d Compare February 13, 2023 22:31

saethlin reviewed Feb 13, 2023

View reviewed changes

tests/ui/lint/invalid_value-polymorphic.rs Outdated Show resolved Hide resolved

saethlin reviewed Feb 13, 2023

View reviewed changes

tests/ui/lint/invalid_value-polymorphic.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

compiler-errors force-pushed the issue-107999 branch 2 times, most recently from 13c9802 to c7b3261 Compare February 13, 2023 23:23

This comment has been minimized.

Sign in to view

compiler-errors commented Feb 14, 2023

View reviewed changes

Don't ICE in might_permit_raw_init if reference is polymorphic

087a013

compiler-errors force-pushed the issue-107999 branch from c7b3261 to da74b09 Compare February 14, 2023 01:03

oli-obk reviewed Feb 14, 2023

View reviewed changes

compiler/rustc_middle/src/query/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed Feb 14, 2023

View reviewed changes

compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs Show resolved Hide resolved

Make permit_uninit/zero_init fallible

b096f0e

compiler-errors force-pushed the issue-107999 branch from da74b09 to b096f0e Compare February 14, 2023 22:39

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 14, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 15, 2023

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 Feb 15, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 15, 2023

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 Feb 15, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 16, 2023

bors merged commit c528357 into rust-lang:master Feb 16, 2023

rustbot added this to the 1.69.0 milestone Feb 16, 2023

tautschnig mentioned this pull request Apr 16, 2023

Upgrade Rust toolchain to nightly-2023-02-17 model-checking/kani#2380

Merged

4 tasks

compiler-errors deleted the issue-107999 branch August 11, 2023 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't ICE in `might_permit_raw_init` if reference is polymorphic #108012

Don't ICE in `might_permit_raw_init` if reference is polymorphic #108012

compiler-errors commented Feb 13, 2023 •

edited

Loading

rustbot commented Feb 13, 2023

This comment has been minimized.

This comment has been minimized.

compiler-errors Feb 14, 2023 •

edited

Loading

saethlin Feb 14, 2023

compiler-errors Feb 14, 2023 •

edited

Loading

saethlin Feb 14, 2023

RalfJung Feb 14, 2023

rustbot commented Feb 14, 2023

saethlin commented Feb 14, 2023 •

edited

Loading

compiler-errors commented Feb 14, 2023

This comment has been minimized.

bors commented Feb 14, 2023

bors commented Feb 15, 2023

This comment has been minimized.

rust-timer commented Feb 15, 2023

oli-obk commented Feb 15, 2023

bors commented Feb 15, 2023

bors commented Feb 15, 2023

bors commented Feb 15, 2023

compiler-errors commented Feb 15, 2023

bors commented Feb 15, 2023

bors commented Feb 16, 2023

bors commented Feb 16, 2023

rust-log-analyzer commented Feb 16, 2023

rust-timer commented Feb 16, 2023

Don't ICE in might_permit_raw_init if reference is polymorphic #108012

Don't ICE in might_permit_raw_init if reference is polymorphic #108012

Conversation

compiler-errors commented Feb 13, 2023 • edited Loading

rustbot commented Feb 13, 2023

This comment has been minimized.

This comment has been minimized.

compiler-errors Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Feb 14, 2023

saethlin commented Feb 14, 2023 • edited Loading

compiler-errors commented Feb 14, 2023

This comment has been minimized.

bors commented Feb 14, 2023

bors commented Feb 15, 2023

This comment has been minimized.

rust-timer commented Feb 15, 2023

Overall result: no relevant changes - no action needed

oli-obk commented Feb 15, 2023

bors commented Feb 15, 2023

bors commented Feb 15, 2023

bors commented Feb 15, 2023

compiler-errors commented Feb 15, 2023

bors commented Feb 15, 2023

bors commented Feb 16, 2023

bors commented Feb 16, 2023

rust-log-analyzer commented Feb 16, 2023

rust-timer commented Feb 16, 2023

Overall result: no relevant changes - no action needed

Don't ICE in `might_permit_raw_init` if reference is polymorphic #108012

Don't ICE in `might_permit_raw_init` if reference is polymorphic #108012

compiler-errors commented Feb 13, 2023 •

edited

Loading

compiler-errors Feb 14, 2023 •

edited

Loading

compiler-errors Feb 14, 2023 •

edited

Loading

saethlin commented Feb 14, 2023 •

edited

Loading