Upstreaming some archive / coff short import writing code?

[simonbuchan]

Howdy!

I've got some code for writing Windows dll import libraries in a PR over here: rust-lang/rustc_codegen_cranelift#1414 - some of it seems like it would be useful to upstream here, with some work to improve the UX, testing, and polishing.

In particular, it has:

• GNU-flavor archive writing with some pretty raw code for both GNU and MSVC symbol tables.
  • I think it's probably in spitting distance of being byte-for-byte with MSVC lib.exe?
  • I've taken a look at the other flavors, and they don't seem too bad to add support for?
  • UX is something of a concern to get right, but I think I can make it a bit nicer than ar or ar_archive_writer's current public API.
  • Using ar_archive_writer might be a better path, given the much more heavily tested code.
• Pointless reproduction of the write::coff code that I missed the existence of 😅 - though maybe there's still something salvageable there
• Short import objects, even though they are pretty straightforward.

And even the overall goal of writing DLL import libs seems like it might be in scope?

I'd be happy to work to get these up to your standard if you're interested.

[philipc]

Unless @bjorn3 wants ar_archive_writer to be replaced by code in object, I think it's better add the archive writing there since it already exists. And if so, I don't think DLL import library support belongs in object, since we don't want to add a dependency on ar_archive_writer.

write::coff isn't directly public code, but you can still access it via write::Object, and from a quick look it should be suitable for what you need, and I don't expect any significant changes to be needed there.

Short import object write support would be fine, since we have a parser for it already.

[simonbuchan]

Sounds good. I've got plenty of stuff left in that PR to go, but I can look at getting a short import writing PR opened here in the meantime, which I assume should basically try to look like Object write?

[philipc]

So the code for the short import is just this:
https://github.com/rust-lang/rustc_codegen_cranelift/blob/a38d47146c0536f7526ded3b138afc945f160891/src/dll_import_lib/coff.rs#L92-L111

pub(crate) fn write_short_import(
    data: &mut DataWriter,
    dll_name: &str,
    name: &&str,
    ordinal_or_hint: Option<std::primitive::u16>,
) {
    data.write_pod(&ImportObjectHeaderUnaligned {
        sig1: u16(IMAGE_FILE_MACHINE_UNKNOWN),
        sig2: u16(IMPORT_OBJECT_HDR_SIG2),
        version: u16(0),
        machine: u16(IMAGE_FILE_MACHINE_AMD64),
        time_date_stamp: u32(0),
        size_of_data: u32((name.len() + 1 + dll_name.len() + 1) as u32),
        ordinal_or_hint: u16(ordinal_or_hint.unwrap_or_default()),
        name_type: u16(IMPORT_OBJECT_CODE << IMPORT_OBJECT_TYPE_SHIFT
            | IMPORT_OBJECT_NAME << IMPORT_OBJECT_NAME_SHIFT),
    });
    data.write_c_str(name);
    data.write_c_str(dll_name);
}

I'm actually not sure how useful it is really going to be to add a small function like this to object. You could add a function in object::write::coff that does that, but then you would need to use the data writing abstractions provided by object instead of your DataWriter, which may not fit well with the rest of your code. Unless you have a strong opinion on what this should look like, can I suggest delaying this until you've finished the rest of your PR, and reevaluate the usefulness then.

[simonbuchan]

I was thinking mostly for consistency wrt the machine name type etc., moving to write::Object, but yeah, putting it onto Object in some way would probably make sense - maybe the public API is just .allow_short_import(bool) or something.

DataWriter will be pretty vestigial after replacing my ar and coff file writing, so I have no problem dumping it for this repo, I already assumed it would be, and I've already dumped a bunch of code in that PR. (I have a habit of writing a bunch of code then cleaning up when figuring out a new thing)

If I feel particularly sad about all the code in that PR I can always make my own crate (with classy ladies and Uno) 😊

[simonbuchan]

