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

[Martin Storsjö via Phabricator via Openmp-commits]

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)
+  set(LIBOMP_GENERATED_DEF_FILE ${LIBOMP_LIB_NAME}.def)
+  set(LIBOMPIMP_GENERATED_DEF_FILE_IMP ${LIBOMP_LIB_NAME}.imp.def)
----------------
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
+    COMMAND ${PERL_EXECUTABLE} ${LIBOMP_TOOLS_DIR}/generate-def.pl ${LIBOMP_GDFLAGS} -D NAME=${LIBOMP_LIB_FILE} -D NOORDINALS
+            -o ${LIBOMPIMP_GENERATED_DEF_FILE_IMP} ${CMAKE_CURRENT_SOURCE_DIR}/dllexports
     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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143431