This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by vadikp-intel on Feb 24 2023, 4:04 PM.

Details

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.

Diff Detail

Event Timeline

vadikp-intel created this revision.Feb 24 2023, 4:04 PM
vadikp-intel requested review of this revision.Feb 24 2023, 4:04 PM

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.

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

vadikp-intel marked 4 inline comments as done.Feb 27 2023, 9:37 PM
vadikp-intel added inline comments.
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 marked an inline comment as done.

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

mstorsjo added inline comments.Feb 28 2023, 12:13 AM
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.

Fixed test and output file name logic.

vadikp-intel marked 4 inline comments as done.

addressed remaining review comments

mstorsjo accepted this revision.Feb 28 2023, 11:27 PM

LGTM, thanks for addressing the feedback!

This revision is now accepted and ready to land.Feb 28 2023, 11:27 PM
hans accepted this revision.Mar 1 2023, 6:12 AM

lgtm, nice!

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