[PATCH] D144765: [llvm-lib] Add basic support for generating a Windows import library from a .def file.

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

When uploading patches for review, please include the additional context (e.g. `git diff -U999`) as suggested in the developer policy, https://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch.

We should try to make sure that if there's any other logic in lld that relates to import library creation, we should include that too - but I believe there's nothing really relevant there. There, we have `parseModuleDefs` which wraps `parseCOFFModuleDefinition`, and then copies the output into lld's own internal data structure. https://github.com/llvm/llvm-project/blob/llvmorg-17-init/lld/COFF/Driver.cpp#L981-L1038 There's some extra logic for forwarder exports in there, but that shouldn't affect the external import library, only how the DLL itself is generated. Then we have `createImportLibrary` which does the reverse data structure copy and calls `writeImportLibrary`, https://github.com/llvm/llvm-project/blob/llvmorg-17-init/lld/COFF/Driver.cpp#L924-L979. The main difference there is with extra logic for not touching an existing file which wouldn't be changed, which I guess isn't critical for llvm-lib at this point. So I think this implementation might be fine.



================
Comment at: llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp:335
+    if (LibMachine == COFF::IMAGE_FILE_MACHINE_UNKNOWN) {
+      llvm::errs() << "/DEF option requires /machine to be specified" << '\n';
+      return 1;
----------------
Lowercase `/def`


================
Comment at: llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp:359
+    std::string Path;
+    if (auto *Arg = Args.getLastArg(OPT_out)) {
+      Path = Arg->getValue();
----------------
For the existing main use case in `llvm-lib`, there's a different codepath that uses `getDefaultOutputPath(Members[0])`. I guess that's not very relevant here though. If it'd be easy to share code with it, that'd be nice, but I guess this is fine too.


================
Comment at: llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp:368
+    
+    return writeImportLibrary(Def->OutputFile, Path, Def->Exports, LibMachine, true)? 1:0;
+  }
----------------
The last bool option here is `bool MinGW`, and I don't think you should be setting it to true here. I think it'd be good for clarity to include the parameter name, e.g. like `/*MinGW=*/false` like we do in a number of places; I know the existing code in llvm-dlltool doesn't do that though.


================
Comment at: llvm/lib/ToolDrivers/llvm-lib/Options.td:25
 def out    : P<"out", "Path to file to write output">;
+def DEF    : P<"DEF", "DEF file to use to generate import library">;
 
----------------
I'd prefer if we'd stick to consistent casing with the existing ones here, i.e. lowercasing both occurrances of `DEF` here. Or is there an issue that the first one can't be called `def` since that's a tablegen keyword? In that case, leave a comment saying that, like for `list` just a few lines above. Or maybe rather make it e.g. `deffile` instead of `DEF` (with a comment explaining the reason). `deffile` seems to be what it's called in lld anyway.


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

https://reviews.llvm.org/D144765