[Openmp-commits] [PATCH] D143431: Switch the Windows OpenMP import library to import by name rather than ordinal.

Martin Storsjö via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Tue Feb 14 01:51:45 PST 2023

mstorsjo added a comment.

This patch broke building in mingw mode, with errors like this:

  ninja: error: build.ninja:875: multiple rules generate runtime/src/libomp.dll.a [-w dupbuild=err] 

I've got a patch I can submit to fix that. But this patch also has a couple of other issues.

Comment at: openmp/runtime/src/CMakeLists.txt:278
   libomp_string_to_list("${LIBOMP_GDFLAGS}" LIBOMP_GDFLAGS)
This patch adds a new nice define `LIBOMP_GENERATED_DEF_FILE` here, but on line 240, we still refer to it by hardcoding its value, `add_custom_target(libomp-needed-windows-files DEPENDS ${LIBOMP_LIB_NAME}.def)`. It would be good if both references would use the same cmake variable, since right now, it seems like you can adjust its name, but that only causes breakage.

Comment at: openmp/runtime/src/CMakeLists.txt:288
     DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/dllexports ${LIBOMP_TOOLS_DIR}/generate-def.pl
Here, you're generating two files with one single cmake `add_custom_command`. This means that cmake essentially doesn't know that the second file, `LIBOMPIMP_GENERATED_DEF_FILE_IMP` is generated. When cleaning, it doesn't get removed, and if you just build `ninja runtime/src/libomp.lib` without having built the rest before, then cmake/ninja won't know about generating the def file before. So I'd suggest generating that second file with a separate command so that cmake knows and can track the file (and probably adding a suitable depenency for the ompimp target so that it generates the def file as needed).

Comment at: openmp/runtime/src/CMakeLists.txt:289
-    # Create new import library that is just the previously created one + kmp_import.cpp
-    add_library(ompimp STATIC ${LIBOMP_GENERATED_IMP_LIB} kmp_import.cpp)
-    set_target_properties(ompimp PROPERTIES
Previously, the finally installed import library contained the compiled `kmp_import.cpp` too, which contains symbols like `_You_must_link_with_exactly_one_OpenMP_library`. These are now lost.

Comment at: openmp/runtime/src/CMakeLists.txt:293
+  # Regenerate the import library to import by name, not ordinal
+  add_library(ompimp STATIC ${LIBOMP_SOURCE_FILES})
+    set_target_properties(ompimp PROPERTIES
By adding all source files here, you end up compiling all the object files twice - even if they aren't really strictly used for the second invocation (where we just invoke `lib.exe` to regenerate an import library). Not sure if cmake is ok with that, or if it would need a dummy source file at least.

Compiling all files twice feels wasteful; the openmp runtime isn't that big so it's not a huge deal, but it seems quite redundant and inelegant.

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list