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.

Sign up for GitHub

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

Jump to bottom

Don't ICE on layout computation failure #111580

Merged
merged 1 commit into from
Aug 29, 2023
Merged

Don't ICE on layout computation failure #111580

merged 1 commit into from
Aug 29, 2023

Conversation

atsuzaki
Copy link
Contributor

Fixes #111176 regression.

r? @oli-obk

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

rustbot commented May 15, 2023

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@atsuzaki
Copy link
Contributor Author

atsuzaki commented May 15, 2023 •
edited
Loading

The translateable diagnostics stuff was required by rustc_codegen_llvm so I just put the stuff there, but since LayoutError in multiple place I assume it should be somewhere more generally accessible? Let me know if this is fine or if I should move it (and where would work best).

Yup, seems like I need it too for codegen_gcc. Should I put the message somewhere more centralized? A separate message in gcc? Please advise, thanks!

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514
Copy link
Member

jyn514 commented May 15, 2023

I think rustc_codegen_ssa is shared between all codegen backends, that might be a good place to put the shared error.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
cjgillot
cjgillot reviewed May 15, 2023
View reviewed changes
compiler/rustc_codegen_llvm/src/context.rs Outdated
Comment on lines 972 to 1005
if let LayoutError::SizeOverflow(_) = err {
self.sess().emit_fatal(Spanned { span, node: err })
} else {
span_bug!(span, "failed to get layout for `{}`: {}", ty, err)
self.tcx.sess.emit_fatal(ssa_errors::FailedToGetLayout { span, ty, err })
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we go without the branch in the 3 handle_layout_err methods, and always emit the same diagnostic struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps...

Or now I'm thinking, maybe the more correct thing to do here is:

        if let LayoutError::SizeOverflow(_) | LayoutError::Cycle = err {
            self.tcx.sess.emit_fatal(ssa_errors::FailedToGetLayout { span, ty, err })
        } else {
            span_bug!(span, "failed to get layout for `{}`: {}", ty, err)
        }

since @oli-obk only really made changes for LayoutError::Cycle, the rest is still probably a bug?

Copy link
Contributor

Choose a reason for hiding this comment

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

since @oli-obk only really made changes for LayoutError::Cycle, the rest is still probably a bug?

correct, but I don't think the value of separating that is very high. We could just always take the if arm in all the cases you changed. Worst case a bug will be an error.

@atsuzaki atsuzaki force-pushed the layout-ice branch 2 times, most recently from 03b3783 to ec276ea Compare May 17, 2023 04:38
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@atsuzaki
Copy link
Contributor Author

atsuzaki commented May 17, 2023 •
edited
Loading

How do I run/bless tests for other archs? Is there some remote machine I can use?

I made the change as suggested above and blessed tests, but tests/ui/limits/issue-69485-var-size-diffs-too-large.rs is // only-x86_64, meanwhile I'm on a M1 Mac...

oli-obk
oli-obk reviewed May 17, 2023
View reviewed changes
compiler/rustc_codegen_cranelift/src/common.rs Outdated
@@ -483,7 +483,7 @@ impl<'tcx> LayoutOfHelpers<'tcx> for RevealAllLayoutCx<'tcx> {
if let layout::LayoutError::SizeOverflow(_) = err {
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition can also go away in favor of just always emitting the error

@jyn514
Copy link
Member

jyn514 commented May 18, 2023

How do I run/bless tests for other archs? Is there some remote machine I can use?

There's not an easy way, unfortunately - I don't think the docker containers support emulating x86 on aarch64. Maybe ask in https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/Cloud.20compute.20.2F.20Dev.20desktop.20feedback if you can get permissions on the cloud-dev machines? https://forge.rust-lang.org/infra/docs/dev-desktop.html

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@atsuzaki
Copy link
Contributor Author

How do I run/bless tests for other archs? Is there some remote machine I can use?

There's not an easy way, unfortunately - I don't think the docker containers support emulating x86 on aarch64. Maybe ask in https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/Cloud.20compute.20.2F.20Dev.20desktop.20feedback if you can get permissions on the cloud-dev machines? https://forge.rust-lang.org/infra/docs/dev-desktop.html

Yeah I initially tried the docker route and didn't get too far, then realized I do have a spare 64bit x86 machine, dusted it out and just did it there. And now I am stuck running into stuff that runs only on 32bit, urgh. I might just manually edit the stderr based on CI output for now...

@oli-obk
Copy link
Contributor

oli-obk commented May 19, 2023

You could avoid the blessing issue by always taking the logic from the if arm, as that will keep all existing diagnostics the same and only require changing tests for the new diagnostic

@oli-obk
Copy link
Contributor

oli-obk commented May 24, 2023

@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 24, 2023
@bors
Copy link
Contributor

bors commented May 26, 2023

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

@chenyukang
Copy link
Member

@atsuzaki
seems the PR is almost finished.
could you please resolve the conflicts.
Other issues could be resolved by this PR:
#114324
#111580

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@atsuzaki
Copy link
Contributor Author

@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 Aug 24, 2023
@atsuzaki atsuzaki requested a review from oli-obk August 28, 2023 14:57
oli-obk
oli-obk reviewed Aug 28, 2023
View reviewed changes
compiler/rustc_codegen_cranelift/src/common.rs
Comment on lines 480 to 485
if let LayoutError::SizeOverflow(_) | LayoutError::ReferencesError(_) = err {
self.0.sess.span_fatal(span, err.to_string())
} else {
span_bug!(span, "failed to get layout for `{}`: {}", ty, err)
self.0.sess.span_fatal(span, format!("failed to get layout for `{}`: {}", ty, err))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

is the diagnostic unhelpful if we just remove the if condition and always do self.0.sess.span_fatal(span, err.to_string())?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was what I did initially, but since it changed a bunch of output I got blocked by having to bless tests on platforms I can't run. Perhaps having it tackled in a separate PR by someone who's better setup to do the blessing might be a good idea?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 28, 2023

The PR lgtm now, please squash all commits

@atsuzaki
Copy link
Contributor Author

Squashed, thanks! @oli-obk

@oli-obk
Copy link
Contributor

oli-obk commented Aug 29, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 29, 2023

📌 Commit 56b7673 has been approved by oli-obk

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 Aug 29, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 29, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 29, 2023
@bors
Auto merge of rust-lang#115354 - matthiaskrgr:rollup-4cotcxz, r=matth…
83995f3 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#111580 (Don't ICE on layout computation failure)
 - rust-lang#114923 (doc: update lld-flavor ref)
 - rust-lang#115174 (tests: add test for rust-lang#67992)
 - rust-lang#115187 (Add new interface to smir)
 - rust-lang#115300 (Tweaks and improvements on SMIR around generics_of and predicates_of)
 - rust-lang#115340 (some more is_zst that should be is_1zst)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 56d7d93 into rust-lang:master Aug 29, 2023
@rustbot rustbot added this to the 1.74.0 milestone Aug 29, 2023
@Luk-ESC Luk-ESC mentioned this pull request Aug 30, 2023
Closed
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Sep 6, 2023
@matthiaskrgr
Rollup merge of rust-lang#111580 - atsuzaki:layout-ice, r=oli-obk
65d75fd 
Don't ICE on layout computation failure

Fixes rust-lang#111176 regression.

r? `@oli-obk`
@compiler-errors compiler-errors mentioned this pull request Sep 22, 2023
Closed
antoyo pushed a commit to antoyo/rust that referenced this pull request Oct 9, 2023
@matthiaskrgr
Rollup merge of rust-lang#111580 - atsuzaki:layout-ice, r=oli-obk
13e402e 
Don't ICE on layout computation failure

Fixes rust-lang#111176 regression.

r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@cjgillot cjgillot cjgillot left review comments

@chenyukang chenyukang chenyukang approved these changes

Assignees

@oli-obk oli-obk

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
Milestone
1.74.0
Development

Successfully merging this pull request may close these issues.

ICE: failed to get layout
8 participants
@atsuzaki @rustbot @rust-log-analyzer @jyn514 @oli-obk @bors @chenyukang @cjgillot