Stop using dlltool for generating import libraries on MinGW

[bjorn3]

View all comments

Older versions of ld.bfd didn't support short import libraries like the builtin implementation of rustc generates. So we worked around this by using binutils dlltool, which produces the old import library format. We have now bumped the minimum supported ld.bfd version to one which does support them, so we can drop the dlltool usage. This makes cross-compilation a bit easier and removes a bunch of code in rustc.

This implements rust-lang/compiler-team#1029 with the exception of removing -Cdlltool support immediately rather than doing a two step migration.

[rustbot]

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred in src/tools/compiletest

cc @jieyouxu

compiletest directives have been modified. Please add or update docs for the
new or modified directive in src/doc/rustc-dev-guide/.

[bjorn3]

With #158020 merged, I believe this can now be done.

@rustbot ready

[rustbot]

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: codegen, compiler
• codegen, compiler expanded to 73 candidates
• Random selection from 19 candidates

[bjorn3]

cc @mati865

[mati865]

I think the consensus on Zulip was that we want to hold back the breaking changes until next release after the bump. To give the time for updating.
So this would need to wait for branching, which is 2 weeks away.

[mati865]

Older versions of ld.bfd didn't support short import libraries like the builtin implementation of rustc generates.

Well, they did but with caveats. Like, it broke when import libs were put inside archive like staticlib/rlib.

[bjorn3]

My bad. Let's wait for beta to branch then.

@rustbot author

[mati865]

It's been almost 4 weeks.
ping @TaKO8Ki as the reviewer

[TaKO8Ki]

@bors r+

[rust-bors]

📌 Commit d4baa00 has been approved by TaKO8Ki

It is now in the queue for this repository.

[mati865]

Asked on Zulip whether this requires a MCP due to stable flag removal. Previously we did that via MCP: #152973

Pulling off the merge queue until answered, if MCP is not required then I apologize for the noise.
@bors r-

EDIT: also sorry for the late notice, I' been meaning to review it but couldn't remember to do that when I had time.

[rust-bors]

This pull request was unapproved.

View changes since this unapproval

MCP is necessary.

[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.

[bjorn3]

@rustbot ready

Turns out we will have to this rather than the more gradual migration path in #161050 after all due to a standard library being compiled without usage of dlltool being ABI incompatible with user code compiled with usage of dlltool and vice versa: #161050 (comment)

@rustbot fcp compiler

This will stop us from respecting -Cdlltool on the *-pc-windows-gnu targets and instead unconditionally use ar_archive_writer to match all other Windows targets.

[bjorn3]

@rfcbot fcp compiler

[bjorn3]

@rfcbot merge compiler

This will stop us from respecting -Cdlltool on the *-pc-windows-gnu targets and instead unconditionally use ar_archive_writer to match all other Windows targets.

[rust-rfcbot]

@bjorn3 has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Mark-Simulacrum
• @TaKO8Ki
• @davidtwco
• @estebank
• @jdonszelmann
• @jieyouxu
• @nnethercote
• @oli-obk
• @petrochenkov
• @spastorino
• @wesleywiser

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[Mark-Simulacrum]

Checking my box, but can you make sure the PR description is updated to reflect that this has diverged from the MCP and is no longer implementing it? I'd also be curious on what clang / gcc do for mingw targets (not directly blocking for the decision though).

Also, can someone confirm that this change shouldn't be ABI-breaking with regards to usage of C libraries or producing them (e.g., staticlib/cdylib) on mingw? This sounds related to the gcc/clang concern.

[bjorn3]

but can you make sure the PR description is updated to reflect that this has diverged from the MCP and is no longer implementing it?

Done

Also, can someone confirm that this change shouldn't be ABI-breaking with regards to usage of C libraries or producing them (e.g., staticlib/cdylib) on mingw? This sounds related to the gcc/clang concern.

Import libraries generated by raw-dylib shouldn't be used outside Rust. Using C DLLs will work fine as only the local symbol name changes, not the name the DLL exports it as. Producing cdylib will be unchanged too as the import library is generated by the linker. And for importing C staticlibs or C DLLs with non-raw-dylib import library I don't think we had the symbol name generation special cased on MinGW before in the first place.

[mati865]

I'd also be curious on what clang / gcc do for mingw targets (not directly blocking for the decision though).

They do nothing as there is no raw-dylib equivalent in C/C++.
The scenario where you want to link library that is not installed is extremely rare. Though, if going down that route, you have to create the definition file yourself and call dlltool via the build system like Meson or CMake.