rustc_codegen_cranelift icon indicating copy to clipboard operation
rustc_codegen_cranelift copied to clipboard

WIP raw_dylib: write the import .lib manually.

Open simonbuchan opened this issue 1 year ago • 35 comments

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):

  • [x] Get her running!
  • [x] Replace archive writing code with ar_archive_writer
  • [ ] Replace coff file writing code with object::write - see https://github.com/gimli-rs/object/issues/591#issuecomment-1805151103
  • [x] Implement ordinal / hint support
  • [x] Check / implement #[link(import_name_type)
  • [x] ~~See what's needed for windows-gnu target - it might be in scope?~~ looks out of scope: https://github.com/rust-lang/rustc_codegen_cranelift/pull/1414#issuecomment-1809861824
  • [x] ~~Implement machine types~~ seems like only x86_64 is supported for windows in cranelift right now?
  • [x] 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.
  • [x] Add tests?

simonbuchan avatar Nov 02 '23 11:11 simonbuchan

Thanks for working on this!

bjorn3 avatar Nov 02 '23 14:11 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)

bjorn3 avatar Nov 02 '23 14:11 bjorn3

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 😅

simonbuchan avatar Nov 02 '23 19:11 simonbuchan

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 avatar Nov 03 '23 07:11 ChrisDenton

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!

simonbuchan avatar Nov 04 '23 12:11 simonbuchan

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

ChrisDenton avatar Nov 04 '23 13:11 ChrisDenton

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.

simonbuchan avatar Nov 04 '23 13:11 simonbuchan

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.

ChrisDenton avatar Nov 04 '23 13:11 ChrisDenton

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 avatar Nov 05 '23 07:11 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 avatar Nov 05 '23 07:11 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 avatar Nov 07 '23 12:11 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.

simonbuchan avatar Nov 07 '23 12:11 simonbuchan

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.

ChrisDenton avatar Nov 07 '23 14:11 ChrisDenton

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 avatar Nov 08 '23 11:11 simonbuchan

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

simonbuchan avatar Nov 08 '23 11:11 simonbuchan

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

simonbuchan avatar Nov 08 '23 11:11 simonbuchan

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.

bjorn3 avatar Nov 08 '23 11:11 bjorn3

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 avatar Nov 09 '23 10:11 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 avatar Nov 11 '23 01:11 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 avatar Nov 11 '23 01:11 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.

simonbuchan avatar Nov 14 '23 09:11 simonbuchan

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.

bjorn3 avatar Nov 14 '23 11:11 bjorn3

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.

ChrisDenton avatar Nov 14 '23 19:11 ChrisDenton

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 https://github.com/gimli-rs/object/pull/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?

simonbuchan avatar Nov 20 '23 12:11 simonbuchan

Arm windows isn't supported yet.

bjorn3 avatar Nov 20 '23 13:11 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.

bjorn3 avatar Nov 20 '23 13:11 bjorn3

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.

simonbuchan avatar Nov 20 '23 13:11 simonbuchan

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.

philipc avatar Nov 21 '23 02:11 philipc

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

simonbuchan avatar Nov 21 '23 02:11 simonbuchan

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.

philipc avatar Nov 21 '23 03:11 philipc