WIP raw_dylib: write the import .lib manually.

[simonbuchan]

Implements #1345.

Currently succeeds building and running the windows-sys crates enum-windows example with RUSTFLAGS=--cfg windows_raw_dylib.

TODO (in no particular order):

• Get her running!
• Replace archive writing code with ar_archive_writer
• Replace coff file writing code with object::write - see Upstreaming some archive / coff short import writing code? gimli-rs/object#591 (comment)
• Implement ordinal / hint support
• Check / implement #[link(import_name_type)
• See what's needed for windows-gnu target - it might be in scope? looks out of scope: WIP raw_dylib: write the import .lib manually. #1414 (comment)
• Implement machine types seems like only x86_64 is supported for windows in cranelift right now?
• Handle being called for other targets (just asserts
• Remove all as casts for explicit try_into() checks (with .expect() where appropriate).
• Error reporting instead of panicking.
• Add tests?

[bjorn3]

Thanks for working on this!

[bjorn3]

You can reuse the archive writer code of ar_archive_writer, right? This is already used by rustc, so adding extern crate ar_archive_writer; to lib.rs should be enough, no need to add it as dependency to Cargo.toml.

[bjorn3]

There seems to be a dearth of ar file writers on crates.io that support the Windows .lib symbol directory members (they need to be before the long names // member, which rules out ar crate for example), but I could have just missed it?

ar_archive_writer supports this. It is a direct port of LLVM's archive writer.

[simonbuchan]

Huh, I did start by checking that crate but didn't take a second look after ar didn't work out and I looked into the particulars of .libs, for some reason. It does panic on Coff, but that probably doesn't matter.

[ChrisDenton]

The LLVM writer (and therefore presumably ar_archive_writer) only writes the legacy linker member and not the modern one. However, msvc seems to be ok with this currently. It'd be great to write both but it's not strictly necessary.

[bjorn3]

The object crate has all those constants.

[bjorn3]

Maybe this should be added to the object crate? It already has parser support for it.

[bjorn3]

Currently this doesn't succeed as the generated .lib files are seemingly not included in the final link.exe argument list: I'm not sure what I'm missing here: the llvm backend doesn't seem to be doing anything clever here to add it to the Session or something.

Rustc's linker code is supposed to automatically add it: https://github.com/rust-lang/rust/blob/62270fb4d674fa109148c3a2618e4db9e03cd91c/compiler/rustc_codegen_ssa/src/back/link.rs#L2174-L2185 (and for rlibs: https://github.com/rust-lang/rust/blob/62270fb4d674fa109148c3a2618e4db9e03cd91c/compiler/rustc_codegen_ssa/src/back/link.rs#L395-L409)

[simonbuchan]

Yeah, I'm not sure what's going on there - it's definitely getting called and emitting files (I've dbg!()ed the output path and copied to desktop to check it does actually write the file...).

My best guess is something is validating the file and filtering it out at some point, but nothing I could find after midnight 😅

[ChrisDenton]

This seems to actually work for me if using the LLVM linker (-C linker-flavor=lld-link).

The MSVC linker requires a bit more ceremony though:

= note: kernel32.dll.lib(kernel32.dll) : error LNK2001: unresolved external symbol __IMPORT_DESCRIPTOR_kernel32

As far as I'm aware the following symbols are required:

• __NULL_IMPORT_DESCRIPTOR (only one needed in a lib file)
• __IMPORT_DESCRIPTOR_{LIBRARY_NAME} (one per dll)
• \x7f{LIBRARY_NAME}_NULL_THUNK_DATA (one per dll)

I think you can ignore everything else (e.g. @compid just uniquely identifies the tool used to generate the lib file).

[ChrisDenton]

Do we need to be careful about lib name collision? I.e. should a random-ish id be added to the pathname to disambiguate?

[bjorn3]

I would expect rustc to merge all extern blocks with the same lib name. If that is indeed the case, there can't be any collisions as the tmpdir is unique per rustc invocation.

[ChrisDenton]

Fair enough then!

[simonbuchan]

So I've got a bit more progress!

Seems the sticking point for the imports being found was the default get_native_object_symbols was not recognizing the short import objects so it considered it an empty archive. I'm not sure why it would be working for @ChrisDenton with or without linker-flavor=lld-link - maybe something to do with the test code? I'm using https://github.com/microsoft/windows-rs/blob/master/crates/samples/windows-sys/enum_windows/src/main.rs

It was easy enough to provide a wrapper to get it working (in the sense of failing later at __IMPORT_DESCRIPTOR_...), but perhaps that logic should be upstreamed to rustc_codegen_ssa's impl.

I took a stab at using ar_archive_writer - it unfortunately doesn't currently let you get bit-compatible output with MSVC right now:

• msvc uses its own, second symbol table member also called / between the standard ar / symbol table member and the // long names member, which I couldn't see how you could write right now.
• msvc archive members have a -1 date and blank uid, gid members, but those are all u32s currently.
  Quite possible this doesn't matter, but I've left it using the dumb wordy code for now until it runs.

Seems the temp .lib won't actually appear on the final link commandline even when it's finding the exports.... I guess the relevant members get copied out or something? I'm not sure I'm following what rustc_codegen_ssa is actually doing in add_archive & friends. If so, getting bit-compatible with MSVC might be pretty pointless?

I'm part way through implementing __IMPORT_DESCRIPTOR_{dll_basename} - it looks roughly right but the linker doesn't find it and dumpbin barfs on reading it:

Archive member name at 5C6: kernel32.dll/
FFFFFFFF time/date
         uid
         gid
       0 mode
     11B size
correct header end

FILE HEADER VALUES
            8664 machine (x64)
               2 number of sections
               0 time date stamp
              E0 file pointer to symbol table
               3 number of symbols
               0 size of optional header
               0 characteristics
clif_kernel32.dll.lib : fatal error LNK1106: invalid file or disk full: cannot seek to 0x3233737D

I'm also missing object::write::coff, so for now I'm doing the same thing as with archive and short import objects going through the MS PE docs and dumpbin output for writing out the relevant fields, so it's probably just something dumb there. I can probably use the object::bytes_of on object::coff types to make it a bit easier to read the intent and make sure I don't miss a type size or something, but in practice this shouldn't be written this way anyway.

I also tossed in the __imp_{name} aliases in the symbol table: it's not initializing them in the GNU symbol table member yet, unfortunately.

I'll try to fix the remaining bits to get the end-to-end going, including the other symbols, hopefully tomorrow, but there's a lot of cleanup to make this mergeable still!

[ChrisDenton]

My test case was simpler, just an extern block with kind="raw-dylib. I'd guess it's the lack of indirection that made it work (i.e. it's not defined in a dependency).

[simonbuchan]

Probably gets rolled up into the target/*/deps/windows-sys*.rlib then.

I wanted to make sure I wasn't accidentally going to pull in an import from the stdlib, but I should be testing a few more combinations, especially no_std.

[ChrisDenton]

The object crate's repository has two utilities for printing the contents of archives (readobj and objdump) which may be useful. See also the test files (although the test cases are a bit weird).

It might be easier to create a simpler sample lib using LLVM's utility (llvm-lib). While the libs files it makes are not bit-for-bit the same as the msvc ones, they're still compatible with link.exe and somewhat simpler. E.g.:

llvm-lib /def:exports.def /OUT:imports.lib /machine:x64

Where "exports.def" is a def file containing what the DLL exports.

[simonbuchan]

More progress... but for some reason I've gotten it yelling at me that:

error[E0308]: mismatched types                                                                                                                                                                      
  --> src\archive.rs:69:9
   |
68 |     fn u16(value: u16) -> U16<LE> {
   |                           ------- expected `object::U16<object::LittleEndian>` because of return type
69 |         U16Bytes::new(LE, value)
   |         ^^^^^^^^^^^^^^^^^^^^^^^^ expected `U16<LittleEndian>`, found `U16Bytes<LittleEndian>`
   |
   = note: expected struct `object::U16<object::LittleEndian>`
              found struct `U16Bytes<object::LittleEndian>`

but only in this repo. If I pull the code out to a separate repo with an object dependency it's fine. Perhaps something to do with the unaligned feature? I can't trigger it.

To clarify, object::U16 should be an an alias for object::U16Bytes, and I get essentially the same errors for all the combinations I've tried.

[simonbuchan]

Of course, I figure it out as soon as I pushed the broken code :) (although I still can't reproduce it outside the repo, which I don't like)

It does crash on run though, which sounds like a problem for later. Probably just missing the thunk export.

[simonbuchan]

This:

• adds writer interfaces that help keep header data in sync for the various bits of code - not complete
• breaks out code to a new dll_import_lib module just for my sanity, if nothing else
• implements the remaining __NULL_IMPORT_DESCRIPTOR and \x7f{dll_basename}_NULL_THUNK_DATA symbols and objects,

Unfortunately, while this code creates .libs that actually link and execute correctly when run outside this repo, it currently crashes on build when included when accessing some object POD types using object::from_bytes_mut() (so you can patch up pointers, sizes, etc), which I believe is due to alignment checking.

If so, the hack fix would be to add the unaligned feature to object, which IIUC makes e.g. U16 and U16Bytes the same type, with alignment 1. I'm not sure if rustc_codegen_cranelift is deliberately avoiding enabling that feature though, and it feels a bit icky to depend on it (even if it's a default feature)

[simonbuchan]

Assuming all that is sorted out, I'd like to hear what your preference for where all the .lib writing code should go:

• Fine as is? (Seems a bit much for here)
• Temporarily leave it in tree while working on getting it upstreamed to object?
• Dump the windows-specific stuff and stick with ar_archive_writer at the top level?
• Pull it out to a new coff crate?
• etc.

Plenty of cleanup work otherwise, still.

[ChrisDenton]

I think at least writing the import object should definitely be in object crate. There's already a reader for ImportFile, having a writer version would be good too.

Using ar_archive_writer, at least for the initial implementation, is a good idea. It reduces the amount of new code necessary and it can produce libs that are the same as rustc's LLVM backend (which is the default). Making libs that have the msvc bits would be nice to have in the future but perhaps that should be a new function in ar_archive_writer or even a new crate. In any case, they aren't essential right now.

Still need to find a sec to catch up with the changes but generally I think the ideal would be to have as little here as necessary and outsource to crates that can be reused in other contexts.

[simonbuchan]

Not much today, just fixing the unaligned access by copy-pasting the object types, and fixing up lints.

I'll poke the object repo and ask about if/how they would like the relevant code upstreamed, it does seem the most appropriate place.

[simonbuchan]

Blagh. Of course there's already an object::write::coff. I should re-write on that :( Still have the short imports though.

[simonbuchan]

(That CI failure isn't something I did, right?)

[bjorn3]

I'm not sure why it would be working for @ChrisDenton with or without linker-flavor=lld-link - maybe something to do with the test code?

lld no longer looks at the archive symbol table as they found directly interning the symbol tables of the individual object files to avoid duplicate work and thus improve performance.

(That CI failure isn't something I did, right?)

No, testing rand on FreeBSD is broken due to it's libm rounding something slightly incorrect.

[simonbuchan]

Well replacing my ar code with ar_archive_writer worked fine, but I didn't have much luck using object::write::Object to replace my coff code.

I'll leave that as is for now, and continue the other fixes.

[simonbuchan]

Added ordinals and I name-types, though I can't figure a good way to test them in Rust code other than it looking right to the equivalent lib /def output, given Windows x64 is pretty boring undecorated name-only exports from what I've found, and you can't specify ordinal and name-type for Rust exports.

This should probably test against MSVC compiled .dlls, I guess? I can't quite figure the testing setup in this repo, it seems to be basically just run all the files in example?

[simonbuchan]

If the dll_import_lib::coff::Import* types seem redundant, they are: I'm trying to isolate the code in there so it can be moved out of tree easily, and for now it also makes it easy to develop the code in my IDE (IntelliJ's RustRover) which doesn't understand the rustc_* private crates.

[simonbuchan]

Took a peek at windows-gnu: it's somewhat different output:

coff file member {
  section 1 .text
    jmp __imp_{import_name}
    nop
    nop

  section 2 .idata$7
    dd _head_{dll_basename}_dll_a

  section 3 .idata$5
    dd .idata$6
    dd 0

  section 4 .idata$4
    dd .idata$6
    dd 0

  section 5 .idata$6
    dw 0
    db "{import_name}"

  symbols
    static .idata$6 => section 5
    external {import_name} => section 1
    external __imp_{import_name} => section 3
    external _head_{dll_basename}_dll_a => undef
}
; repeat above for each import

coff file member {
  section 1 .text:
    ; empty?

  section 2 .idata$2
    dd .idata$4
    dd 0
    dd 0
    dd __lib{dll_basename}_dll_a_iname
    dd .idata$5

  section 3 .idata$5
    ; empty ?

  section 4 .idata$4
    ; empty?

  symbols
    filename .file => debug [ fake ]
    static .idata$2 => section 2
    static .idata$4 => section 4
    static .idata$5 => section 3
    external _head_lib{dll_basename}_dll_a => section 2
    external __lib{dll_basename}_dll_a_iname => undef
}

coff file member {
  section 1 .text
    ; empty

  section 2 .idata$4
    dd 0
    dd 0

  section 3 .idata$5
    dd 0
    dd 0

  section 4 .idata$7
    db "{dll_filename}"

  symbols
    filename .file => debug
    external __lib{dll_basename}_dll_a_iname => section 4
}

; repeat above for each dll

I think it's definitely doable, but probably out of scope for this PR. Hopefully gnullvm is pretty close to it too.

[bjorn3]

Hopefully gnullvm is pretty close to it too.

I did expect it to match MSVC. LLVM doesn't have support for the format used by the GCC based MinGW.

[ChrisDenton]

windows-gnu is getting support for normal msvc/llvm import libraries once the bundled mingw tools are updated. Unfortunately updating isn't a trivial process so it might be a little while yet (hopefully before the end of the year tho).

In any case, this is a problem that will solve itself... eventually.

[simonbuchan]

Hmm - is it expected this should only work for x86_64-pc-windows-msvc right now? i686-* isn't supported by cranelift, -gnu is currently out of scope, and I'm not sure if aarch64-pc-windows-msvc is supposed to work right now: $env:TARGET_TRIPLE='aarch64-pc-windows-msvc'; .\y build panics compiling stdlib with a bunch of:

thread 'thread '<unnamed>' panicked at <unnamed>C:\Users\simon\.cargo\registry\src\index.crates.io-6f17d22bba15001f\cranelift-object-0.101.2\src\backend.rs' panicked at :C:\Users\simon\.cargo\registry\src\index.crates.io-6f17d22bba15001f\cranelift-object-0.101.2\src\backend.rs750::75022:22:
not implemented: Aarch64AdrGotPage21 is not supported for this file format:

but maybe I'm doing something dumb. Should I be able to either cross-compile the stdlib or fetch a prebuilt one?

Anyway, I think I'm closing in on this one: I want to do a sub-branch porting over to gimli-rs/object#595 which should clear out nearly all the coff file gunk in here if that lands before this; then it's pretty much just housekeeping stuff like checking for edge cases like 0xFFFF-ish numbers of imports; getting rid of a few unwrap()s and as casts. etc.

I would love to get some tests in here for this, but there doesn't seem to be much infrastructure at the moment for that (which I get, if you can't use cargo test). If you have any suggestions?

[bjorn3]

Arm windows isn't supported yet.

[bjorn3]

I would love to get some tests in here for this, but there doesn't seem to be much infrastructure at the moment for that (which I get, if you can't use cargo test). If you have any suggestions?

All tests are defined in build_system/tests.rs. Tests can be arbitrary code. Just prefer avoiding invocation of a C compiler. As test I think you can compile a rust crate as cdylib, remove the .lib file and then depend on it as raw-dylib from an executable. Or alternatively depend on some system library as raw-dylib.

[simonbuchan]

Huh, weird: when I try to patch object to https://github.com/philipc/object/tree/coff-writer (both a git dep and a path dep to a local clone) the std build fails at link with a bunch of duplicate and missing definitions.

[philipc]

Sorry about that, there was a bug with writing the string table and test coverage wasn't as good as I thought. I've forced pushed a fix to that branch, and checked that I can run the cg_clif tests under Windows.

[simonbuchan]

Would that cause a build error in building std in cranelift though? Eg ./y build.

My best guess was the custom build scripts were somehow pulling in multiple builds of object

[philipc]

Yeah, the fix I pushed was enough to get ./y.sh build working. I don't think it's anything to do with multiple builds of object, just that object was generating files with wrong symbol names.

[simonbuchan]

Huh. I guess windows-sys is in std already, but I didn't think that --cfg windows_raw_dylib would be set, so none of this code would be used in there? I've definitely screwed up writing .libs before and it's not broken until I used the generated cargo-clif.

[philipc]

The error was nothing to do with this PR or import libraries; it would happen if you used that object branch with the master branch here. object is already being used by cranelift-object to generate COFF files, and that would be used when compiling std.

[simonbuchan]

I added a simple build and run test to run the equivalent code to the enum-windows example only on x86_64-pc-windows-msvc. It's been decently reliable catching linking issues so far, but it will need to be addressed when other targets are supported.

Other than finishing the port onto the coff-writer branch of object (the example code by philipc is actually already most of it actually, so it's not much work), I think it's just catching any panics and lints?

[simonbuchan]

So gimli-rs/object#595 is merged but not published yet, so I don't want to update this branch to it just yet, but the updated branch is in a fairly decent shape now, and covers the remaining task list items: https://github.com/simonbuchan/rustc_codegen_cranelift/tree/raw_dylib_object_write.

Now's the time for bikeshedding I guess? I'm not super happy about the dll_import_lib module name for example: archive.rs should probably be promoted to archive/mod.rs and the current code that's essentially specific to -msvc flavor libs should go in archive/msvc.rs / archive/msvc/mod.rs and .../short_import.rs? In any case there's essentially no generic COFF code any more with object::write::coff available.

Finally, the history here is a huge mess now (which I guess was going to be pretty obvious!), would you prefer this was re-opened with a more sensible history? The real meat of this is only one logical commit, really, especially with the actual coff writing upstreamed. Of course, squash commit does the same job.

[philipc]

Suggested change

	if symbol.is_definition() {
	if !symbol.is_undefined() && symbol.is_global() {

I'm not completely sure this is what you need, but you definitely don't want is_definition.

[bjorn3]

@simonbuchan What is the status of this PR? Do you plan on continuing work on this? Do you want a review from me?

[simonbuchan]

@bjorn3 dang, I totally missed this comment, sorry!

I think I'm theory I was waiting for object to publish the COFF import writing code so I could land the branch I mentioned above but I pretty quickly forgot / got distracted 😬

I expect this is mostly redundant with rust-lang/ar_archive_writer#15 now?

The code would be quite different either way, so really this specific PR should close, but if it isn't conflicting with other people I'm happy to try picking this back up.

[bjorn3]

No worries. I've got a branch to use the import library writing support of ar_archive_writer at https://github.com/rust-lang/rustc_codegen_cranelift/tree/import_libs (and rust-lang/rust#128206 for deduplicating most of this code with cg_llvm), but it is still blocked on using __imp_ prefixed symbols for dllimport when necessary. (Or at least I think that is the reason I'm getting linker errors. I've confirmed that the generated import library is byte for byte identical to cg_llvm already.) If you want to work on implementing that, feel free to. I'm not currently working on it.

[bjorn3]

@dpaoliello seems to be working on implementing the rest of the necessary changes.

[simonbuchan]

Yep, was just trying to figure out how this was fitting together, and saw that PR on ar_archive_writer which seemed relevant.

[dpaoliello]

Yep, I have a prototype of raw-dylib working in cranelift using the new features in ar_archive_writer - I'll send out a draft PR soon, but I want to wait for rust-lang/rust#128206 to land first, since it will greatly clean up the implementation.