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

linking issue on x86_64-pc-windows-gnu target + thinLTO related to static function pointers #98302

Closed
Super-Pizza opened this issue Jun 20, 2022 · 12 comments
Labels
A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. O-windows-gnu Toolchain: GNU, Operating system: Windows P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Super-Pizza
Copy link

// lib.rs
#[inline]
pub fn aaa() {
    static mut F: fn() = || {};
    unsafe { F() };
}
// main.rs
fn main() {
    lto::aaa();
}

errors on link with

C:\Users\user\crate\target\release\deps\crate-85a417bb6cdac81e.crate.684d89f7-cgu.1.rcgu.o:crate.684d:(.text+0x3): undefined reference to `__imp__ZN5inner3aaa3AAA17h6072a64b61f520c1E'

and it only occurs:
• on x86_64-pc-windows-gnu
• With thin LTO
• With #[inline]
• With a static mut or UnsafeCell
• With a function pointer

Meta

rustc --version --verbose:

rustc 1.61.0 (fe5b13d68 2022-05-18)
binary: rustc
commit-hash: fe5b13d681f25ee6474be29d748c65adcd91f69e
commit-date: 2022-05-18
host: x86_64-pc-windows-gnu
release: 1.61.0
LLVM version: 14.0.0

Closest I could find: #71720

@Super-Pizza Super-Pizza added the C-bug Category: This is a bug. label Jun 20, 2022
@Super-Pizza
Copy link
Author

It's an issue I found in the wild: gl

@ChrisDenton ChrisDenton added A-linkage Area: linking into static, shared libraries and binaries O-windows-gnu Toolchain: GNU, Operating system: Windows labels Jun 20, 2022
@ChrisDenton
Copy link
Member

This specific example seems to succeed in 1.46 but fail in 1.47. I've not done a bisect though. The most significant thing in the release notes for 1.47 was the upgrade to LLVM 11 but this could be caused by something else.

@ChrisDenton ChrisDenton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 21, 2022
@Super-Pizza
Copy link
Author

Oh, and it doesn't seem to be a problem with lld?

@mati865
Copy link
Contributor

mati865 commented Sep 28, 2022

I believe it works with LLD because it can fixup missing dllimport symbols if it finds regular ones: https://github.com/llvm/llvm-project/blob/16e5d6d7f98f1119aab3d10ec4f9e59b5aacd359/lld/COFF/SymbolTable.cpp#L488
Which appears to be the case here:

$ nm target/release/deps/thinlto_test-f0129d30483da959.thinlto_test.63a4c5c1-cgu.0.rcgu.o | rg thinlto_test3aaa1F17h679dc1481bd45021E
                 U __imp__ZN12thinlto_test3aaa1F17h679dc1481bd45021E

$ nm target/release/deps/thinlto_test-f0129d30483da959.thinlto_test-72d93203162d98ec.thinlto_test.a44eecf0-cgu.0.rcgu.o.rcgu.o | rg thinlto_test3aaa1F17h679dc1481bd45021E
0000000000000000 D _ZN12thinlto_test3aaa1F17h679dc1481bd45021E

For now sticking with LLD is probably the best option, this issue needs more investigation what happened with dllimports/dllexports.

bors bot added a commit to crossbeam-rs/crossbeam that referenced this issue Sep 29, 2022
914: 0.8: Prepare for the next release r=taiki-e a=taiki-e

Backports #913 and CI-related patches.

Changes:
- crossbeam-epoch 0.8.11 -> 0.8.12
  - Removes the dependency on the `once_cell` crate to restore the MSRV. (#913)
  - Work around [rust-lang#98302](rust-lang/rust#98302), which causes compile error on windows-gnu when LTO is enabled. (#913)
- crossbeam-utils 0.8.11 -> 0.8.12
  - Removes the dependency on the `once_cell` crate to restore the MSRV. (#913)
  - Work around [rust-lang#98302](rust-lang/rust#98302), which causes compile error on windows-gnu when LTO is enabled. (#913)

Co-authored-by: Taiki Endo <[email protected]>
@Super-Pizza
Copy link
Author

Well, good to see some investigation! It seems to mostly affect crossbeam seeing all the links related to it.

@mati865
Copy link
Contributor

mati865 commented Sep 29, 2022

Guess I don't have permissions but let's try:

@rustbot modify labels: +regression-from-stable-to-stable
@rustbot prioritize

@rustbot rustbot added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 29, 2022
@mati865
Copy link
Contributor

mati865 commented Sep 30, 2022

Before 1.47.0 these symbols were created without dllimport/dllexport:

$ nm target/debug/deps/thinlto_test-17415e9ded1c2189.thinlto_test-c418b5c87981f3e8.13mswjpwc6x43k1o.rcgu.o.rcgu.o | rg _ZN12thinlto_test3aaa1F17ha0fefd1a54397fe9E
0000000000000000 D _ZN12thinlto_test3aaa1F17ha0fefd1a54397fe9E

$ nm target/debug/deps/thinlto_test-17415e9ded1c2189.lg950o7fadi5c1i.rcgu.o | rg _ZN12thinlto_test3aaa1F17ha0fefd1a54397fe9E
0000000000000000 r .rdata$.refptr._ZN12thinlto_test3aaa1F17ha0fefd1a54397fe9E
0000000000000000 R .refptr._ZN12thinlto_test3aaa1F17ha0fefd1a54397fe9E
                 U _ZN12thinlto_test3aaa1F17ha0fefd1a54397fe9E

Which makes me believe it's actually my fault in #72049

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@John-Nagle
Copy link

Possibly the cause of this problem. See taike-e's comment above.

It's happening in a situation where thinLTO has real benefits - locking primitives in a high-performance graphics crate.
Cross-compiling to --target x86_64-pc-windows-gnu target, compiling on Linux to multiple targets.

@John-Nagle
Copy link

John-Nagle commented Feb 20, 2023

In my program, changing

lto = "thin"

to

lto = "fat"

Fixed the problem. So, I seem to be hitting this bug too.

"Fat" is the right term here. The linking step fills up memory on the 8GB machine, and then Linux kills the process. I have to use a 32GB machine to link my program. Linking now takes 1-2 minutes, so compile times for a one-character change went from about 15 seconds to 90 seconds on the big machine (32GB, 12 cores.)

@mati865
Copy link
Contributor

mati865 commented Nov 7, 2024

Fixed in the nightly, likely by #129079

@Super-Pizza
Copy link
Author

Closing., since the issue seems fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. O-windows-gnu Toolchain: GNU, Operating system: Windows P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants