Only use dlltool.exe on MinGW if -Cdlltool is passed

[bjorn3]

View all comments

The linker used by MinGW now works with ar_archive_writer generated short import libraries too.

Implements rust-lang/compiler-team#1029

r? @mati865

[mati865]

Blocked only the on MCP.

View changes since this review

[bjorn3]

@rustbot ready

MCP has been accepted.

[mati865]

@bors r+

[rust-bors]

📋 This PR cannot be approved because it currently has the following label: S-waiting-on-MCP.

[mati865]

@bors r+

I suppose we will want to include it in release notes, no?

[rust-bors]

📌 Commit fd82481 has been approved by mati865

It is now in the queue for this repository.

[JonathanBrouwer]

@bors try jobs=dist-i686-mingw

[mati865]

I can look into that failure tomorrow. Looks like we don't prefix import symbols correctly on i686 windows GNU for some reason.

[JonathanBrouwer]

@bors r-
This is probably the problem

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#161688), which was unapproved.

View changes since this unapproval

[rust-bors]

💔 Test for 3d9e5bb failed: CI. Failed job:

• try - dist-i686-mingw (web logs, enhanced plaintext logs)

[mati865]

I can look into that failure tomorrow. Looks like we don't prefix import symbols correctly on i686 windows GNU for some reason.

Okay, I couldn't resist.
This refactor will make it clearer how things work: #161701 and this is how this PR should look like (modulo updated comments): mati865@fe906617

[bjorn3]

and this is how this PR should look like (modulo updated comments): mati865@fe906617

Doesn't that mean -Cdlltool=foo would require recompiling the standard library as the standard library would contain ar_archive_writer generated import libraries, but functions that contains calls against these import libraries this might be monomorphized in crates that have -Cdlltool passed to them.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[mati865]

Uhm, that's a good point about stdlib. I was under impression we don't use raw-dylib by default there, but that's not the case.
That's because https://github.com/rust-lang/rust/blob/e457a7b0d326d67b4322ef0d11bd715cfaeda48f/library/windows_link/src/lib.rs is not the only way we import Windows libs as I previously thought.
Some of them are always using raw-dylib, like

rust/library/std/src/sys/pal/windows/c.rs

Line 105 in e457a7b

link(name = "bcryptprimitives", kind = "raw-dylib", import_name_type = "undecorated")

or

rust/library/std/src/sys/pal/windows/c.rs

Line 166 in e457a7b

kind = "raw-dylib",

So, your change already make the std contain ar_archive_writer generated import libraries and user cannot opt out them without rebuilding std.

And we only saw that because without changes in mati865@fe906617 your PR creates mismatch in symbol decoration logic (ar_archive_writer vs dlltool). And that logic is scattered across many places as you can see in my commit.

[mati865]

If we apply mati865@fe906617 and build std with -Cdlltool for now, we wouldn't need to worry about monorphization, right?

[bjorn3]

Wouldn't we need to record whether dlltool was used into the raw-dylib definition and then choose between the codegen path for dlltool vs ar_archive_writer when codegening calls to this raw-dylib? Otherwise standard library functions codegened outside the standard library would assume ar_archive_writer even though only a dlltool import library is present.

[mati865]

Otherwise standard library functions codegened outside the standard library would assume ar_archive_writer even though only a dlltool import library is present.

Certainly I'm not up to speed in this area. Does that mean we might feed ar_archive_writer an undecorated symbol (meant for dlltool)?
If that's the case, then pulling the rug and removing dlltool support altogether might be the only sensible way forward.

[bjorn3]

If I understand correctly ar_archive_writer and dlltool generated import libraries will expose the imported functions under different names. (I think one exposes them with their decorated name and the other with their undecorated name) And because of this cg_llvm has to generate different calls depending on if the import library was generated by ar_archive_writer or by dlltool. If I recall correctly the import library is generated (and thus the symbol name set to one or the other option) when compiling the crate that defines the raw-dylib. But if calls are generated in another crate, mati865@fe906617 would pick the symbol name to call based on the local -Cdlltool value rather than when the import library was generated.

If that's the case, then pulling the rug and removing dlltool support altogether might be the only sensible way forward.

I'm afraid so.

[mati865]

If I understand correctly ar_archive_writer and dlltool generated import libraries will expose the imported functions under different names. (I think one exposes them with their decorated name and the other with their undecorated name)

Dlltool adds the decorations itself (or rather adds them unless you pass --no-leading-underscore like we started doing at some point), so we have to skip them in that branch. This case is similar to how we use it:

❯ printf 'EXPORTS\nbar' > foo.def

❯ i686-w64-mingw32-dlltool -d foo.def -D foo.dll -l libfoo.dll.a

❯ nm libfoo.dll.a | rg bar
00000000 T _bar
00000000 I __imp__bar

We asked it to create dllimported bar, so it prefixed it with __imp_, but since this is i686 it also added the underscore right before bar part, resulting in __imp__bar. The exact same names will be made by ar_archive_writer, we just have to feed it the already decorated names.

