[cmake] Fix for a bug https://llvm.org/bugs/show_bug.cgi?id=30489 "Cannot build with -DLIBOMP_FORTRAN_MODULES=True"

[pawosm01]

Summary

Missing make_directory added.

[jlpeyton]

I think this introduces a race during parallel builds where the POST_BUILD for the targets libomp-mod and omp both try to create the LIBOMP_EXPORTS_CMN_DIR.

To fix this, directories can be created during configuration time using file(MAKE_DIRECTORY ...). That way, all these lines can be removed:

COMMAND ${CMAKE_COMMAND} -E make_directory ...

[beanz]

Creating the directory during configuration time is not good. If someone deletes it from their build directory it will not be re-created until cmake is re-run. A better solution would be to create a separate add_custom_command that creates the directory, and a target that wraps that command. Then have the targets which have these POST_BUILD actions depend on the target that creates the directory.

Also worth noting, you do need to wrap the add_custom_command that creates the directory in a target because custom commands can race if multiple targets reference their outputs as dependencies.

[pawosm01]

In D24959#554036, @jlpeyton wrote:

I think this introduces a race during parallel builds where the POST_BUILD for the targets libomp-mod and omp both try to create the LIBOMP_EXPORTS_CMN_DIR.

To fix this, directories can be created during configuration time using file(MAKE_DIRECTORY ...). That way, all these lines can be removed:

COMMAND ${CMAKE_COMMAND} -E make_directory ...

From what I found this code was never meant to run in parallel, it is only included when:

# Move files to exports/ directory if requested
if(${LIBOMP_COPY_EXPORTS})
  include(LibompExports)
endif()

...while LIBOMP_COPY_EXPORTS is described:

# Should the libomp library and generated headers be copied into the original source exports/ directory
# Turning this to FALSE aids **parallel builds ** to **not interfere** with each other.
# Currently, the testsuite module expects the just built OpenMP library to be located inside the exports/
# directory.  TODO: have testsuite run under llvm-lit directly.  We can then get rid of copying to exports/
set(LIBOMP_COPY_EXPORTS TRUE CACHE STRING
  "Should exports be copied into source exports/ directory?")

...so I guess my fix is still ok.

[pawosm01]

Also note that make_directory does not complain when created directory already exists (so parallel directory creation should not be a problem). If it was a problem, it would not be possible to re-run CMake after previous build and it would be noticed very quickly.

[jlpeyton]

From what I found this code was never meant to run in parallel

I should clarify that by parallel I mean running make in parallel (make -j).

When building using make -j, both the omp target and the libomp-mod targets can be built in parallel since neither depends on the other. Since they can run in parallel, their POST_BUILD custom commands could run in parallel, and your change would make it possible (very remote chance) for two make_directory ${LIBOMP_EXPORTS_CMN_DIR} commands to run in parallel.

...so I guess my fix is still ok.
Also note that make_directory does not complain when created directory already exists (so parallel directory creation should not be a problem).

Your fix is fine if make_directory is thread safe on every supported platform (Linux, NetBSD, FreeBSD, Mac, Windows) and that doesn't take into account possible oddities of the underlying filesystem (is mkdir on NFS thread safe?). And by thread safe, I mean that make_directory always successfully exits given simultaneous executions trying to create the exact same directory. I don't know that it is. It could be, but I would feel far safer implementing what Chris has suggested.

[pawosm01]

In D24959#554060, @beanz wrote:

Creating the directory during configuration time is not good. If someone deletes it from their build directory it will not be re-created until cmake is re-run. A better solution would be to create a separate add_custom_command that creates the directory, and a target that wraps that command. Then have the targets which have these POST_BUILD actions depend on the target that creates the directory.

No can do: POST_BUILD actions can NOT have dependencies, see:

https://cmake.org/cmake/help/v3.5/command/add_custom_command.html#build-events

[pawosm01]

The way LIBOMP_OMPT_SUPPORT is handled brought me some idea though:

diff -ruN openmp.old/runtime/cmake/LibompExports.cmake openmp/runtime/cmake/LibompExports.cmake
--- openmp.old/runtime/cmake/LibompExports.cmake        2016-09-27 11:23:44.209728000 +0100
+++ openmp/runtime/cmake/LibompExports.cmake    2016-09-28 00:10:54.843307159 +0100
@@ -64,10 +64,12 @@
 if(${LIBOMP_FORTRAN_MODULES})
   add_custom_command(TARGET libomp-mod POST_BUILD
     COMMAND ${CMAKE_COMMAND} -E make_directory ${LIBOMP_EXPORTS_MOD_DIR}
-    COMMAND ${CMAKE_COMMAND} -E copy omp_lib.h ${LIBOMP_EXPORTS_CMN_DIR}
     COMMAND ${CMAKE_COMMAND} -E copy omp_lib.mod ${LIBOMP_EXPORTS_MOD_DIR}
     COMMAND ${CMAKE_COMMAND} -E copy omp_lib_kinds.mod ${LIBOMP_EXPORTS_MOD_DIR}
   )
+  add_custom_command(TARGET omp POST_BUILD
+    COMMAND ${CMAKE_COMMAND} -E copy omp_lib.h ${LIBOMP_EXPORTS_CMN_DIR}
+  )
 endif()
 
 # Copy OpenMP library into exports/ directory post build

what do you think?

[pawosm01]

Tested, works.

[jlpeyton]

This is fine for now. I may come back to it though.

openmp/trunk/runtime/cmake/LibompExports.cmake

 if(${LIBOMP_OMPT_SUPPORT})
   add_custom_command(TARGET omp POST_BUILD
     COMMAND ${CMAKE_COMMAND} -E copy ompt.h ${LIBOMP_EXPORTS_CMN_DIR}
   )
 endif()
 if(${LIBOMP_FORTRAN_MODULES})
   add_custom_command(TARGET libomp-mod POST_BUILD
     COMMAND ${CMAKE_COMMAND} -E make_directory ${LIBOMP_EXPORTS_MOD_DIR}
-    COMMAND ${CMAKE_COMMAND} -E copy omp_lib.h ${LIBOMP_EXPORTS_CMN_DIR}
     COMMAND ${CMAKE_COMMAND} -E copy omp_lib.mod ${LIBOMP_EXPORTS_MOD_DIR}
     COMMAND ${CMAKE_COMMAND} -E copy omp_lib_kinds.mod ${LIBOMP_EXPORTS_MOD_DIR}
   )
+  add_custom_command(TARGET omp POST_BUILD
+    COMMAND ${CMAKE_COMMAND} -E copy omp_lib.h ${LIBOMP_EXPORTS_CMN_DIR}
+  )
 endif()
 
 # Copy OpenMP library into exports/ directory post build
 if(WIN32)
   get_target_property(LIBOMP_OUTPUT_DIRECTORY omp RUNTIME_OUTPUT_DIRECTORY)
 else()
   get_target_property(LIBOMP_OUTPUT_DIRECTORY omp LIBRARY_OUTPUT_DIRECTORY)
 endif()