[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

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

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/ToolDrivers/llvm-lib/LibDriver.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/BinaryFormat/COFF.h"
 #include "llvm/BinaryFormat/Magic.h"
 #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 "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
 #include "llvm/Support/raw_ostream.h"
 #include <optional>
 
 using namespace llvm;
+using namespace llvm::object;
 
 namespace {
 
 enum {
   OPT_INVALID = 0,
 #define OPTION(_1, _2, ID, _4, _5, _6, _7, _8, _9, _10, _11, _12) OPT_##ID,
 #include "Options.inc"
 #undef OPTION

 #include "Options.inc"
 #undef OPTION
 };
 
 class LibOptTable : public opt::GenericOptTable {
 public:
   LibOptTable() : opt::GenericOptTable(InfoTable, true) {}
 };
-}
+} // namespace
 
 static std::string getDefaultOutputPath(const NewArchiveMember &FirstMember) {
   SmallString<128> Val = StringRef(FirstMember.Buf->getBufferIdentifier());
   sys::path::replace_extension(Val, ".lib");
   return std::string(Val.str());
 }
 
 static std::vector<StringRef> getSearchPaths(opt::InputArgList *Args,
static std::vector<StringRef> getSearchPaths(opt::InputArgList *Args,
   while (!Env.empty()) {
     StringRef Path;
     std::tie(Path, Env) = Env.split(';');
     Ret.push_back(Path);
   }
   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;
     sys::path::append(Path, File);
     if (sys::fs::exists(Path))
       return std::string(Path);
   }
   return "";
if (Args.hasArg(OPT_help)) {
     return 0;
   }
 
   // Parse /ignore:
   llvm::StringSet<> IgnoredWarnings;
   for (auto *Arg : Args.filtered(OPT_ignore))
     IgnoredWarnings.insert(Arg->getValue());
 
+  // get output library path, if any
+  std::string OutputPath;
+  if (auto *Arg = Args.getLastArg(OPT_out)) {
+    OutputPath = 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 (OutputPath.empty()) {
+      llvm::errs() << "no output path given\n";
+      return 1;
+    }
+
+    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;
+    }
+
+    return writeImportLibrary(Def->OutputFile, OutputPath, Def->Exports, LibMachine,
+                              /*MinGW=*/false)
+               ? 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)) {
     if (!IgnoredWarnings.contains("emptyoutput")) {
       llvm::errs() << "warning: no input files, not writing output file\n";
       llvm::errs() << "         pass /llvmlibempty to write empty .lib file,\n";
       llvm::errs() << "         pass /ignore:emptyoutput to suppress warning\n";
       if (Args.hasFlag(OPT_WX, OPT_WX_no, false)) {
         llvm::errs() << "treating warning as error due to /WX\n";
         return 1;
       }
     }
     return 0;
   }
 
   if (Args.hasArg(OPT_lst)) {
     doList(Args);
     return 0;
   }
 
   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;
 
   // Create a NewArchiveMember for each input file.
   for (auto *Arg : Args.filtered(OPT_INPUT)) {
     // Find a file
     std::string Path = findInputFile(Arg->getValue(), SearchPaths);
for (auto *Arg : Args.filtered(OPT_INPUT)) {
     // Append a file.
     appendFile(Members, LibMachine, LibMachineSource, MBRef);
 
     // Take the ownership of the file buffer to keep the file open.
     MBs.push_back(std::move(*MOrErr));
   }
 
   // Create an archive file.
-  std::string OutputPath;
-  if (auto *Arg = Args.getLastArg(OPT_out)) {
-    OutputPath = Arg->getValue();
-  } else if (!Members.empty()) {
+  if (OutputPath.empty() && !Members.empty()) {
     OutputPath = getDefaultOutputPath(Members[0]);
   } else {
     llvm::errs() << "no output path given, and cannot infer with no inputs\n";
     return 1;
   }
   // llvm-lib uses relative paths for both regular and thin archives, unlike
   // standard GNU ar, which only uses relative paths for thin archives and
   // basenames for regular archives.

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

 }
 
 def ignore : P<"ignore", "Specify warning codes to ignore">;
 def libpath: P<"libpath", "Object file search path">;
 
 // 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">;
+// have to use 'DEF' since 'def' is a reserved word
+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">;
 
 def llvmlibempty : F<"llvmlibempty">,
     HelpText<"When given no contents, produce an empty .lib file">;
 
 def machine: P<"machine", "Specify target platform">;

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

 Test that import libraries (and the members thereof) can be added to another
 static library.
 
 RUN: rm -rf %t
 RUN: mkdir -p %t
 
 RUN: echo -e "EXPORTS\nMyFunc" > %t/lib.def
 RUN: llvm-dlltool -m i386:x86-64 -l %t/lib.lib -d %t/lib.def -D lib.dll
 RUN: llvm-lib -out:%t/newlib.lib %t/lib.lib
 
 RUN: llvm-ar t %t/newlib.lib | FileCheck %s
 CHECK: lib.dll
+
+Test that import libraries can be created from a def file
+
+RUN: echo -e "NAME MyFunc.dll\nEXPORTS\nMyFunc" > %t/implib.def
+RUN: llvm-lib -out:%t/implib.lib -def:%t/implib.def
+
+RUN: llvm-ar t %t/implib.lib | FileCheck %s
+CHECK: MyFunc.dll
+