[PATCH] D16653: [CMake] Support platform building builtins without a full toolchain

[Alexey Samsonov via llvm-commits]

samsonov added a comment.

Thanks for doing this! Overall, I'm fine with the direction of this patch and it all makes sense.
I agree we have to introduce alternative and simple way to build "builtins" library for cross-compilation use cases w/o relying on valid working toolchain that is necessary for sanitizers.
Please do break this into a series of smaller patches: as I understand, many of them will just involve moving code around, splitting common configuration routines into separate files, etc.

Some issues/nits/suggestions that caught my eye as I skimmed through the patch.


================
Comment at: CMakeLists.txt:270
@@ -321,1 +269,3 @@
+  # Building the tests requires LLVM's CMake modules for lit support
+  include(AddLLVM)
   add_subdirectory(unittests)
----------------
Sink this include to the files which actually use smth. from AddLLVM? (test/CMakeLists.txt and unittest/CMakeLists.txt)?

================
Comment at: cmake/Modules/BuiltinTests.cmake:2
@@ +1,3 @@
+
+# This function takes an OS and a list of architectures and identifies the
+# subset of the architectures list that the installed toolchain can target.
----------------
This comment is wrong.

================
Comment at: cmake/Modules/CompilerRTDarwinUtils.cmake:115
@@ +114,3 @@
+    set(arch_linker_flags "-arch ${arch} ${os_linker_flags}")
+    try_compile_only(CAN_TARGET_${os}_${arch} -v -arch ${arch} ${DARWIN_${os}_CFLAGS})
+    if(CAN_TARGET_${os}_${arch})
----------------
Why can't use the same TEST_COMPILE_ONLY variable as in test_target_arch, and merge this into darwin_test_archs instead? 

================
Comment at: cmake/base-config-ix.cmake:1
@@ +1,2 @@
+# The CompilerRT build system requires CMake version 2.8.8 or higher in order
+# to use its support for building convenience "libraries" as a collection of
----------------
The comment is now irrelevant here: we actually set CMake version in a different place.

================
Comment at: cmake/base-config-ix.cmake:26
@@ +25,3 @@
+  if(NOT MSVC)
+    set(COMPILER_RT_TEST_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang)
+  else()
----------------
IIRC there were changes in these files recently: we've add COMPILER_RT_TEST_CXX_COMPILER, you might need to rebase.

================
Comment at: cmake/base-config-ix.cmake:74
@@ +73,3 @@
+# If successful, saves target flags for this architecture.
+macro(test_target_arch arch def)
+  set(TARGET_${arch}_CFLAGS ${ARGN})
----------------
Hm... do you think it's worth moving these architecture support checking to a yet another .cmake module?

================
Comment at: cmake/base-config-ix.cmake:84
@@ +83,3 @@
+  elseif(TEST_COMPILE_ONLY)
+    set(argstring "${CMAKE_EXE_LINKER_FLAGS} ${argstring}")
+    try_compile_only(CAN_TARGET_${arch} ${TARGET_${arch}_CFLAGS})
----------------
Wait, are you using argstring at all in this branch?

================
Comment at: cmake/config-ix.cmake:102
@@ -112,3 +101,3 @@
 # If successful, saves target flags for this architecture.
 macro(test_target_arch arch def)
   set(TARGET_${arch}_CFLAGS ${ARGN})
----------------
Looks like you've moved this macro to a different location, should it be deleted here now?


http://reviews.llvm.org/D16653