Fix weak linkage on windows and apple platforms #672

Amjad50 · 2024-08-22T03:01:52Z

Fixes #653

There were some issues regarding windows and apple platform, we were exporting symbols that are already provided by the compiler but weren't marked as weak which resulted in conflicted symbols in the linking process.

Initially, we didn't add weak because we thought it is not supported on windows and apple platforms, but it looks like its only not supported on windows-gnu platforms

There were some issues regarding windows and apple platform, we were exporting symbols that are already provided by the compiler but weren't marked as `weak` which resulted in conflicted symbols in the linking process. Initially, we didn't add `weak` because we thought it is not supported on windows and apple platforms, but it looks like its only not supported on windows-gnu platforms Signed-off-by: Amjad Alsharafi <26300843+Amjad50@users.noreply.github.com>

tgross35 · 2024-08-22T06:03:23Z

Thanks for the fix, this matches what was in src/mem/mod.rs before #598. What exactly happens on windows GNU?

tgross35 · 2024-08-22T06:06:13Z

Could you create a PR to rust-lang/rust updating compiler-builtins 0.1.120 after the release PR merges? #671

Amjad50 · 2024-08-22T06:07:46Z

Looks like it crashes for some reason on windows-gnu, for some reason with weak it gives undefined reference errors: there is a PR that tried to fix the issue by making everything weak (#660)

Amjad50 · 2024-08-22T06:08:55Z

Could you create a PR to rust-lang/rust updating compiler-builtins 0.1.120 after the release PR merges? #671

Sure

…r=tgross35 Update `compiler_builtins` to `0.1.120` Includes rust-lang/compiler-builtins#672 which fixes regression issue with Apple and Windows compilers. r? `@tgross35`

mati865 · 2024-08-22T17:14:19Z

Looks like it crashes for some reason on windows-gnu, for some reason with weak it gives undefined reference errors: there is a PR that tried to fix the issue by making everything weak (#660)

Do you have full error messages?
AFAIK weak symbols are unsupported in windows-msvc, there is some support in windows-gnu (GCC+Binutils), but it works only with static libraries and they mostly work with windows-gnullvm, although there are some DLL inherited limitations with shared libraries.

mati865 · 2024-08-22T17:15:56Z

src/macros.rs

@@ -256,7 +256,7 @@ macro_rules! intrinsics {
        #[cfg(all(any(windows, target_os = "uefi"), target_arch = "x86_64", not(feature = "mangled-names")))]
        mod $name {
            #[no_mangle]
-            #[cfg_attr(all(not(windows), not(target_vendor = "apple")), linkage = "weak")]
+            #[cfg_attr(not(all(windows, target_env = "gnu")), linkage = "weak")]


It should likely also check not(target_abi = "llvm", how do you test it? I'd like to test with gnullvm.

Just testing this crate locally? You should be able to cd to testcrate and cargo test --target ....

Oh, I thought this issue would manifest only during Rust build, gonna look into it this weekend.

The rust-lang/rust update of compiler-builtins merged, so it should also be available to test with the next rustc nightly (couple hours after 0:00 UTC)

…<try> Update `compiler_builtins` to `0.1.120` Includes rust-lang/compiler-builtins#672 which fixes regression issue with Apple and Windows compilers. try-job: aarch64-apple try-job: x86_64-apple-1 try-job: x86_64-msvc

tgross35 · 2024-08-22T19:38:23Z

Looks like it crashes for some reason on windows-gnu, for some reason with weak it gives undefined reference errors: there is a PR that tried to fix the issue by making everything weak (#660)

Do you have full error messages?

The error messages are at the PR linked there #660, direct link https://github.com/rust-lang/compiler-builtins/actions/runs/10257106333/job/28377511730?pr=660. Basically a lot of Warning: .drectve `-exclude-symbols:"..." ' unrecognized, followed by undefined referencees. Possibly related to rust-lang/rust#112368.

AFAIK weak symbols are unsupported in windows-msvc, there is some support in windows-gnu (GCC+Binutils), but it works only with static libraries and they mostly work with windows-gnullvm, although there are some DLL inherited limitations with shared libraries.

MSVC does support some form of weak linking, see #653 and a comment about DLLs there. We were already using this for arm functions so it seems to have worked, see the changes to src/arm.rs in #598.

Are you usually able to link windows-gnu binaries with weak symbols?

mati865 · 2024-08-22T21:46:37Z

The error messages are at the PR linked there #660, direct link rust-lang/compiler-builtins/actions/runs/10257106333/job/28377511730?pr=660.

Bummer, it requires inspection of the objects and won't be as simple as hoped.

Basically a lot of Warning: .drectve -exclude-symbols:"..." ' unrecognized␍, followed by undefined reference`es. Possibly related to rust-lang/rust#112368.

Yeah, that's caused by an outdated linker. It's harmless but causes binaries to contain more symbols than they need to (MSVC behaves the same way modulo the warnings as this is LLVM's invention).

MSVC does support some form of weak linking, see #653 and a comment about DLLs there.

Oh, I haven't heard about them before. Regarding DLLs, there is a "we have weak symbols at home" attribute called selectany, while it's not nearly as powerful as weak symbols, it's sometimes used to emulate it. What it does is making the linker pick the first occurrence of a symbol when there are multiple definitions.

Are you usually able to link windows-gnu binaries with weak symbols?

As long as they all come from static libraries and all are resolved (Windows binaries cannot contain undefined references) they seem to work. They fall apart when you try to use them inside DLLs.
Clang+LLD can handle them better in more situations but is still bound by Windows limitations.

tgross35 · 2024-08-22T22:57:33Z

Do you have any specific concerns about the change here, or just that it would be ideal if they worked on windows-gnu too? I ran a msvc try job on the update PR at rust-lang/rust#129400 and it passed, so I don't think there is anything too worrying. Especially since it seems builtins always gets statically linked, per my comment here #653 (comment) - are weak symbols for DLLs still relevant with this in mind?

For what it's worth, if there is a newer linker version that doesn't hit the listed errors then feel free to send a PR updating our CI jobs here as needed. I ran into it here too #667.

…r=tgross35 Update `compiler_builtins` to `0.1.120` Includes rust-lang/compiler-builtins#672 which fixes regression issue with Apple and Windows compilers. try-job: aarch64-apple try-job: x86_64-apple-1 try-job: x86_64-msvc

Rollup merge of rust-lang#129400 - Amjad50:update-compiler-builtins, r=tgross35 Update `compiler_builtins` to `0.1.120` Includes rust-lang/compiler-builtins#672 which fixes regression issue with Apple and Windows compilers. try-job: aarch64-apple try-job: x86_64-apple-1 try-job: x86_64-msvc

mati865 · 2024-08-23T15:51:29Z

Do you have any specific concerns about the change here, or just that it would be ideal if they worked on windows-gnu too?

Regarding windows-gnu just a slight concern, something might be still broken and was just hidden.
When it comes to windows-gnullvm, (as the maintainer) I believe it should work on par with both MSVC and GNU. Otherwise it has to be fixed in LLVM.

Especially since it seems builtins always gets statically linked, per my comment here #653 (comment) - are weak symbols for DLLs still relevant with this in mind?

There should be no problem for MSVC since weak symbols were just proven to work. I'm concerned only about windows-gnu breaking during some supposedly irrelevant changes.

For what it's worth, if there is a newer linker version that doesn't hit the listed errors then feel free to send a PR updating our CI jobs here as needed. I ran into it here too #667.

I attempted to build a new version with recent components using crosstool-ng to save time (I was terribly wrong here...) and wasted dozens of hours on it: https://github.com/mati865/mingw-build rust-lang/rust#119229
But the mingw-builds project is getting a new release, so hopefully it can be solved soonish.

tgross35 · 2024-08-23T18:35:29Z

windows-gnu shouldn't have ever had weak intrinsics now or before, right?

Would you mind opening a new issue so we can keep a better eye on this?

mati865 · 2024-08-25T11:18:45Z

windows-gnu shouldn't have ever had weak intrinsics now or before, right?

TBH I have no idea how they are used and whats their purpose, so I cannot answer that.

Would you mind opening a new issue so we can keep a better eye on this?

Regarding cfg attributes from this PR or something else?

FWIW I've tested both windows-gnu and windows-gnullvm locally with PR reverted. Running cargo test --target i686-pc-windows-gnu (similarly for gnullvm) inside testcrate/ works fine, even ./ci/run.sh i686-pc-windows-gnu got to the point when it errored about missing compiler-rt:

  thread 'main' panicked at build.rs:600:17:
  RUST_COMPILER_RT_ROOT is not set. You may need to download compiler-rt.

Either CI GNU C toolchain (the linker and stuff) has a bug, or this error happens before reaching CI's error.

Also it struck me just now, the 64-bit target has passed, so this might be an issue with name mangling. 32-bit targets (all of them, be it GNU, LLVM, MSVC) use additional underscore before symbol names. Maybe that's the problem here?

tgross35 · 2024-08-25T19:56:49Z

Would you mind opening a new issue so we can keep a better eye on this?

Regarding cfg attributes from this PR or something else?

Whatever you would like to continue investigating or are still looking into :) I just don't want to continue discussion on a merged PR, so you can repost the above couple comments to a new issue.

tgross35 merged commit be4374f into rust-lang:master Aug 22, 2024
24 checks passed

Amjad50 deleted the fix-weak-linkage branch August 22, 2024 06:08

tgross35 mentioned this pull request Aug 22, 2024

Mark symbols as weak on Windows and Apple as well #660

Closed

Amjad50 mentioned this pull request Aug 22, 2024

Update compiler_builtins to 0.1.120 rust-lang/rust#129400

Merged

tgross35 mentioned this pull request Aug 22, 2024

Various symbols on win/apple are no longer weak after #598 #653

Open

mati865 reviewed Aug 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix weak linkage on windows and apple platforms #672

Fix weak linkage on windows and apple platforms #672

Amjad50 commented Aug 22, 2024

tgross35 commented Aug 22, 2024

tgross35 commented Aug 22, 2024

Amjad50 commented Aug 22, 2024

Amjad50 commented Aug 22, 2024

mati865 commented Aug 22, 2024

mati865 Aug 22, 2024

tgross35 Aug 22, 2024

mati865 Aug 23, 2024

tgross35 Aug 23, 2024

tgross35 commented Aug 22, 2024 •

edited

Loading

mati865 commented Aug 22, 2024 •

edited

Loading

tgross35 commented Aug 22, 2024 •

edited

Loading

mati865 commented Aug 23, 2024 •

edited

Loading

tgross35 commented Aug 23, 2024

mati865 commented Aug 25, 2024

tgross35 commented Aug 25, 2024

Fix weak linkage on windows and apple platforms #672

Fix weak linkage on windows and apple platforms #672

Conversation

Amjad50 commented Aug 22, 2024

tgross35 commented Aug 22, 2024

tgross35 commented Aug 22, 2024

Amjad50 commented Aug 22, 2024

Amjad50 commented Aug 22, 2024

mati865 commented Aug 22, 2024

mati865 Aug 22, 2024

Choose a reason for hiding this comment

tgross35 Aug 22, 2024

Choose a reason for hiding this comment

mati865 Aug 23, 2024

Choose a reason for hiding this comment

tgross35 Aug 23, 2024

Choose a reason for hiding this comment

tgross35 commented Aug 22, 2024 • edited Loading

mati865 commented Aug 22, 2024 • edited Loading

tgross35 commented Aug 22, 2024 • edited Loading

mati865 commented Aug 23, 2024 • edited Loading

tgross35 commented Aug 23, 2024

mati865 commented Aug 25, 2024

tgross35 commented Aug 25, 2024

tgross35 commented Aug 22, 2024 •

edited

Loading

mati865 commented Aug 22, 2024 •

edited

Loading

tgross35 commented Aug 22, 2024 •

edited

Loading

mati865 commented Aug 23, 2024 •

edited

Loading