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

Handle normalization failure in struct_tail_erasing_lifetimes #124548

Closed
wants to merge 1 commit into from

Conversation

gurry
Copy link
Contributor

@gurry gurry commented Apr 30, 2024

Fixes #113272

The ICE occurred because the struct being normalized had an error. This PR adds some defensive code to guard against that.

@rustbot
Copy link
Collaborator

rustbot commented Apr 30, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
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 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 30, 2024
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Please provide a detailed explanation for why this actually fails. Specifically, I don't think you have actually explained where this call to struct_tail_with_normalize is being located -- knowing where that happens is very important for making sure that this can't be approached some other way.

compiler/rustc_middle/src/ty/util.rs Outdated Show resolved Hide resolved
@gurry
Copy link
Contributor Author

gurry commented May 1, 2024

Please provide a detailed explanation for why this actually fails. Specifically, I don't think you have actually explained where this call to struct_tail_with_normalize is being located -- knowing where that happens is very important for making sure that this can't be approached some other way.

This is the failing code:

trait Trait {
    type RefTarget;
}

impl Trait for () where Missing: Trait {}

struct Other {
    data: <() as Trait>::RefTarget,
}

fn main() {
    unsafe {
        std::mem::transmute::<Option<()>, Option<&Other>>(None);
    }
}

