Don't copy symbols from dylibs with -Zdylib-lto
#105800
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
@bors try |
|
⌛ Trying commit be5685b with merge 7e8277aefa12f1469fb1df01418ff5846a7854a9... |
|
☀️ Try build successful - checks-actions |
|
Since we discussed this fix together, r? @bjorn3 |
bjorn3
reviewed
Dec 17, 2022
|
@bors r+ p=1 fixes a P-critical issue |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (65c53c3): comparison URL. Overall result: ❌✅ regressions and improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. |
|
Since it fixes a P-critical issue and it's a trivial change, we could discuss backporting to beta/stable to have better quality bug reports for ICEs: @rustbot label: +beta-nominated +stable-nominated |
rustbot
added
beta-nominated
This was referenced Dec 17, 2022
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
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.
rustbot
added
the
beta-accepted
Mark-Simulacrum
removed
the
beta-nominated
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 27, 2022
…mulacrum [beta] backport rollup * Revert "Replace usage of ResumeTy in async lowering with Context" rust-lang#105915 * Don't copy symbols from dylibs with -Zdylib-lto rust-lang#105800 * rustdoc: Only hide lines starting with # in rust code blocks rust-lang#105539 * Mangle "main" as "__main_void" on wasm32-wasi rust-lang#105468 r? `@ghost`
rustbot
removed
the
stable-nominated
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When
rustc_driverstarted 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_driveroverrode the panic hook, but it didn't apply to rustc main's hook (the default from libstd). This is the cause of #105637, in some situations the panic hook installed byrustc_driverisn'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 with cargo involved, the incorrect panic hook is executed on my machine. It is hard to reproduce in a unit test:
cargo run+rustupinvolves LD_LIBRARY_PATH, which is not the case forcompiletest. cargo also adds unconditional flags that are then overridden inbootstrapwhen building rustc withrust.lto = thinas done on CI).All this to say the compilation and execution environment in
bootstrapleading to the bug buildingrustc_driveris 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
7e8277aefa12f1469fb1df01418ff5846a7854a9trybuild:unwraponNoneinfind_cycle_in_stack#105321 back to the state in its OP compared to the description in Missing ICE info after some compiler panics #105637While I believe this technically fixes the P-critical issue #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.