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

Fix transmute intrinsic mir validation ICE #109959

Merged
merged 1 commit into from
Apr 12, 2023

Conversation

JakobDegen
Copy link
Contributor

@JakobDegen JakobDegen commented Apr 5, 2023

I stumbled across this at work, the minimal reproducer is included as a test which ICEs before this change.

I'm not 100% sure this is the right fix, but it matches what we do in mir_assign_valid_types so seems reasonable at least.

fixes #110151

r? @lcnr since they've been keeping the relevant logic correct, cc @scottmcm

@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 Apr 5, 2023
Copy link
Member

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this! Looks entirely reasonable to me, but I don't think I have enough context in exactly why it's needed to sign off.

@JakobDegen JakobDegen force-pushed the transmute-validate branch from aa6aaac to dd92a6a Compare April 5, 2023 06:50
@compiler-errors
Copy link
Member

compiler-errors commented Apr 9, 2023

I don't think we should just skip this check for opaques. We should probably instead normalize to reveal the opaque types, since we're always in a Reveal::All param env:

diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs
index 0f56fda18f5..d4bed97380b 100644
--- a/compiler/rustc_const_eval/src/transform/validate.rs
+++ b/compiler/rustc_const_eval/src/transform/validate.rs
@@ -679,13 +679,21 @@ macro_rules! check_kinds {
                             // Unlike `mem::transmute`, a MIR `Transmute` is well-formed
                             // for any two `Sized` types, just potentially UB to run.
 
-                            if !op_ty.is_sized(self.tcx, self.param_env) {
+                            if !self
+                                .tcx
+                                .normalize_erasing_regions(self.param_env, op_ty)
+                                .is_sized(self.tcx, self.param_env)
+                            {
                                 self.fail(
                                     location,
                                     format!("Cannot transmute from non-`Sized` type {op_ty:?}"),
                                 );
                             }
-                            if !target_type.is_sized(self.tcx, self.param_env) {
+                            if !self
+                                .tcx
+                                .normalize_erasing_regions(self.param_env, *target_type)
+                                .is_sized(self.tcx, self.param_env)
+                            {
                                 self.fail(
                                     location,
                                     format!("Cannot transmute to non-`Sized` type {target_type:?}"),

@compiler-errors
Copy link
Member

compiler-errors commented Apr 9, 2023

For the more type system savvy, checking some "trivial" opaque self type obligation impl A: A holds in a Reveal::All param-env will in general fail because when trying to confirm an alias bound, we normalize here:

project::normalize_with_depth(

... which means that we then try to equate the obligation impl A: A and that alias bound that now has the impl A self type revealed to its hidden type here:

.sup(DefineOpaqueTypes::No, ty::Binder::dummy(placeholder_trait_ref), trait_bound)

... which will fail because DefineOpaques::No.

That's why normalizing here is the right thing to do -- we can only verify fully revealed predicates in fully revealed environments.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-14 failed! Check out the build log: (web) (plain)

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

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 10, 2023

📌 Commit d8ed2fb has been approved by compiler-errors

It is now in the queue for this repository.

@bors 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 Apr 10, 2023
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Apr 11, 2023
…ompiler-errors

Fix transmute intrinsic mir validation ICE

I stumbled across this at work, the minimal reproducer is included as a test which ICEs before this change.

I'm not 100% sure this is the right fix, but it matches what we do in `mir_assign_valid_types` so seems reasonable at least.

fixes rust-lang#110151

r? `@lcnr` since they've been keeping the relevant logic correct, cc `@scottmcm`
@bors
Copy link
Contributor

bors commented Apr 11, 2023

⌛ Testing commit d8ed2fb with merge 8c9883e63fd369fd83fd5be467dc14f8476bc2cb...

@bors
Copy link
Contributor

bors commented Apr 11, 2023

💔 Test failed - checks-actions

@bors 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 Apr 11, 2023
@rust-log-analyzer
Copy link
Collaborator

The job dist-x86_64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling rustc_mir_transform v0.0.0 (/Users/runner/work/rust/rust/compiler/rustc_mir_transform)
[RUSTC-TIMING] rustc_infer test:false 90.890
   Compiling rustc_hir_typeck v0.1.0 (/Users/runner/work/rust/rust/compiler/rustc_hir_typeck)
[RUSTC-TIMING] rustc_hir_typeck test:false 4.744
rustc exited with signal: 11 (SIGSEGV)

Caused by:
Caused by:
  process didn't exit successfully: `/Users/runner/work/rust/rust/build/bootstrap/debug/rustc --crate-name rustc_hir_typeck --edition=2021 compiler/rustc_hir_typeck/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -C embed-bitcode=no -C debuginfo=0 -Zunstable-options --check-cfg 'values(feature)' --check-cfg 'names()' --check-cfg 'values()' -C metadata=0a1dabe24f72bf47 -C extra-filename=-0a1dabe24f72bf47 --out-dir /Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps --target x86_64-apple-darwin -L dependency=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps -L dependency=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/release/deps --extern rustc_ast=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_ast-13c8237bfaf2b218.rmeta --extern rustc_data_structures=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_data_structures-acf5ec6690477c7c.rmeta --extern rustc_errors=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_errors-a6a0c8001d7e3dfc.rmeta --extern rustc_graphviz=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_graphviz-03263d1cc2f6d564.rmeta --extern rustc_hir=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_hir-6c8e97074a8194d5.rmeta --extern rustc_hir_analysis=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_hir_analysis-78c41dbb444d7625.rmeta --extern rustc_hir_pretty=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_hir_pretty-a91b12e65115263d.rmeta --extern rustc_index=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_index-8de82df4fd85f8dd.rmeta --extern rustc_infer=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_infer-fdb3ce2b6a9846cb.rmeta --extern rustc_lint=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_lint-466b544a14deddb6.rmeta --extern rustc_macros=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/release/deps/librustc_macros-4dacd9f7a99621cc.dylib --extern rustc_middle=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_middle-5283663fd856675c.rmeta --extern rustc_serialize=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_serialize-46ea928a052f761a.rmeta --extern rustc_session=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_session-4037a17a0b44f638.rmeta --extern rustc_span=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_span-9f1f4ec519ad9936.rmeta --extern rustc_target=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_target-78145233440e267b.rmeta --extern rustc_trait_selection=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_trait_selection-d3b8a2698ec3503d.rmeta --extern rustc_type_ir=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/librustc_type_ir-f013cacf6bbfbd7d.rmeta --extern smallvec=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/libsmallvec-f7f8e36a33ee12ee.rmeta --extern tracing=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/deps/libtracing-7672f4225ba667b7.rmeta --cfg=windows_raw_dylib -Csymbol-mangling-version=v0 -Zunstable-options '--check-cfg=values(bootstrap)' '--check-cfg=values(parallel_compiler)' '--check-cfg=values(no_btreemap_remove_entry)' '--check-cfg=values(crossbeam_loom)' '--check-cfg=values(span_locations)' '--check-cfg=values(rustix_use_libc)' '--check-cfg=values(emulate_second_only_system)' '--check-cfg=values(windows_raw_dylib)' -Zmacro-backtrace -Zosx-rpath-install-name '-Clink-args=-Wl,-rpath,@loader_path/../lib' -Csplit-debuginfo=unpacked -Zunstable-options '-Wrustc::internal' -Cprefer-dynamic -Zdylib-lto -Clto=thin -Cembed-bitcode=yes -Z binary-dep-depinfo -L native=/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-rustc/x86_64-apple-darwin/release/build/psm-4e3e6dd64c8eb7fe/out` (exit status: 254)
[RUSTC-TIMING] rustc_traits test:false 51.156
[RUSTC-TIMING] rustc_trait_selection test:false 99.000
[RUSTC-TIMING] rustc_middle test:false 152.128
[RUSTC-TIMING] rustc_const_eval test:false 69.397

@JakobDegen
Copy link
Contributor Author

Segfault so... maybe spurious?

@lcnr
Copy link
Contributor

lcnr commented Apr 12, 2023

r? @compiler-errors

@bors retry

@bors 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 Apr 12, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 12, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#109959 (Fix transmute intrinsic mir validation ICE)
 - rust-lang#110176 (Renumbering cleanups)
 - rust-lang#110182 (Use `itertools::Either` instead of own impl)
 - rust-lang#110188 (Remove orphaned remove_dir_all implementation from rust-installer)
 - rust-lang#110190 (Custom MIR: Support `BinOp::Offset`)
 - rust-lang#110209 (Add regression test for rust-lang#59003)
 - rust-lang#110210 (`DescriptionCtx` cleanups)
 - rust-lang#110217 (doc: loongarch: Fix typos)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fd40f51 into rust-lang:master Apr 12, 2023
@rustbot rustbot added this to the 1.70.0 milestone Apr 12, 2023
@JakobDegen JakobDegen deleted the transmute-validate branch April 30, 2023 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

ICE: Cannot transmute to non-Sized type
7 participants