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

[vadikp-intel]

Summary

'llvm-lib' cannot currently generate an import library from a Windows .def file, functionality supported by 'lib'. This incompatibility is currently breaking clang based Windows openmp builds. This revision adds basic support for this feature to llvm-lib by cloning the corresponding code from dlltool.

[vadikp-intel]

white space

[mstorsjo]

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.

llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp

335

Lowercase /def

359

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.

368

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.

llvm/lib/ToolDrivers/llvm-lib/Options.td

25

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.

[mstorsjo]

Oh, I totally forgot - this needs a testcase too.

[vadikp-intel]

llvm/lib/ToolDrivers/llvm-lib/Options.td

25

'def' is a reserved word, so have to use 'DEF' here (the end result is more or less the same). I've changed it to be lowercase in the help string. 'deffile' would not be lib compatible and also complicate things in CMAKE which unconditionally defines CMAKE_LINK_DEF_FILE_FLAG to '/DEF:', I believe.

[vadikp-intel]

Addressed review feedback.
Added a test for creating an import library from a def file.

[mstorsjo]

llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp

434

This change breaks virtually all existing tests for llvm-lib.

llvm/lib/ToolDrivers/llvm-lib/Options.td

25

I quite obviously didn't ask you to change the name of the option exposed to the actual user to deffile, that would obviously break the indended compatibility.

I asked you to change def DEF into def deffile, to keep it consistent as lowercase, consistent with the other options. That would also be consistent with the same option in lld, see https://github.com/llvm/llvm-project/blob/llvmorg-17-init/lld/COFF/Options.td#L130-L132.

llvm/test/tools/llvm-lib/implibs.test

16 ↗

(On Diff #501028)

Having the library named MyFunc.dll looks a bit weird here. Can you pick a name different from the function itself? The existing testcase above uses the name lib.dll.

19 ↗

(On Diff #501028)

This FileCheck with no --check-prefix would inspect all CHECK lines in the file, so it would try to match both the CHECK: lib.dll further above on line 12, and the newly added one. But when running this test, we don't even get that far, because another change broke this test so that it fails already on line 9.

If this can use the existing CHECK line, then just keep it as such, or alternatively use a different prefix, e.g. DEFIMPLIB: othername.dll and check it with FileCheck --check-prefix=DEFIMPLIB.

In any case, when adding a testcase in a patch, I would have assumed that you would have tested running it.

[vadikp-intel]

Fixed test and output file name logic.

[vadikp-intel]

addressed remaining review comments

[mstorsjo]

LGTM, thanks for addressing the feedback!

[hans]

lgtm, nice!

This might be worth adding to llvm/docs/ReleaseNotes.rst too.

[vadikp-intel]

merge test

llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp

 #include "llvm/Bitcode/BitcodeReader.h"
 #include "llvm/Object/ArchiveWriter.h"
 #include "llvm/Object/COFF.h"
+#include "llvm/Object/COFFModuleDefinition.h"
 #include "llvm/Object/WindowsMachineFlag.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"

 #include <optional>
 
 using namespace llvm;
+using namespace llvm::object;
 
 namespace {
 

 public:
   LibOptTable() : opt::GenericOptTable(InfoTable, true) {}
 };
-}
+} 
 
 static std::string getDefaultOutputPath(const NewArchiveMember &FirstMember) {
   SmallString<128> Val = StringRef(FirstMember.Buf->getBufferIdentifier());

   return Ret;
 }
 
+// Opens a file. Path has to be resolved already. (used for def file)
+std::unique_ptr<MemoryBuffer> openFile(const Twine &Path) {
+  ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MB = MemoryBuffer::getFile(Path);
+
+  if (std::error_code EC = MB.getError()) {
+    llvm::errs() << "cannot open file " << Path << ": " << EC.message() << "\n";
+    return nullptr;
+  }
+
+  return std::move(*MB);
+}
+
 static std::string findInputFile(StringRef File, ArrayRef<StringRef> Paths) {
   for (StringRef Dir : Paths) {
     SmallString<128> Path = Dir;

   for (auto *Arg : Args.filtered(OPT_ignore))
     IgnoredWarnings.insert(Arg->getValue());
 
+  COFF::MachineTypes LibMachine = COFF::IMAGE_FILE_MACHINE_UNKNOWN;
+  std::string LibMachineSource;
+  if (auto *Arg = Args.getLastArg(OPT_machine)) {
+    LibMachine = getMachineType(Arg->getValue());
+    if (LibMachine == COFF::IMAGE_FILE_MACHINE_UNKNOWN) {
+      llvm::errs() << "unknown /machine: arg " << Arg->getValue() << '\n';
+      return 1;
+    }
+    LibMachineSource =
+        std::string(" (from '/machine:") + Arg->getValue() + "' flag)";
+  }
+
+  // create an import library
+  if (Args.hasArg(OPT_DEF)) {
+
+    if (LibMachine == COFF::IMAGE_FILE_MACHINE_UNKNOWN) {
+      llvm::errs() << "/DEF option requires /machine to be specified" << '\n';
+      return 1;
+    }
+
+    std::unique_ptr<MemoryBuffer> MB =
+        openFile(Args.getLastArg(OPT_DEF)->getValue());
+    if (!MB)
+      return 1;
+
+    if (!MB->getBufferSize()) {
+      llvm::errs() << "definition file empty\n";
+      return 1;
+    }
+
+    Expected<COFFModuleDefinition> Def =
+        parseCOFFModuleDefinition(*MB, LibMachine, true);
+
+    if (!Def) {
+      llvm::errs() << "error parsing definition\n"
+                   << errorToErrorCode(Def.takeError()).message();
+      return 1;
+    }
+
+    std::string Path;
+    if (auto *Arg = Args.getLastArg(OPT_out)) {
+      Path = Arg->getValue();
+    }
+    
+    if (Path.empty()) {
+      llvm::errs() << "no output path given\n";
+      return 1;
+    }
+    
+    return writeImportLibrary(Def->OutputFile, Path, Def->Exports, LibMachine, true)? 1:0;
+  }
+
   // If no input files and not told otherwise, silently do nothing to match
   // lib.exe
   if (!Args.hasArgNoClaim(OPT_INPUT) && !Args.hasArg(OPT_llvmlibempty)) {

 
   std::vector<StringRef> SearchPaths = getSearchPaths(&Args, Saver);
 
-  COFF::MachineTypes LibMachine = COFF::IMAGE_FILE_MACHINE_UNKNOWN;
-  std::string LibMachineSource;
-  if (auto *Arg = Args.getLastArg(OPT_machine)) {
-    LibMachine = getMachineType(Arg->getValue());
-    if (LibMachine == COFF::IMAGE_FILE_MACHINE_UNKNOWN) {
-      llvm::errs() << "unknown /machine: arg " << Arg->getValue() << '\n';
-      return 1;
-    }
-    LibMachineSource =
-        std::string(" (from '/machine:") + Arg->getValue() + "' flag)";
-  }
-
   std::vector<std::unique_ptr<MemoryBuffer>> MBs;
   StringSet<> Seen;
   std::vector<NewArchiveMember> Members;

llvm/lib/ToolDrivers/llvm-lib/Options.td

 // Can't be called "list" since that's a keyword.
 def lst    : F<"list">, HelpText<"List contents of .lib file on stdout">;
 def out    : P<"out", "Path to file to write output">;
+def DEF    : P<"DEF", "DEF file to use to generate import library">;
 
 def llvmlibthin : F<"llvmlibthin">,
     HelpText<"Make .lib point to .obj files instead of copying their contents">;