(Possibly should be a new issue, but I'm still happy to PR any changes you think are appropriate to get this working with object)

Hmm. Took a stab at using write::Object but I don't think I can implement write_import_descriptor() with that?

dumpbin shows the following symbols for the valid member:

COFF SYMBOL TABLE
000 00000000 SECT1  notype       External     | __IMPORT_DESCRIPTOR_user32
001 00000000 SECT1  notype       Section      | .idata$2
002 00000000 SECT2  notype       Static       | .idata$6
003 00000000 UNDEF  notype       Section      | .idata$4
004 00000000 UNDEF  notype       Section      | .idata$5
005 00000000 UNDEF  notype       External     | __NULL_IMPORT_DESCRIPTOR
006 00000000 UNDEF  notype       External     | user32_NULL_THUNK_DATA

Which if I dump it using read::coff::CoffFile looks like:

  - Ok("__IMPORT_DESCRIPTOR_user32"): Data => Section(SectionIndex(1))
  - Ok(".idata$2"): Section => Section(SectionIndex(1))
  - Ok(".idata$6"): Data => Section(SectionIndex(2))
  - Ok(".idata$4"): Section => Common
  - Ok(".idata$5"): Section => Common
  - Ok("__NULL_IMPORT_DESCRIPTOR"): Data => Undefined
  - Ok("\u{7f}user32_NULL_THUNK_DATA"): Data => Undefined

But Object::add_symbol() doesn't let me emit with SymbolKind::Section and SymbolSection::Common for .idata$4 and .idata$5 (it assumes SymbolSection::Section()).

write_null_thunk_data creates a separate object with those sections:

  sections:
  - .idata$5: 8 bytes
    0 relocations:
  - .idata$4: 8 bytes
    0 relocations:
  symbols:
  - Ok("\u{7f}user32_NULL_THUNK_DATA"): Data => Section(SectionIndex(1))

Not sure why that's what MSVC does, but I blindly copied the structure, and it works. so 🤷

(To clarify, this is so the linker will build a valid import directory entry for the DLL, using the $n suffixes to order, and relocations to link up the imports)

I tried a few different approaches, but nothing seems to successfully link: using values like Data / Undefined the linker complains about missing .idata$4/5 symbols, as you might expect, or Object::write() panics etc.

I tried inlining the section definitions:

  sections:
  - .idata$2: 20 bytes
    3 relocations:
    - 0: Relocation { kind: ImageOffset, encoding: Generic, size: 32, target: Symbol(SymbolIndex(0)), addend: 0, implicit_addend: true }
    - 12: Relocation { kind: ImageOffset, encoding: Generic, size: 32, target: Symbol(SymbolIndex(4)), addend: 0, implicit_addend: true }
    - 16: Relocation { kind: ImageOffset, encoding: Generic, size: 32, target: Symbol(SymbolIndex(2)), addend: 0, implicit_addend: true }
  - .idata$4: 8 bytes
    0 relocations:
  - .idata$5: 8 bytes
    0 relocations:
  - .idata$6: 11 bytes
    0 relocations:
  symbols:
  - Ok(".idata$4"): Section => Section(SectionIndex(2))
  - Ok(".idata$5"): Section => Section(SectionIndex(3))
  - Ok(".idata$6"): Section => Section(SectionIndex(4))
  - Ok("__IMPORT_DESCRIPTOR_user32"): Data => Section(SectionIndex(1))
  - Ok("__NULL_IMPORT_DESCRIPTOR"): Data => Undefined
  - Ok("\u{7f}user32_NULL_THUNK_DATA"): Data => Section(SectionIndex(3))

but that just crashes on link and dumpbin shows an empty import section for user32.dll 😔

Am I missing something?

[philipc]

Yeah that looks like a combination of fields that we don't support yet. Don't expect read::coff::CoffFile to give meaningful results for this. Do you know of any documentation that describes these symbols?

[philipc]

One option may be to add a SymbolFlags::Coff variant to let you override these fields (at least storage_class and maybe typ too), similar to what is already possible for other file formats.

[philipc]

The best documentation I could find is just the idata description at https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#the-idata-section.

I'm now thinking that write::Object is too complicated for what this needs to do, and maybe we should add write::coff::Writer (similar to write::elf::Writer).

This would also be a good place to add higher level helpers for creating the complete objects, as well as the short imports (so the equivalent of things like your coff::write_import_descriptor and coff::write_short_import).

If you want, I can do the work to create write::coff::Writer, and let you do the higher level helpers.

[simonbuchan]

Yeah, theres' some GNU code out there, etc., but that seems to be only only official documentation for linkers. I've been mostly just trying to match what MSVC does.

I'm not sure that write_import_descriptor by itself makes sense as a public API - not without better experience figuring out what MSVC is even doing there at least. The combined import descriptor / null thunk entries together should work as a "import directory entry" but I'm not sure exactly why it's not a single COFF file already: probably something to do with merging multiple import libraries for the same DLL? I'm more confident about a higher-level "write import library" / "add imports to an archive" type API, but that seems like it's probably out of scope for this crate.

[philipc]

Which if I dump it using read::coff::CoffFile looks like:

• Ok(".idata$4"): Section => Common
• Ok(".idata$5"): Section => Common

These are a bug; they aren't actually common symbols. Instead, they are section symbols for sections that are defined in another member of the archive. #592 improves the read side of this.

write::Object still isn't going to handle these correctly, but we could easily add support for the combination of SymbolKind::Section + SymbolSection::Undefined to handle .idata$4 and .idata$5 (so both of those would be IMAGE_SYM_CLASS_SECTION).

However, I don't know why .idata$2 uses IMAGE_SYM_CLASS_SECTION and .idata$6 uses IMAGE_SYM_CLASS_STATIC, or what values the API should use to let the user choose between these. Are you able to test if this matters? e.g. is it okay to change one or the other to be the same?

[simonbuchan]

Yeah, I was planning to poke around to see what works before defining a public API. I'm particularly curious why MSVC has the null thunks and null directory entry in a separate member: my understanding was that linkers operate on the section level so I can't figure out why they would have it be that complicated, $4 and $5 seem like they should be fine to be defined in the same never. Something about ordering or merging, perhaps? (But there's no COMDATs...)

[simonbuchan]

So: $6 being SECTION instead of STATIC makes the executable fail on load with: exit code: 0xc0000135, STATUS_DLL_NOT_FOUND which is interesting, the DLL name gets completely dropped from dumpbin /exports on the executable:

File Type: EXECUTABLE IMAGE

  Section contains the following imports:

<-- No DLL name here! -->
             14001E2B0 Import Address Table
             140027C50 Import Name Table
                     0 time date stamp
                     0 Index of first forwarder reference

                           0 GetWindowTextA
                           1 EnumWindows

    KERNEL32.dll
             14001E000 Import Address Table
             1400279A0 Import Name Table
                     0 time date stamp
                     0 Index of first forwarder reference

                          89 CloseHandle
                         4B8 ReleaseSRWLockExclusive
                         4B6 ReleaseMutex
                         4B9 ReleaseSRWLockShared
                         26A GetLastError

$6 is the DLL name, so that makes some sense... but not why it would just disappear when the others don't (everything is still in the emitted .lib and parses fine, to be clear)

On the other hand, changing $2 to STATIC seems to work fine.

The docs are shockingly worthless - less than worthless even - here:

Constant	Value	Description/interpretation of the Value field
IMAGE_SYM_CLASS_STATIC	3	The offset of the symbol within the section. If the Value field is zero, then the symbol represents a section name.
IMAGE_SYM_CLASS_SECTION	104	A definition of a section (Microsoft tools use STATIC storage class instead).

🤦‍♂️

Interestingly, moving the thunk sections $4 and $5 into the main directory member and removing the UNDEF references work fine ... but only when you drop the EXTERNAL (I guess the directory entry is magic to support both a SECTION/STATIC symbol and an EXTERNAL symbol?). No such luck on the NULL_DIRECTORY_ENTRY.

Presumably this is because this is the only library defining imports for the dll_name, but not the only import library, so the linker can't only pick the one.

That sort of answers why there needs to be three members too: so that they can be referenced by EXTERNAL symbols and duplicates null thunk members or null directory entries can be dropped when merging libraries?

[simonbuchan]

Now for the fun bit downstream of figuring out all this again, but for windows-gnu target 😅

[philipc]

I think each DLL has its own null thunk (shouldn't ever be duplicates), but there is only one null directory (duplicates are dropped). So it makes sense that there might be a reason why it needs to be separate, but I still don't know why exactly.

[philipc]

I've started implementing write::coff::Writer and I think it'll be useful for this. Might be a few days though.

[simonbuchan]

Thanks, yeah!

Sorry I wasn't clearer about your earlier suggestion to do so: I don't want to step on any toes if you have a preferred API.

I've been focusing on figuring out what else needs to be done to get my PR over the line, and while it's a bit ugly, the current code doesn't block that.

I'm now a lot more confident about what the import library API using this would ideally look like though: if you have an API surface you expect to implement I can get that up in it's own repo (and maybe crate?) so you can end to end of you like.

[philipc]

The code I'm writing won't know anything about import libraries. I'll modify write::Object to use it, so that'll be a decent amount of test coverage. I'll also experiment with converting bits of your cf_clif PR to ensure that API is usable for you. I think there's still room for an import library API to be layered above this, but that doesn't need to be done yet.

[philipc]

#595 adds a COFF writer, and I've partially converted your PR in philipc/rustc_codegen_cranelift@a8e5804 (untested). The main difference from your writer is that it supports streaming the output, so it needs to reserve file ranges up front instead of mutating headers later. Review is welcome.

[simonbuchan]

Cool, I'll take a look this weekend.

[simonbuchan]

So I've got a branch rebased completely onto the merged code in #595 now: https://github.com/simonbuchan/rustc_codegen_cranelift/tree/raw_dylib_object_write

I think something like the write_short_import related code in there makes sense to upstream, but it should probably be both read + write, more object::Object::write than object::coff::write, if that makes sense.

I've got a very little bit of reading it back in get_symbols, just for reference.

In theory a short import is just shorthand for a coff file with a symbol export mapped to an .idata import table thunk, so it should be representable as an object::Object even, but it's a bit of an odd case.

[philipc]

it should probably be both read + write, more object::Object::write than object::coff::write, if that makes sense.

Heh, nah I'm missing something there. How is that code related to read or object::Object? I can understand it being separate from object::write::coff::Writer, but I would still expect it to be in object::write::coff. And if we include write_short_import, I think we should include the rest of the file as well, since it's not really useful by itself.

I've got a very little bit of reading it back in get_symbols, just for reference.

Ideally you should be able to use object::read::coff::ImportFile for that.

This doesn't use object::pe::ImportObjectHeader as that asserts 4-byte alignment

That sounds like something we should fix. Technically it's a breaking change though...

type NtCoffFile<'data> = object::read::coff::CoffFile<'data, &'data [u8], object::pe::ImageFileHeader>;

You should be able to use CoffFile<'data> instead, since the other two type parameters have defaults.

In theory a short import is just shorthand for a coff file with a symbol export mapped to an .idata import table thunk, so it should be representable as an object::Object even, but it's a bit of an odd case.

Yeah that makes sense. We can consider doing that if it's useful.

[simonbuchan]

it should probably be both read + write, more object::Object::write than object::coff::write, if that makes sense.

Heh, nah I'm missing something there. How is that code related to read or object::Object? I can understand it being separate from object::write::coff::Writer, but I would still expect it to be in object::write::coff. And if we include write_short_import, I think we should include the rest of the file as well, since it's not really useful by itself.

I simply mean in terms of being a structure that you can read and write, rather than the low level read/write. But that would probably just be the impl Object stuff I referred to at the end.

I've got a very little bit of reading it back in get_symbols, just for reference.

Ideally you should be able to use object::read::coff::ImportFile for that.

Oh, is that for short imports? Damn.

This doesn't use object::pe::ImportObjectHeader as that asserts 4-byte alignment

That sounds like something we should fix. Technically it's a breaking change though...

Yeah. Honestly the whole unaligned feature is a bit confusing to me: I can see optimizing aligned access, but ideally that would be something you could pick at use-site (eg read and read_unaligned?)

type NtCoffFile<'data> = object::read::coff::CoffFile<'data, &'data [u8], object::pe::ImageFileHeader>;

You should be able to use CoffFile<'data> instead, since the other two type parameters have defaults.

That's what I thought, but it was yelling at me. I might have just been screwing it up though? Probably just missing the lifetime parameter and misreading the error.

[philipc]

I simply mean in terms of being a structure that you can read and write, rather than the low level read/write. But that would probably just be the impl Object stuff I referred to at the end.

Ah I see. So basically add write capability to read::coff::ImportFile, except that I don't think that is a suitable structure for writing. I think adding something new in write::coff is better.

Honestly the whole unaligned feature is a bit confusing to me: I can see optimizing aligned access, but ideally that would be something you could pick at use-site (eg read and read_unaligned?)

We're not copying anything, just pointing at it. Alignment is a property of the type and rust doesn't allow unaligned pointers. So the only way I can see to allow you to pick at use-site is to use different types, and I don't want that.

The goal was that you shouldn't ever need to enable the unaligned feature for normal files, because we should have set the alignment of the types to whatever is used in practice. The unaligned feature is an escape hatch for unusual cases (e.g. the files are embedded in something else that isn't aligned).

In hindsight, I'm not sure that the performance benefit is worth the complexity cost. For now, you can enable the unaligned feature to avoid the errors. It's what would happen anyway if I removed the feature.

[simonbuchan]

I see! You could have the unaligned read method return a copy, without the ref it would be automatically aligned. Probably more expensive and uglier to use than unconditionally using unaligned access.

The bigger issue with aligned access is the data is probably being read from a Vec<u8>, which you can't technically guarantee the alignment of (but it's probably going to be at least usize alignment, which should be enough)

In my case, I didn't feel it appropriate to add the unaligned feature to rustc_backend_cranelift in my first PR, especially since it seems like they deliberately disabled it, but who knows 🤷

[bjorn3]

The unaligned feature is not intentionally disabled. Wasm support is though. Note that ar_archive_writer already enables the unaligned feature anyway.

[dpaoliello]

FYI, ar_archive_writer can now create import libraries: rust-lang/ar_archive_writer#15

[philipc]

In that case I expect there's nothing more to be done here.