There goes the theory, I just checked the binaries and in the practice we mess up big time here.
This is how our raw-dylib import symbols should look like:

❯ llvm-nm /usr/i686-w64-mingw32/lib/libbcryptprimitives.a | rg ProcessPrng | rg -v Guid
00000000 T _ProcessPrng@8
00000000 I __imp__ProcessPrng@8
# or (both are fine)
❯ llvm-nm /opt/llvm-mingw/llvm-mingw-ucrt/i686-w64-mingw32/lib/libbcryptprimitives.a | rg ProcessPrng | rg -v Guid
00000000 T _ProcessPrng@8
00000000 T __imp__ProcessPrng@8

And sure i686-pc-windows-gnullvm looks like that:

❯ llvm-nm ~/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/i686-pc-windows-gnullvm/lib/libstd-5c84ca49ba3e22ca.rlib | rg ProcessPrng
         U __imp__ProcessPrng@8
00000000 T _ProcessPrng@8
00000000 T __imp__ProcessPrng@8

But what the heck happened with i686-pc-windows-gnu?

❯ llvm-nm ~/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/i686-pc-windows-gnu/lib/libstd-28eef08820bd8b87.rlib | rg ProcessPrng
         U __imp_ProcessPrng
00000000 T ProcessPrng
00000000 I __imp_ProcessPrng

_ProcessPrng@8 is stdcall calling convention, _ProcessPrng would be cdecl, but ProcessPrng, on i686?
I knew that mingw-w64 and the linker can fixup wrong calling convention, but bad mangling? That's a novelty to me, and I have no idea how it doesn't straight up crash when running (I made an example using that symbol, it really works).

And because of this cg_llvm has to generate different calls depending on if the import library was generated by ar_archive_writer or by dlltool. If I recall correctly the import library is generated (and thus the symbol name set to one or the other option) when compiling the crate that defines the raw-dylib.

We should always generate call to the same dllimport symbol, no matter how the import lib was made.

"Do as I say, not as I do"

rust/compiler/rustc_codegen_llvm/src/callee.rs

Lines 139 to 144 in fe90661

	// MinGW: For backward compatibility we rely on the linker to decide whether it
	// should use dllimport for functions.
	if cx.use_dll_storage_attrs
	&& let Some(library) = tcx.native_library(instance_def_id)
	&& library.kind.is_dllimport()
	&& !matches!(tcx.sess.target.env, Env::Gnu | Env::Uclibc)

But that is irrelevant for our raw-dylib consideration.

So, if we fix this cursed dlltool hack and both ar_archive_writer and dlltool generate the symbols, there should be no problem with switching between them?
But is it even worth investing time into that? Nuking all dlltool usage from the compiler would be so much easier...

[ChrisDenton]

But is it even worth investing time into that? Nuking all dlltool usage from the compiler would be so much easier...

Yes, I think we should just nuke it. If it had worked with the ar_archive_writer implementation from the beginning then we would never have introduced dlltool in the first place. I'm not sure what keeping it around in zombie form gains us, tbh. I guess it could be a hedge against any implementation issues but I'd rather put the effort into fixing any bugs or compatibility problems that may (or hopefully won't) crop up.

[mati865]

Haven't ran the tests on Windows machine yet and some changes are done hastily without deep consideration, but that looks like a nice cleanup in addition to fixing the discrepancy: main...mati865:rust:push-wwnqklwnwkwu

Before:

❯ nm build/x86_64-unknown-linux-gnu/stage1-std/i686-pc-windows-gnu/dist/build/std/47b34ecc0226a577/out/libstd-47b34ecc0226a577.rlib | rg ProcessPrng
         U __imp_ProcessPrng
00000000 I __imp_ProcessPrng
00000000 T ProcessPrng

❯ nm build/x86_64-unknown-linux-gnu/stage1-std/i686-pc-windows-gnullvm/dist/build/std/d118868017530d1d/out/libstd-d118868017530d1d.rlib | rg ProcessPrng
         U __imp__ProcessPrng@8
00000000 I __imp__ProcessPrng@8
00000000 T _ProcessPrng@8

After:

❯ nm build/x86_64-unknown-linux-gnu/stage1-std/i686-pc-windows-gnu/dist/build/std/47b34ecc0226a577/out/libstd-47b34ecc0226a577.rlib | rg ProcessPrng
         U __imp__ProcessPrng@8
00000000 I __imp__ProcessPrng@8
00000000 T _ProcessPrng@8

❯ nm build/x86_64-unknown-linux-gnu/stage1-std/i686-pc-windows-gnullvm/dist/build/std/d118868017530d1d/out/libstd-d118868017530d1d.rlib | rg ProcessPrng
         U __imp__ProcessPrng@8
00000000 I __imp__ProcessPrng@8
00000000 T _ProcessPrng@8

[mati865]

^^ no, forget about it. Dlltool was not designed to handle all our cases. Achieving identical symbol decorations for typical use cases is easy, but handling import_name_type would require us to create multiple import libs for the same library with different dlltool options, and I don't even know if this will link successfully.

We are left only with the nuclear solution.