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.