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

linker: Allow MSVC to use Meson and MinGW-style libraries #123436

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

amyspark
Copy link

@amyspark amyspark commented Apr 3, 2024

Hi all,

This PR implements support for MsvcLinker to use static and import libraries that have been generated by MinGW toolchains and the Meson build system.

For the latter case, I had to carve a new parameter for link_dylib_by_name so that it could perform the path lookup for the import library, as Meson follows the libfoo.dll.a naming convention to disambiguate between static and import libraries.

All feedback is appreciated!

Fixes #122455

cc @sdroege @nirbheek

@rustbot
Copy link
Collaborator

rustbot commented Apr 3, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @compiler-errors (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 3, 2024
@mati865
Copy link
Contributor

mati865 commented Apr 4, 2024

Sorry for the offtpic but I'm curious whether there weren't compatibility issues regarding Meson decision.
Unlike import libraries and DLLs, static libraries are basically incompatible between MSVC and MinGW. So using the same extension for them is risky.

That said, making dylibs consistent with ribs and static libs sounds like a good idea.

@nirbheek
Copy link

nirbheek commented Apr 4, 2024

Meson follows the libfoo.dll.a naming convention

typo: libfoo.a naming convention for static libraries.

Sorry for the offtpic but I'm curious whether there weren't compatibility issues regarding Meson decision.
Unlike import libraries and DLLs, static libraries are basically incompatible between MSVC and MinGW. So using the same extension for them is risky.

This is not accurate, we wrote the above-linked Meson FAQ because it's a common misconception. I'll repeat and expand the relevant bits here for the benefit of everyone reading:

  1. MSVC and MinGW toolchains both use the ar format for static libraries (and also for import libraries, but that's a separate matter), so the file format is compatible.
  2. MSVC and MinGW have also been using UCRT for quite some time now. MSYS2 has a UCRT64 variant that is the recommended variant.
  3. You should not mix static libraries built with different CRTs, but that is true regardless of which toolchain you are using. You will get exactly the same problems if you link MSVC static libraries built with MSVCRT with MSVC static libraries built with UCRT.
    • In the current implementation, this configuration is considered user error, and outside of the purview of the linker.

In summation: there is no reason to treat MinGW-built and MSVC-built static libraries as incompatible. In fact, at GStreamer we have been linking them together for 5 years with no adverse effects, since everything is using UCRT.

FWIW, as of recently, CMake also accepts libfoo.a static libraries via pkg-config when building with an MSVC toolchain.

An important quality-of-life change for users there is to ignore static libraries presented by Strawberry Perl's pkg-config[1][2] (which is unconditionally added to PATH and must not be used as a MinGW distribution), since Strawberry Perl is included in PATH in Github CI images by default, and there is no way to turn it off.

Similar to CMake and Meson, this needs to be handled in pkg-config-rs, and I have filed an issue about it.

@petrochenkov petrochenkov self-assigned this Apr 4, 2024
@amyspark
Copy link
Author

amyspark commented Apr 4, 2024

typo: libfoo.a naming convention for static libraries.

@nirbheek I was thinking of the particular case of import libraries (libfoo.dll.a) for which I already experienced such a clash elsewhere: https://aomedia-review.googlesource.com/c/aom/+/173603

@compiler-errors
Copy link
Member

r? @wesleywiser

sorry, not the best person to review this

@mati865
Copy link
Contributor

mati865 commented Apr 9, 2024

@nirbheek at MSYS2 we've had many reports of broken builds when using static libraries made with MSVC in mingw-w64 based toolchains and we always referred to what I think was original MinGW (not mingw-w64) webpage or some mailing list message that said DLLs are the only supported interface between MSVC and MinGW.
Maintainer of mingw-w64 stuff at LLVM says the same thing: mstorsjo/llvm-mingw#305 (comment)
It's true UCRT fixed many of the issues but I doubt any mingw-w64 developer would claim static libraries to be fully compatible. I don't mean it doesn't work, it's just unsupported and shouldn't be considered as a bug if compatibly breaks.

Some of the issues on top of my head:

  • MSVCRT toolchains basically required __USE_MINGW_ANSI_STDIO to have working stdio but it's not compatible with MSVC (MSYS2 still enables it for all packages, even UCRT ones)
  • GCC (and IIRC Clang to match it) uses x87 for f64/double on 32-bit targets
  • Dwarf-2 debuginfo used by 32-bit toolchains is definitely not compatible with MSVC, IIRC SEH used by 64-bit toolchains is also somewhat different from MSVC
  • there used to be (maybe still is?) some discrepancy between MSVC and GCC/Clang at registers usage in edge cases, cannot recall the details after all those years, could be related to varargs
  • C and C++ libraries not distinguishable by the linker and accidental inclusion of any C++ library is a guaranteed broken build

MSVC and MinGW toolchains both use the ar format for static libraries (and also for import libraries, but that's a separate matter), so the file format is compatible.

File format is not the problem here but since lib<name>.a is typically associated with MinGW/mingw-w64 toolchains this might be confusing for users when mingw-w64 GCC/Clang created static library breaks their build, like infamous strawberry perl that you've mentioned.

MSVC and MinGW have also been using UCRT for quite some time now. MSYS2 has a UCRT64 variant that is the recommended variant.

Unfortunately there are still many vendors providing MSVCRT toolchain as the default one, especially when cross-compiling.

You should not mix static libraries built with different CRTs, but that is true regardless of which toolchain you are using.

OFC, we've been telling it at MSYS2 for years, it's also mentioned at MSYS2 webpage.

In summation: there is no reason to treat MinGW-built and MSVC-built static libraries as incompatible. In fact, at GStreamer we have been linking them together for 5 years with no adverse effects, since everything is using UCRT.

This is refreshing to hear things are getting better but I still worry those are either too simple libraries to exhibit incompatibility issues or was not tested deeply enough.

FWIW, as of recently, CMake also accepts libfoo.a static libraries via pkg-config when building with an MSVC toolchain.

Oh, I didn't know about it yet.


Regarding Meson's FAQ

Both Clang and GNU compilers can search for libfoo.a when specifying a library as -lfoo. This does not work for alternative naming schemes for static libraries such as libfoo.lib.

Both ld.bfd and LLD search for <name>.lib (notice missing lib) which is MSVC naming convention, but .dll.a and .a libs are higher priority.

Projects built with the MinGW compiler are fully compatible with MSVC as long as they use the same CRT (e.g. UCRT with MSYS2).

Meson is free to support it (and workaround eventual issues) but this is not officially supported by GNU nor LLVM mingw-w64 based toolchains.

@petrochenkov
Copy link
Contributor

I have a general concern about turning linker library_by_name invocations into linker /full/path/library_by_name invocations, where the library_by_name -> /full/path/library_by_name conversion is done by rustc.

In the first case, the search directories are determined by the linker and may include some system directories, specific to a distribution directory layout, or controlled by some default linker config, or something else outside of rustc's purview.
In the second case the search paths will only include directories inside rustc toolchain, or directories explicitly passed with -L options.

find_native_static_library is currently used when the library actually needs to be processed by rustc itself (not by linker), e.g. to pack it into a rlib.
In that case you don't have a choice but to use rustc's search directories without looking at the system (unless explicitly asked by user).
(Other 2 uses of find_native_static_library in linker.rs are for options that apparently don't support linker search by name at all.)

What system directories we can lose when replacing linker search with rustc search on windows-(msvc,gnu) targetes?

@petrochenkov
Copy link
Contributor

If we just fully switched to rustc-based library search everywhere, then I suspect we'd fail to find libc on like half of Unix-like systems.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 10, 2024
@nirbheek
Copy link

@mati865 Thanks for the detailed explanation of MinGW and MSVC compatibility. My point was primarily that the two are not "incompatible" as was stated, but that they are indeed compatible in many cases, and people have been using it for years.

You've given many more cases, such as C++ static libraries, where there is incompatibility even with static libraries produced by the same toolchain, which IMO supports the argument that this is not the linker's job. It's up to the user and the build system.

This is refreshing to hear things are getting better but I still worry those are either too simple libraries to exhibit incompatibility issues or was not tested deeply enough.

We build 100 projects, most of which used to be built with Autotools/MinGW and have been slowly ported over to Meson.

Regarding Meson's FAQ

Both Clang and GNU compilers can search for libfoo.a when specifying a library as -lfoo. This does not work for alternative naming schemes for static libraries such as libfoo.lib.
Both ld.bfd and LLD search for .lib (notice missing lib) which is MSVC naming convention, but .dll.a and .a libs are higher priority.

The point is that foo.lib is already an import library, so we can't use that because it causes a filename collision. One other option is libfoo.lib which is used in some places, but Clang and GNU compilers do not pick it up when specifying a library as -lfoo, so it had to be rejected.

@nirbheek
Copy link

I have a general concern about turning linker library_by_name invocations into linker /full/path/library_by_name invocations, where the library_by_name -> /full/path/library_by_name conversion is done by rustc.

It gets worse, as I understand it, Rust is also converting absolute paths into -Ldir/ -lfoo pairs first, and then applying this conversion to convert it back to an absolute path. This can be very wrong in cases where you're picking up libraries from multiple -L paths, or if there are multiple options and the user prefers one.

@bors
Copy link
Contributor

bors commented Apr 13, 2024

☔ The latest upstream changes (presumably #123854) made this pull request unmergeable. Please resolve the merge conflicts.

@amyspark amyspark force-pushed the allow-msvc-to-use-meson-and-mingw-import-libraries branch from 33da95a to df739a4 Compare April 18, 2024 00:50
@amyspark
Copy link
Author

What system directories we can lose when replacing linker search with rustc search on windows-(msvc,gnu) targetes?

@petrochenkov If I understand you correctly, by rustc search you mean resolving and baking in the libraries' path, right? I'd definitely prefer this approach, it'll make error logs noisier because of the command length, but it'll definitely tell us which files are going to be linked.

Rebase pushed to address bors's detection too.

@rust-log-analyzer

This comment has been minimized.

@amyspark amyspark force-pushed the allow-msvc-to-use-meson-and-mingw-import-libraries branch from df739a4 to 8cdf1ee Compare April 18, 2024 01:08
@rust-log-analyzer

This comment has been minimized.

@amyspark amyspark force-pushed the allow-msvc-to-use-meson-and-mingw-import-libraries branch 2 times, most recently from c44e94f to 4d3b7e8 Compare April 20, 2024 00:19
@rust-log-analyzer

This comment has been minimized.

@amyspark amyspark force-pushed the allow-msvc-to-use-meson-and-mingw-import-libraries branch from 4d3b7e8 to c31cd60 Compare April 20, 2024 00:37
@amyspark
Copy link
Author

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 20, 2024
@petrochenkov
Copy link
Contributor

Reviewing this requires from me doing some additional research, so I won't get to it until I return from vacation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rust can detect GNU/Meson style named libraries but the MSVC linker backend ignores them
9 participants