[PATCH] D104530: [LLD] [COFF] Support linking directly against DLLs in MinGW mode

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: lld/COFF/InputFiles.cpp:1164
+    bool code = false;
+    for (size_t i = 1, e = coffObj->getNumberOfSections(); i <= e; i++) {
+      const coff_section *sec = CHECK(coffObj->getSection(i), this);
----------------
rnk wrote:
> Factoring out the computation of this boolean would help readability. Initially I thought "this should be a data structure", but there are so few PE sections that it really doesn't matter.
Yep; this does end up with something like O(n*m) efficiency too, but as the number of sections is only half a dozen normally, it's probably not a biggie.

I can split it out to a separate function.


================
Comment at: lld/COFF/InputFiles.cpp:1175-1178
+    s->dllName = dllName;
+    s->symbolName = symbolName;
+    s->importType = code ? ImportType::IMPORT_CODE : ImportType::IMPORT_DATA;
+    s->nameType = ImportNameType::IMPORT_NAME;
----------------
rnk wrote:
> Instead of copying these strings into a newly allocated data structure, is there some way to put the export directory index into the LazyDLLSymbol? Most of the fields of DLLFile::Symbol seem like they can be calculated later after symbol resolution. This would avoid copying all the exported strings once.
I iterated a bit back and forth on this while making the patch...

In practice, we actually do need most of the fields from the get go; we do need to know whether to insert the `__imp_`-less symbol into the symbol table (so we need to find the `bool code` for each symbol). Technically we don't need to store the original `symbolName` (for a data import), but once we synthesize the import struct we do need the original `symbolName` again (and I'd prefer to avoid working with heuristics for stripping out `__imp_` from the symbol name when we can avoid it). The `dllName` isn't needed until the end, but a `StringRef` for it is pretty much as cheap as a reference to the COFFObjectFile for fetching it later, assuming the operation for fetching it isn't too costly.

Secondly, `ExportDirectoryEntryRef` is kinda opaque - it's a bit large in itself and it doesn't expose the index publicly. I did experiment with storing the whole `ExportDirectoryEntryRef` in `LazyDLLSymbol`, but I needed to have the `code` flag separately anyway, and then it exceeded the size for the symbol union.

So in practice, it feels a bit excessive to fetch all the info originally at first, but there's surprisingly little to shave off...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104530/new/

https://reviews.llvm.org/D104530