The ICE'ing code path begins in the typeck of main and eventually hits the call to normalize_erasing_regions as shown in the back trace below:

  22:     0x7f645488cc4d - rustc_middle[9f74ec5cb00a905a]::util::bug::bug_fmt
  23:     0x7f64549cf295 - <rustc_middle[9f74ec5cb00a905a]::ty::normalize_erasing_regions::NormalizeAfterErasingRegionsFolder>::normalize_generic_arg_after_erasing_regions
  24:     0x7f64549cef43 - <rustc_middle[9f74ec5cb00a905a]::ty::Ty as rustc_type_ir[2174c24ce5957ab4]::fold::TypeFoldable<rustc_middle[9f74ec5cb00a905a]::ty::context::TyCtxt>>::fold_with::<rustc_middle[9f74ec5cb00a905a]::ty::normalize_erasing_regions::NormalizeAfterErasingRegionsFolder>
  25:     0x7f6454c26fec - <rustc_middle[9f74ec5cb00a905a]::ty::context::TyCtxt>::struct_tail_erasing_lifetimes
  26:     0x7f64554e6d15 - <rustc_middle[9f74ec5cb00a905a]::ty::layout::SizeSkeleton>::compute
  27:     0x7f645690e0c1 - <rustc_middle[9f74ec5cb00a905a]::ty::layout::SizeSkeleton>::compute::{closure#1}
  28:     0x7f64554e6fd7 - <rustc_middle[9f74ec5cb00a905a]::ty::layout::SizeSkeleton>::compute
  29:     0x7f64559e88f2 - <rustc_hir_typeck[510371791de31c85]::fn_ctxt::FnCtxt>::check_transmute
  30:     0x7f6454d17195 - rustc_hir_typeck[510371791de31c85]::typeck

This happens when we are trying to compute the SizeSkeleton for Option<&Other>.

The underlying reason for the normalization failure of Option<&Other> is that Other is carrying an error because its data field is projecting RefTarget on () which is actually not implemented anywhere.

@compiler-errors
Copy link
Member

I am not at my computer, but looking at the backtracr, I think the fix can just be localized to SizeSkeleton then. I don't think we need to touch struct_tail_with_normalize.

Probably just by manually calling struct_tail_with_normalize with a callback that doesn't ICE.

@gurry
Copy link
Contributor Author

gurry commented May 1, 2024

Yeah, that's an option. But doing it in struct_tail_erasing_lifetimes is going to help us in the future because any type that has an error, even from locations other than SizeSkeleton::compute, is likely to ICE in struct_tail_erasing_lifetimes anyway.

@compiler-errors
Copy link
Member

That's the problem, though. I consider it a bug if we are ever calling struct_tail_erasing_regions when we have struct definitions with errors. My understanding of that function is that it's only really correct to be used during codegen (or codegen-like operations) -- anywhere else seems suspicious, and you haven't provided any evidence that other callsites deserve this treatment.

If there are enough cases that we want a separate, fallible version of that function, we should be adding that separately. For example, look at the difference between normalize_erasing_regions and try_normalize_erasing_regions.

@gurry gurry force-pushed the 113272-ice-failed-to-normalize branch from e82e830 to 673ae99 Compare May 1, 2024 02:17
@gurry
Copy link
Contributor Author

gurry commented May 1, 2024

That's the problem, though. I consider it a bug if we are ever calling struct_tail_erasing_regions when we have struct definitions with errors. My understanding of that function is that it's only really correct to be used during codegen (or codegen-like operations) -- anywhere else seems suspicious, and you haven't provided any evidence that other callsites deserve this treatment.

If there are enough cases that we want a separate, fallible version of that function, we should be adding that separately. For example, look at the difference between normalize_erasing_regions and try_normalize_erasing_regions.

In the past I've been tempted to do just that i.e. create a fallible version of it. Maybe I'll do that now because the need for such a function is likely to grow thanks to #120847. That PR makes it much more likely that types carrying errors will reach typeck and later phases instead of being stopped at wfcheck.

@gurry
Copy link
Contributor Author

gurry commented May 1, 2024

Are you okay with me creating a fallible version of struct_tail_erasing_regions? I'll wait for your answer before I do it.

@compiler-errors
Copy link
Member

No, I have just said that there's no evidence that another callsite needs to be treated like this. Please do it in SizeSkeleton by manually calling struct_tail_with_normalize for now.

@rust-log-analyzer

This comment has been minimized.

@gurry
Copy link
Contributor Author

gurry commented May 1, 2024

No, I have just said that there's no evidence that another callsite needs to be treated like this.

I thought when you said that you were discouraging making normalize_erasing_regions itself fallible not creating a separate version of it which is fallible. I was suggesting the latter.

@gurry gurry force-pushed the 113272-ice-failed-to-normalize branch from 673ae99 to 0c71c9d Compare May 1, 2024 04:00
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

r=me after changing this to a delay span bug

compiler/rustc_middle/src/ty/layout.rs Outdated Show resolved Hide resolved
@compiler-errors
Copy link
Member

@rustbot author

@rustbot 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 May 7, 2024
Fixes an ICE that occurred when the struct in question has an error
@gurry gurry force-pushed the 113272-ice-failed-to-normalize branch from 0c71c9d to 72acb12 Compare May 8, 2024 12:53
@gurry
Copy link
Contributor Author

gurry commented May 8, 2024

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot 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 May 8, 2024
@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 8, 2024

📌 Commit 0c71c9d 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 May 8, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 8, 2024
…ze, r=compiler-errors

Handle normalization failure in `struct_tail_erasing_lifetimes`

Fixes rust-lang#113272

The ICE occurred because the struct being normalized had an error. This PR adds some defensive code to guard against that.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 8, 2024
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#124548 (Handle normalization failure in `struct_tail_erasing_lifetimes`)
 - rust-lang#124761 (Fix insufficient logic when searching for the underlying allocation)
 - rust-lang#124864 (rustdoc: use stability, instead of features, to decide what to show)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 8, 2024
Rollup merge of rust-lang#124548 - gurry:113272-ice-failed-to-normalize, r=compiler-errors

Handle normalization failure in `struct_tail_erasing_lifetimes`

Fixes rust-lang#113272

The ICE occurred because the struct being normalized had an error. This PR adds some defensive code to guard against that.
@ehuss
Copy link
Contributor

ehuss commented May 8, 2024

This appears to have been merged in #124890, but GitHub did not auto-close it. Maybe there was something related to the merge conflict? Anyway, I recommend closing this after confirming it is merged correctly.

@compiler-errors
Copy link
Member

oh no :( bors merged an old version!

@compiler-errors
Copy link
Member

@gurry can you reopen this PR applying the changes i last asked for?

@bors
Copy link
Contributor

bors commented May 8, 2024

☔ The latest upstream changes (presumably #124890) made this pull request unmergeable. Please resolve the merge conflicts.

@bors 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 May 8, 2024
@compiler-errors
Copy link
Member

Actually, I did it here: #124909

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 8, 2024
…ers, r=compiler-errors

Reapply the part of rust-lang#124548 that bors forgot

rust-lang#124548 (comment)
r? compiler-errors
bors added a commit to rust-lang-ci/rust that referenced this pull request May 8, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#123344 (Remove braces when fixing a nested use tree into a single item)
 - rust-lang#124587 (Generic `NonZero` post-stabilization changes.)
 - rust-lang#124775 (crashes: add lastest batch of crash tests)
 - rust-lang#124869 (Make sure we don't deny macro vars w keyword names)
 - rust-lang#124876 (Simplify `use crate::rustc_foo::bar` occurrences.)
 - rust-lang#124892 (Update cc crate to v1.0.97)
 - rust-lang#124903 (Ignore empty RUSTC_WRAPPER in bootstrap)
 - rust-lang#124909 (Reapply the part of rust-lang#124548 that bors forgot)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 8, 2024
Rollup merge of rust-lang#124909 - compiler-errors:struct-tail-leftovers, r=compiler-errors

Reapply the part of rust-lang#124548 that bors forgot

rust-lang#124548 (comment)
r? compiler-errors
@gurry
Copy link
Contributor Author

gurry commented May 9, 2024

Actually, I did it here: #124909

Thanks @compiler-errors

@gurry gurry deleted the 113272-ice-failed-to-normalize branch May 9, 2024 00:44
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request May 11, 2024
…mpiler-errors

Reapply the part of #124548 that bors forgot

rust-lang/rust#124548 (comment)
r? compiler-errors
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request May 24, 2024
…mpiler-errors

Reapply the part of #124548 that bors forgot

rust-lang/rust#124548 (comment)
r? compiler-errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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: Failed to normalize Alias(Projection, AliasTy ..
7 participants