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

Temporarily disable building rustc with ThinLTO on x86_64-unknown-linux-gnu and x86_64-pc-windows-msvc #105662

Closed
wants to merge 2 commits into from

Conversation

lqd
Copy link
Member

@lqd lqd commented Dec 13, 2022

Context: we recently started building librustc_driver.so with ThinLTO on:

Unfortunately, it appears to regress the message we print during ICEs sometimes, including the query stack and explanation on how to open a GH issue. This was noticed in #105637 on mac (where I've enabled ThinLTO recently, and why I was pinged because of the regression and started looking into this), but is present on all the above targets today. There's also a summary and examples in that issue here and some more discussion in this zulip prioritization topic.

Since this is useful information for reporting issues, triage and bisections, it impacts the quality of bug reports. The issue was deemed P-critical, so this PR works around it by temporarily disabling ThinLTO on linux/windows (this setting is already being temporarily disabled for mac as well in #105646) until we can fully investigate and fix the underlying issue causing this early abort (e.g. some UB somewhere causing problems now because of the newly enabled optimizations).

Since this issue impacts the quality of bug reports, it could be more important than the perf ThinLTO provides. I don't have the time to fully investigate the cause right now, but maybe someone else does. (I believe we'd rather workaround the issue than live with it ?)

To that end, I'll cc:

  • t-compiler leads @pnkfelix and @wesleywiser on: whether this workaround is the approach we want to take, and if the issue/PR should be nominated for discussion at the next t-compiler meeting, if anyone from the team would be available/willing to investigate, etc
  • the prioritization working group @rust-lang/wg-prioritization
  • the authors of this original work, @bjorn3 and @Kobzol

(Note: #101403 will land in 1.66 on 12/15 in 2 days, but rebuilding stable artifacts this late was deemed not worth it in the zulip topic linked above)

(Opening as draft to not merge early)

@rustbot
Copy link
Collaborator

rustbot commented Dec 13, 2022

r? @jyn514

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

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Dec 13, 2022
@lqd
Copy link
Member Author

lqd commented Dec 13, 2022

(apologies to joshua for the unexpected ping, I forgot to r? @ghost)

@bors try

@rustbot

This comment was marked as resolved.

@jyn514 jyn514 assigned lqd and unassigned jyn514 Dec 13, 2022
@lqd
Copy link
Member Author

lqd commented Dec 13, 2022

@bors ping

@bors
Copy link
Contributor

bors commented Dec 13, 2022

😪 I'm awake I'm awake

@lqd
Copy link
Member Author

lqd commented Dec 13, 2022

@bors try

@lqd lqd closed this Dec 13, 2022
@lqd lqd reopened this Dec 13, 2022
@lqd
Copy link
Member Author

lqd commented Dec 13, 2022

@bors try

@Kobzol
Copy link
Contributor

Kobzol commented Dec 13, 2022

Seems like bors doesn't want to part with LTO perf. gains :)

@lqd
Copy link
Member Author

lqd commented Dec 13, 2022

@bors try

@apiraino apiraino added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Dec 14, 2022
@bors
Copy link
Contributor

bors commented Dec 14, 2022

⌛ Trying commit 548cc35 with merge 935dcc042c9966d1b4d4ee6c6cd3f528c1fe3ed9...

@bors
Copy link
Contributor

bors commented Dec 14, 2022

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

@lqd
Copy link
Member Author

lqd commented Dec 17, 2022

I believe #105800 fixed the underlying issue, so this workaround shouldn't be needed anymore.

@lqd lqd closed this Dec 17, 2022
@lqd lqd deleted the disable-thinlto branch December 17, 2022 21:53
@apiraino apiraino removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants