[CMake] Bug 14109 - CMake build for compiler-rt should use just-built clang

[beanz]

Summary

Many small improvements to LLVM_BUILD_EXTERNAL_COMPILER_RT.

• Works correctly with Ninja by working around CMake Bug 14771 (https://cmake.org/Bug/view.php?id=14771)
• Has install-compiler-rt target, and installs as part of the default install target
• Sets the install paths properly so that it matches the non-standalone build
• Only generate the test targets if(LLVM_INCLUDE_TESTS)

[samsonov]

runtime/CMakeLists.txt

33 ↗

(On Diff #36409)

Should this also be named cmake_3_4_USES_TERMINAL_OPTIONS?

40 ↗

(On Diff #36409)

This one is later re-defined from ExternalProject_Get_Property. Does it provide the same value there?

42 ↗

(On Diff #36409)

Where do you use this target?

47 ↗

(On Diff #36409)

Why do you need these deps?

71 ↗

(On Diff #36409)

Why did you remove -DCOMPILER_RT_ENABLE_WERROR=ON? I think it's fine to keep it enabled, as it's ok to ensure that ToT Clang builds ToT compiler-rt with no warnings.

[beanz]

I will send updated patches shortly.

Comments inline below.

runtime/CMakeLists.txt

33 ↗

(On Diff #36409)

This was used in an earlier iteration to pass USES_TERMINAL into a call to ExternalProject_Add_Step I will remove it.

40 ↗

(On Diff #36409)

Yep, it can be removed.

42 ↗

(On Diff #36409)

That is a convenience target for clearing out the compiler-rt directory. It isn't strictly needed, but I have found it (and the bootstrap-clear) to be useful.

Particularly as I've been changing which files are generated by the compiler-rt build and where they end up it is nice to be able to nuke the build directory.

47 ↗

(On Diff #36409)

Specifying those deps makes it so that if llvm-config or clang change compiler-rt gets cleaned out and rebuilt.

71 ↗

(On Diff #36409)

I'm trying to set the minimal set of options required to make compiler-rt build. Any additional options can be passed in via CLANG_COMPILER_RT_CMAKE_ARGS.

I think that if we expect COMPILER_RT_ENABLE_WERROR to be the way everyone is building, we should make that the default in compiler-rt, not pass it in here.

[beanz]

• Cleanup based on feedback from samsonov

[probinson]

I'm a CMake ignoramus but functionally if I'm building a Clang cross-compiler, I'd want a cross-built compiler-rt but not run its tests even if LLVM_INCLUDE_TESTS is on. Does that make sense? If so, does this change make that work properly?
Thanks...

[beanz]

probinson,

Not by itself, but this could be used as a step in that direction. The CLANG_COMPILER_RT_CMAKE_ARGS variable can be used to configure how compiler-rt is built with a great deal of flexibility, but the goal of this patch is to try and closely match how compiler-rt builds in-tree today.

-Chris

[beanz]

Ping?

[samsonov]

runtime/CMakeLists.txt

41 ↗

(On Diff #36532)

So, that's the target that you can invoke manually to clean compiler-rt build?

46 ↗

(On Diff #36532)

Hm? But "compiler-rt" target also depends on clang and llvm-config.

69 ↗

(On Diff #36532)

I thought we only refer to COMPILER_RT_INSTALL_PATH in compiler-rt's CMake. Do we actually need CMAKE_INSTALL_PREFIX there as well?

70 ↗

(On Diff #36532)

Disagree - we can't set COMPILER_RT_ENABLE_WERROR in compiler-rt's CMake, because standalone compiler-rt can be built with any compiler in the world, which we don't control. OTOH, if we *know* we're building it with trunk Clang, having it -Werror-clean is smth. we can enforce. It's also consistent with what autoconf did previously.

78 ↗

(On Diff #36532)

This is also convenience target, right?

[beanz]

runtime/CMakeLists.txt

41 ↗

(On Diff #36532)

Yes.

46 ↗

(On Diff #36532)

It should. We need it to rebuild whenever clang and llvm-config change.

Although it just occurred to me that I also need to make sure that the clearing happens before compiler-rt gets built, so there will need to be a dependency on that too... I'll figure that out.

69 ↗

(On Diff #36532)

I don't think we're actually using it in compiler-rt today, but I think that passing it through is a good idea so that if in the future compiler-rt ever uses CMAKE_INSTALL_PREFIX it will be properly populated.

70 ↗

(On Diff #36532)

At the very least if we do that it should be tied to LLVM_ENABLE_WERROR, not just defaulted to On. None of our projects default WERROR to On anywhere. While I agree building with Werror is desirable, we shouldn't be defaulting it on, and we certainly shouldn't be doing it in a way that makes it so the user has to edit the build files in order to turn it off.

78 ↗

(On Diff #36532)

Yes.

[beanz]

Updates to fix dependency mapping.

Compiler-rt will be cleaned and rebuilt if clang changes and reconfigured if llvm-config or clang changes.

[samsonov]

Looks pretty good. I'm still not convinced with your -Werror arguments, and will be unavailable during the next week, so feel free to land it, provided we'll return to the discussion later :)

Thank you!

runtime/CMakeLists.txt

62 ↗

(On Diff #36897)

Why should it be tied to LLVM_ENABLE_WERROR? LLVM_ENABLE_WERROR only makes sense if host compiler is known to be "good" and we want it to build LLVM w/o errors. Here, the compiler is *always* good. As for the user, we don't give her the option to define the compiler that would build compiler-rt: it is set to be just-built Clang. I can imagine the test case which verifies that just-built Clang compiles given source file w/o warnings. We can as well keep a large subproject just-built-Clang clean.

cfe/trunk/runtime/CMakeLists.txt

function(get_ext_project_build_command out_var target)
   else()
     set(${out_var} ${CMAKE_COMMAND} --build . --target ${target}
                                     --config $<CONFIGURATION> PARENT_SCOPE)
   endif()
 endfunction()
 
 set(COMPILER_RT_SRC_ROOT ${LLVM_MAIN_SRC_DIR}/projects/compiler-rt)
 if(LLVM_BUILD_EXTERNAL_COMPILER_RT AND EXISTS ${COMPILER_RT_SRC_ROOT}/)
-  if(CMAKE_GENERATOR MATCHES "Ninja")
-    message(FATAL_ERROR
-            "Ninja generator can't build compiler-rt as ExternalProject."
-            "Unset LLVM_BUILD_EXTERNAL_COMPILER_RT, or don't use Ninja."
-            "See http://www.cmake.org/Bug/view.php?id=14771")
+  if(CMAKE_VERSION VERSION_GREATER 3.3.20150708)
+    set(cmake_3_4_USES_TERMINAL_OPTIONS
+      USES_TERMINAL_CONFIGURE 1
+      USES_TERMINAL_BUILD 1
+      USES_TERMINAL_INSTALL 1
+      )
   endif()
 
   # Add compiler-rt as an external project.
   set(COMPILER_RT_PREFIX ${CMAKE_BINARY_DIR}/projects/compiler-rt)
-  
+  set(STAMP_DIR ${CMAKE_CURRENT_BINARY_DIR}/compiler-rt-stamps/)
+  set(BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/compiler-rt-bins/)
+
+  add_custom_target(compiler-rt-clear
+    COMMAND ${CMAKE_COMMAND} -E remove_directory ${BINARY_DIR}
+    COMMAND ${CMAKE_COMMAND} -E remove_directory ${STAMP_DIR}
+    COMMENT "Clobberring compiler-rt build and stamp directories"
+    )
+
   ExternalProject_Add(compiler-rt
     PREFIX ${COMPILER_RT_PREFIX}
     SOURCE_DIR ${COMPILER_RT_SRC_ROOT}
-    CMAKE_ARGS -DCMAKE_C_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/clang
+    STAMP_DIR ${STAMP_DIR}
+    BINARY_DIR ${BINARY_DIR}
+    CMAKE_ARGS ${CLANG_COMPILER_RT_CMAKE_ARGS}
+               -DCMAKE_C_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/clang
                -DCMAKE_CXX_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/clang++
-               -DCMAKE_BUILD_TYPE=Release
+               -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
                -DLLVM_CONFIG_PATH=${LLVM_RUNTIME_OUTPUT_INTDIR}/llvm-config
                -DCOMPILER_RT_OUTPUT_DIR=${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION}
                -DCOMPILER_RT_EXEC_OUTPUT_DIR=${LLVM_RUNTIME_OUTPUT_INTDIR}
-               -DCOMPILER_RT_INSTALL_PATH=lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}
+               -DCOMPILER_RT_INSTALL_PATH=${CMAKE_INSTALL_PREFIX}/lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}
                -DCOMPILER_RT_INCLUDE_TESTS=${LLVM_INCLUDE_TESTS}
-               -DCOMPILER_RT_ENABLE_WERROR=ON
+               -DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}
     INSTALL_COMMAND ""
     STEP_TARGETS configure build
-    )
-  # Due to a bug, DEPENDS in ExternalProject_Add doesn't work
-  # in CMake 2.8.9 and 2.8.10.
-  add_dependencies(compiler-rt llvm-config clang)
-
-  # Add a custom step to always re-configure compiler-rt (in case some of its
-  # sources have changed).
-  ExternalProject_Add_Step(compiler-rt force-reconfigure
-    DEPENDERS configure
-    ALWAYS 1
+    ${cmake_3_4_USES_TERMINAL_OPTIONS}
     )
 
-  ExternalProject_Add_Step(compiler-rt clobber
-    COMMAND ${CMAKE_COMMAND} -E remove_directory <BINARY_DIR>
-    COMMAND ${CMAKE_COMMAND} -E make_directory <BINARY_DIR>
-    COMMENT "Clobberring compiler-rt build directory..."
-    DEPENDERS configure
-    DEPENDS ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang
+  get_ext_project_build_command(run_clean_compiler_rt clean)
+  ExternalProject_Add_Step(compiler-rt clean
+    COMMAND ${run_clean_compiler_rt}
+    COMMENT "Cleaning compiler-rt..."
+    DEPENDEES configure
+    DEPENDERS build
+    DEPENDS clang
+    WORKING_DIRECTORY ${BINARY_DIR}
     )
 
-  ExternalProject_Get_Property(compiler-rt BINARY_DIR)
-  set(COMPILER_RT_BINARY_DIR ${BINARY_DIR})
+  add_dependencies(compiler-rt-configure clang llvm-config)
+
+  install(CODE "execute_process\(COMMAND ${CMAKE_COMMAND} -P ${BINARY_DIR}/cmake_install.cmake \)"
+    COMPONENT compiler-rt)
+
+  add_custom_target(install-compiler-rt
+                    DEPENDS compiler-rt
+                    COMMAND "${CMAKE_COMMAND}"
+                             -DCMAKE_INSTALL_COMPONENT=compiler-rt
+                             -P "${CMAKE_BINARY_DIR}/cmake_install.cmake")
 
   # Add top-level targets that build specific compiler-rt runtimes.
   set(COMPILER_RT_RUNTIMES asan builtins dfsan lsan msan profile tsan ubsan)
   foreach(runtime ${COMPILER_RT_RUNTIMES})
     get_ext_project_build_command(build_runtime_cmd ${runtime})
     add_custom_target(${runtime}
       COMMAND ${build_runtime_cmd}
       DEPENDS compiler-rt-configure
-      WORKING_DIRECTORY ${COMPILER_RT_BINARY_DIR}
+      WORKING_DIRECTORY ${BINARY_DIR}
       VERBATIM)
   endforeach()
 
+  if(LLVM_INCLUDE_TESTS)
     # Add binaries that compiler-rt tests depend on.
     set(COMPILER_RT_TEST_DEPENDENCIES
       FileCheck count not llvm-nm llvm-symbolizer)
 
     # Add top-level targets for various compiler-rt test suites.
     set(COMPILER_RT_TEST_SUITES check-asan check-asan-dynamic check-dfsan
       check-lsan check-msan check-sanitizer check-tsan check-ubsan)
     foreach(test_suite ${COMPILER_RT_TEST_SUITES})
       get_ext_project_build_command(run_test_suite ${test_suite})
       add_custom_target(${test_suite}
         COMMAND ${run_test_suite}
         DEPENDS compiler-rt-build ${COMPILER_RT_TEST_DEPENDENCIES}
-      WORKING_DIRECTORY ${COMPILER_RT_BINARY_DIR}
+        WORKING_DIRECTORY ${BINARY_DIR}
         VERBATIM)
     endforeach()
 
     # Add special target to run all compiler-rt test suites.
     get_ext_project_build_command(run_check_compiler_rt check-all)
     add_custom_target(check-compiler-rt
       COMMAND ${run_check_compiler_rt}
       DEPENDS compiler-rt-build ${COMPILER_RT_TEST_DEPENDENCIES}
-    WORKING_DIRECTORY ${COMPILER_RT_BINARY_DIR}
+      WORKING_DIRECTORY ${BINARY_DIR}
       VERBATIM)
   endif()
+endif()