Add an optional COMDAT record for the .idata$3 section

[ChrisDenton]

I wasn't entirely sure how to usefully test this but I inserted a thing to check that using the comdat option still results in the same object files being in the archive, except for the .idata$3 changes.

Fixes #19

[bjorn3]

Did you test that using this fixes the /WHOLEARCHIVE issue?

[ChrisDenton]

I've test that it can produce a lib file which I can use with rustc when linking with /WHOLEARCHIVE. I've not tried hooking it up to rustc proper yet. But I can do that.

[bjorn3]

Did you try including two import libraries files for different dll's and including those in the staticlib generated by rustc? Or did you only try a single import library?

[ChrisDenton]

I was assuming the symbol would already have been found from another import library but good point.

Now that I've got a hacky build of rustc that uses my branch of ar_archive_writer, I know there's at least two import files used by std. So I returned to the original issue and retried some test cases:

R:\rust\dll> cat dll.def
LIBRARY dll
EXPORTS
        hello
        number

R:\rust\dll> cat static.rs
#[no_mangle]
pub extern "C" fn hello() {
    println!("Hello world!");
}

#[no_mangle]
pub extern "C" fn number() -> u32 {
        42
}
R:\rust\dll> rustc static.rs --crate-type="staticlib"
R:\rust\dll> link c /WHOLEARCHIVE:./static.lib /dll /def:dll.def /out:dll.dll msvcrt.lib kernel32.lib ws2_32.lib ntdll.lib userenv.lib
Microsoft (R) Incremental Linker Version 14.41.34120.0
Copyright (C) Microsoft Corporation.  All rights reserved.

static.lib(api-ms-win-core-synch-l1-2-0.dll) : error LNK2005: __NULL_IMPORT_DESCRIPTOR already defined in static.lib(bcryptprimitives.dll)
   Creating library dll.lib and object dll.exp
dll.dll : fatal error LNK1169: one or more multiply defined symbols found
R:\rust\dll> rustc +stage1 static.rs --crate-type="staticlib"
R:\rust\dll> link c /WHOLEARCHIVE:./static.lib /dll /def:dll.def /out:dll.dll msvcrt.lib kernel32.lib ws2_32.lib ntdll.lib userenv.lib
Microsoft (R) Incremental Linker Version 14.41.34120.0
Copyright (C) Microsoft Corporation.  All rights reserved.

   Creating library dll.lib and object dll.exp

It successfully links with the ar-archive-writer patch so this does appear to fix the issue.

[bjorn3]

Published as v0.3.4.

[bjorn3]

Wait, this is a breaking change.

[bjorn3]

Yanked and re-published as v0.4.0.

[ChrisDenton]

Do you know if there's anything blocking rustc itself from using ar-archive-writer in place of the LLVM writer? Or would it be ok to just make a PR that switches?

[dpaoliello]

Do you know if there's anything blocking rustc itself from using ar-archive-writer in place of the LLVM writer? Or would it be ok to just make a PR that switches?

There shouldn't be anything blocking the use of ar_archive_writer for generating import libraries for all backends.

I wouldn't want to switch rlib/static lib generation since ar_archive_writer doesn't support LLVM IR objects.

[ChrisDenton]

Thanks. That makes sense.

[dpaoliello]

One additional question: should we do something about -gnu targets requiring dlltool?

It sounds like binutils 2.40 should be able to handle LLVM's import libraries, but if we can't require that any time soon, should we modify ar_archive_writer to have an "old binutils compat mode" that generates import libs like binutil's dlltool?

[bjorn3]

I wouldn't want to switch rlib/static lib generation since ar_archive_writer doesn't support LLVM IR objects.

I actually switched the LLVM backend to ar_archive_writer yesterday in rust-lang/rust#128936. The LLVM backend uses a custom ObjectReader which calls into LLVM for getting all symbols. (Before this PR is was only calling into LLVM for getting symbols from LLVM bitcode and any file starting with 00 00 FF FF, but doing so seems to result in a symbol getting missed on MSVC.)

[bjorn3]

Do you know if there's anything blocking rustc itself from using ar-archive-writer in place of the LLVM writer? Or would it be ok to just make a PR that switches?

You can probably copy https://github.com/rust-lang/rustc_codegen_cranelift/blob/69cec6faeacb062033fec5c80e2d828d774b6531/src/archive.rs#L20-L92 verbatim for switching other backends to ar_archive_writer for import library generation.

[ChrisDenton]

One additional question: should we do something about -gnu targets requiring dlltool?

It sounds like binutils 2.40 should be able to handle LLVM's import libraries, but if we can't require that any time soon, should we modify ar_archive_writer to have an "old binutils compat mode" that generates import libs like binutil's dlltool?

I think mati865 is trying to update the mingw tools distributed with rust again. But there have been a lot of issues previously so I'm not too confident that it'll happen quickly. I guess there's also the question of what old distro's have.

Which is to say, if we did implement that then it probably wouldn't be a wasted effort.

[ChrisDenton]

Sorry to say, but I think I've convinced myself this isn't going to work. I just picked a platform (x64) a linker (msvc's link.exe) that just-so-happens to produce something that (mostly) works with the Windows loader, despite the issues it causes. Change any one of those things and everything falls apart.

I think import libs are fundamentally incompatible with /WHOLEARCHIVE. Any solution will need to be on the rustc side.

[bjorn3]

Opened #22 to track removing this feature again.