[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 inline comments.


================
Comment at: llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp:434
   // 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]);
----------------
This change breaks virtually all existing tests for llvm-lib.


================
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">;
 
----------------
vadikp-intel wrote:
> mstorsjo wrote:
> > 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.
> '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.
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.


================
Comment at: llvm/test/tools/llvm-lib/implibs.test:16
+
+RUN: echo -e "NAME MyFunc.dll\nEXPORTS\nMyFunc" > %t/implib.def
+RUN: llvm-lib -out:%t/implib.lib -def:%t/implib.def
----------------
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`.


================
Comment at: llvm/test/tools/llvm-lib/implibs.test:19
+
+RUN: llvm-ar t %t/implib.lib | FileCheck %s
+CHECK: MyFunc.dll
----------------
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.


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

https://reviews.llvm.org/D144765