'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.
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.
For the existing main use case in llvm-lib, there's a different codepath that uses getDefaultOutputPath(Members). 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.
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.
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.
This change breaks virtually all existing tests for llvm-lib.
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.
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.
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.