[PATCH] [compiler-rt][builtins] CTest to execute builtins testsuite

Alexey Samsonov vonosmas at gmail.com
Thu Jul 10 16:54:22 PDT 2014


Once again, sorry for delay with reviewing this.

================
Comment at: test/builtins/CMakeLists.txt:1
@@ +1,2 @@
+# CTest for builtins Unit test-suite
+#
----------------
Please specify somewhere that you're actually using a host compiler to build these tests, and link them against a just-built builtins library.

================
Comment at: test/builtins/CMakeLists.txt:184
@@ +183,3 @@
+# For tests that rely on common(*_lib.h) headers
+include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../../lib/builtins)
+
----------------
${COMPILER_RT_SOURCE_DIR}/lib/builtins

================
Comment at: test/builtins/CMakeLists.txt:187
@@ +186,3 @@
+# gcc personality needs an extra source file
+set(gcc_personality_EXTRA Unit/gcc_personality_test_helper.cxx)
+
----------------
Is it picked up by CMake automatically?

================
Comment at: test/builtins/CMakeLists.txt:193
@@ +192,3 @@
+foreach(arch x86_64 i386 arm ppc mips)
+  if(CAN_TARGET_${arch})
+    foreach (src ${GENERIC_SOURCES} ${${arch}_SOURCES})
----------------
please check COMPILER_RT_CAN_EXECUTE_TESTS here instead - we don't need to add targets otherwise.

================
Comment at: test/builtins/CMakeLists.txt:196
@@ +195,3 @@
+      # Strip leading directories and file-name suffix for executable
+      string(REGEX REPLACE "[^/]*/" "" test ${src})
+      string(REPLACE "_test.c" "" test ${test})
----------------
consider using get_filename_component instead.

================
Comment at: test/builtins/CMakeLists.txt:209
@@ +208,3 @@
+      set_target_properties(${test} PROPERTIES
+        LINK_FLAGS "-L${CMAKE_BINARY_DIR}/lib/${COMPILER_RT_OS_DIR}")
+    endforeach ()
----------------
Use ${COMPILER_RT_LIBRARY_OUTPUT_DIR} instead.

================
Comment at: test/builtins/CMakeLists.txt:214
@@ +213,3 @@
+      COMPILE_OPTIONS "-fexceptions"
+      LINK_LIBRARIES "-lstdc++")
+    set_target_properties(trampoline_setup PROPERTIES
----------------
Why not LINK_FLAGS?

http://reviews.llvm.org/D4251






More information about the llvm-commits mailing list