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

Missing ICE info after some compiler panics #105637

Closed
jruderman opened this issue Dec 12, 2022 · 11 comments
Closed

Missing ICE info after some compiler panics #105637

jruderman opened this issue Dec 12, 2022 · 11 comments
Labels
C-bug Category: This is a bug. P-critical Critical priority

Comments

@jruderman
Copy link
Contributor

Several panic messages changed recently:

Missing output example

As an example, here's the post-stack-trace output when testing #105632 with an older build:

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.68.0-nightly (657eefe2d 2022-12-11) running on x86_64-apple-darwin

query stack during panic:
#0 [typeck] type-checking `bar`
#1 [typeck_item_bodies] type-checking all item bodies
#2 [analysis] running analysis passes on this crate
end of query stack

Regression

Seems to be a regression from bdb07a8 (#103647) (@lqd)

Version

rustc 1.68.0-nightly (bdb07a8ec 2022-12-11)
binary: rustc
commit-hash: bdb07a8ec8e77aa10fb84fae1d4ff71c21180bb4
commit-date: 2022-12-11
host: x86_64-apple-darwin
release: 1.68.0-nightly
LLVM version: 15.0.6
@jruderman jruderman added the C-bug Category: This is a bug. label Dec 12, 2022
@jruderman
Copy link
Contributor Author

Requesting prioritization because affects the quality of bug reports and may interfere with bisection.

@rustbot prioritize

@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 12, 2022
@compiler-errors
Copy link
Member

@jruderman do you test on an x86_64 mac? otherwise, I'm curious how you bisected this to #103647.

@jruderman
Copy link
Contributor Author

Yes, this is all on x86_64 mac.

@lqd
Copy link
Member

lqd commented Dec 13, 2022

I'm not sure why this would affect ICEs so I've posted the #105646 revert to allow time for some investigation.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Dec 13, 2022
Revert "enable ThinLTO for rustc on x86_64-apple-darwin dist builds"

Apparently ThinLTO on x64 mac can regress some of the ICEs' output. This reverts rust-lang#103647 to allow for investigation, and fixes  rust-lang#105637 in the meantime.
@lqd
Copy link
Member

lqd commented Dec 13, 2022

This issue isn't platform-specific, and happens on all nightlies built with ThinLTO:

Examples:

  1. On the most recent nightlies, where ThinLTO is enabled on the main targets above: ICE with mismatch in trait associated constant #105632 linked in the OP
#![feature(associated_const_equality)]
#![allow(dead_code)]

trait TraitWAssocConst {
        const A: usize;
}

fn foo<T: TraitWAssocConst<A = 1>>() {}

fn bar<T: TraitWAssocConst<A = 0>>() {
        foo::<T>();
}

fn main() {}

And we can see on the playground that the usual ICE post-trace output is missing.

Note that this ICE was not present on nightly-2022-10-24, and is most useful for bisecting on win/mac, not linux: the following example should be usable for that.

  1. An ICE whose existence crosses the Enable LTO for rustc_driver.so #101403 linux merge date, so you can see the before and after changes (this ICE happens in 1.66 and is fixed in 1.67): ICE: assertion failed: elem.index() < self.domain_size #103748
struct Apple((Apple, Option(Banana ? Citron)));

fn main() {}

We can see the ICE is missing the post trace output on the current beta (1.66.beta.2) on the playground, and the output changed in nightly-2022-10-24, it is present in nightly-2022-10-23.


Since this is useful output for reporting issues, debugging and bisection, should we do something about ThinLTO landing on stable on linux in 2 days (e.g. disabling rust.lto=thin on the x64 linux dist builder on beta) ? I'm not sure anyone will have the time to investigate and land a fix by then.

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Dec 13, 2022
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 13, 2022
Revert "enable ThinLTO for rustc on x86_64-apple-darwin dist builds"

Apparently ThinLTO on x64 mac can regress some of the ICEs' output. This reverts rust-lang#103647 to allow for investigation, and helps with rust-lang#105637 in the meantime.
@lqd
Copy link
Member

lqd commented Dec 17, 2022

#105800 should fix the -Zdylib-lto bug causing this issue, so we shouldn't need to revert the rust.lto = thin config on the dist builders.

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 17, 2022
Don't copy symbols from dylibs with `-Zdylib-lto`

When `rustc_driver` started being built with `-Zdylib-lto -Clto=thin`, some libstd symbols were copied by the LTO process into the dylib. That causes duplicate local symbols that are not present otherwise.

Depending on the situation (lib loading order apparently), the duplicated symbols could cause issues: `rustc_driver` overrode the panic hook, but it didn't apply to rustc main's hook (the default from libstd). This is the cause of rust-lang#105637, in some situations the panic hook installed by `rustc_driver` isn't executed, and only libstd's backtrace is shown (and a double panic). The query stack, as well as the various notes to open a GH about the ICE, don't appear.

It's not clear exactly what is needed to trigger the issue, but I have simulated a reproducer [here](https://github.com/lqd/issue-105637) with cargo involved, the incorrect panic hook is executed on my machine. It is hard to reproduce in a unit test: `cargo run` + `rustup` involves LD_LIBRARY_PATH, which is not the case for `compiletest`. cargo also adds unconditional flags that are then overridden in [`bootstrap` when building rustc with `rust.lto = thin`](https://github.com/rust-lang/rust/blob/9c07efe84f28a44f3044237696acc295aa407ee5/src/bootstrap/compile.rs#L702-L714) as done on CI).

All this to say the compilation and execution environment in `bootstrap` leading to the bug building `rustc_driver` is different from our UI tests, and I believe one of the reasons it's hard to make an exact reproducer test. Thankfully there's _still_ a difference in the behavior though: although in the unit test the correct panic hook seems to be executed compared to my repro and the current nightly, only the fix removes the double panic here.

The `7e8277aefa12f1469fb1df01418ff5846a7854a9` `try` build:
- fixes the reproducer repo linked above
- restores the ICE messages from rust-lang#105321 back to the state in its OP compared to the description in rust-lang#105637
- restores the ICE message and the query stack from rust-lang#105777 compared to nightly

While I believe this technically fixes the P-critical issue rust-lang#105637, I would not want to close it yet as we may want to backport to beta/stable (if a point release happens, it would fix the ICEs reported on 1.66.0, which is built with ThinLTO on linux). Once this PR lands, I'll also open another PR to re-enable ThinLTO on x64 darwin's dist builder.
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 19, 2022
…mulacrum

Re-enable ThinLTO for rustc on `x86_64-apple-darwin`

ThinLTO was disabled on x64 mac in rust-lang#105646 because of the rust-lang#105637 regression.

It was later discovered that the issue was present on other targets as well, as the mac revert was already landing. The linux/win reverts, however, did not land before the root cause was identified.

rust-lang#105800 fixed the underlying issue in `-Zdylib-lto` handling, and the x64 msvc and linux targets are now fixed, ICEs are using the correct `rustc_driver` panic hook.

This PR re-enables ThinLTO on mac for improved perf now that the issue should be fixed everywhere.
@Noratrieb
Copy link
Member

Can this be closed now?

@lqd
Copy link
Member

lqd commented Dec 31, 2022

Probably. The fix has landed on nightly and beta, but the PR is still stable-nominated. It's not really critical to do such a backport unless someone experiences issues on 1.66 or the triage WG sees too many bug reports they don't like.

@apiraino
Copy link
Contributor

As @lqd says - the stable backport was discussed but ultimately kept "on hold" to see how the patch behaves in the beta channel. A stable backport sometimes means a point release and that's up to T-infra to decide.

Ok if you prefer to close this issue, we can always re-open if a stable backport will be approved (that will be discussed probably next week)

@jyn514
Copy link
Member

jyn514 commented Apr 27, 2023

This is several months old at this point, I think it's safe to say there won't be a stable backport 😄

@jyn514 jyn514 closed this as completed Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-critical Critical priority
Projects
None yet
Development

No branches or pull requests

7 participants