search

rust-lang / rustc_codegen_cranelift

Cranelift based backend for rustc
Apache License 2.0
1.52k stars 94 forks source link

WIP raw_dylib: write the import .lib manually. #1414

Open simonbuchan opened 8 months ago

simonbuchan commented 8 months ago

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

bjorn3 commented 8 months ago

Thanks for working on this!

bjorn3 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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:

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

simonbuchan commented 8 months ago

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:

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 7 months ago

This:

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 commented 7 months ago

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

Plenty of cleanup work otherwise, still.

ChrisDenton commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

simonbuchan commented 7 months ago

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

bjorn3 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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?

bjorn3 commented 7 months ago

Arm windows isn't supported yet.

bjorn3 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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.

bjorn3 commented 2 months ago

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