[CMake] Remove unnecesary list of source files for Xray unit tests.

[delcypher]

Summary

[CMake] Remove unnecesary list of source files for Xray unit tests.

The list duplicates information already available in the parent
directory so use that instead. It is unclear to me why we need
to spell out the dependencies explicitly but fixing that should
be done in a separate patch.

[delcypher]

@dberris The tests aren't built under macOS so I couldn't test if this patch actually works. Would it be possible for you to test it for me?

[george.karpenkov]

Looks reasonable, but I think @dberris would have to approve.

[dberris]

LGTM -- @nglevin was working on getting the XRay unit tests at least build and run but got interrupted.

The original reason for doing this was to ensure that when any of the implementation files changed, that the unit tests would be built. As you've learned this only made sense for platforms where the tests are built and run.

I'm fine with landing this then iterating, no need for me to test myself.

[delcypher]

In D49177#1159416, @dberris wrote:

LGTM -- @nglevin was working on getting the XRay unit tests at least build and run but got interrupted.

The original reason for doing this was to ensure that when any of the implementation files changed, that the unit tests would be built. As you've learned this only made sense for platforms where the tests are built and run.

In general, in CMake, the target for running a test is made to be dependent on the unit test being executed.. In turn the unit test is linked against the libraries it uses. If the source files for a library change that should trigger the library to be rebuilt. At the minimum this will cause the unit test to be relinked. If the unit test also depends on header files that have changed that should cause the unit test to be recompiled too.

Why is that not sufficient for us?

[delcypher]

I'm fine with landing this then iterating, no need for me to test myself.

Okay. I'll land this now.

[dberris]

In D49177#1160011, @delcypher wrote:

In D49177#1159416, @dberris wrote:

LGTM -- @nglevin was working on getting the XRay unit tests at least build and run but got interrupted.

The original reason for doing this was to ensure that when any of the implementation files changed, that the unit tests would be built. As you've learned this only made sense for platforms where the tests are built and run.

In general, in CMake, the target for running a test is made to be dependent on the unit test being executed.. In turn the unit test is linked against the libraries it uses. If the source files for a library change that should trigger the library to be rebuilt. At the minimum this will cause the unit test to be relinked. If the unit test also depends on header files that have changed that should cause the unit test to be recompiled too.

Why is that not sufficient for us?

I looked into this a bit and it seems that, since the linking of the XRay runtimes is done by clang, there's no hard dependency between the runtime library built and the one that clang adds to the link line. This causes the issue where even if the XRay runtime is re-built, the unit tests don't get re-linked because there wasn't an explicit dependency between the test and the files. This is important for the flags, for instance, along with the runtime-initialisation code that the XRay unit tests also depend on.

This gets worse for internal headers/implementation details changing, since the generate_compiler_rt_tests(...) function doesn't actually use file dependency tracking (implicit tracking I mean) but instead does something different from the normal CMake approach of say using add_test(...) and having normal test binaries.

It can be argued that we could split up the components better, and is definitely something worth exploring later if there's a better way of doing this.

lib/xray/CMakeLists.txt

set(XRAY_IMPL_HEADERS
   xray_profiling_flags.h
   xray_profiling_flags.inc
   xray_recursion_guard.h
   xray_segmented_array.h
   xray_tsc.h
   xray_utils.h
   xray_x86_64.inc)
 
+# Create list of all source files for
+# consumption by tests.
+set(XRAY_ALL_SOURCE_FILES
+  ${XRAY_SOURCES}
+  ${XRAY_FDR_MODE_SOURCES}
+  ${XRAY_BASIC_MODE_SOURCES}
+  ${XRAY_PROFILING_MODE_SOURCES}
+  ${x86_64_SOURCES}
+  ${arm_SOURCES}
+  ${armhf_SOURCES}
+  ${mips_SOURCES}
+  ${mipsel_SOURCES}
+  ${mips64_SOURCES}
+  ${mips64el_SOURCES}
+  ${powerpc64le_SOURCES}
+  ${XRAY_IMPL_HEADERS}
+)
+list(REMOVE_DUPLICATES XRAY_ALL_SOURCE_FILES)
+# Make list that uses absolute paths
+set(XRAY_ALL_SOURCE_FILES_ABS_PATHS "")
+foreach (src_file ${XRAY_ALL_SOURCE_FILES})
+  list(APPEND
+    XRAY_ALL_SOURCE_FILES_ABS_PATHS
+    "${CMAKE_CURRENT_SOURCE_DIR}/${src_file}")
+endforeach()
+
+
 # Now put it all together...
 include_directories(..)
 include_directories(../../include)
 
 set(XRAY_CFLAGS ${SANITIZER_COMMON_CFLAGS})
 set(XRAY_COMMON_DEFINITIONS XRAY_HAS_EXCEPTIONS=1)
 
 # We don't need RTTI in XRay, so turn that off.

lib/xray/tests/CMakeLists.txt

 include_directories(..)
 
 add_custom_target(XRayUnitTests)
 set_target_properties(XRayUnitTests PROPERTIES FOLDER "XRay unittests")
 
-# Create an XRAY_IMPL_FILES variable which must include all the implementation
-# files that are in the lib directory. We *must* keep this list up-to-date.
-set(XRAY_IMPL_FILES
-  ../../xray_AArch64.cc
-  ../../xray_allocator.h
-  ../../xray_arm.cc
-  ../../xray_basic_flags.cc
-  ../../xray_basic_flags.h
-  ../../xray_basic_logging.cc
-  ../../xray_basic_logging.h
-  ../../xray_buffer_queue.cc
-  ../../xray_buffer_queue.h
-  ../../xray_defs.h
-  ../../xray_fdr_flags.cc
-  ../../xray_fdr_flags.h
-  ../../xray_fdr_logging.cc
-  ../../xray_fdr_logging.h
-  ../../xray_fdr_log_records.h
-  ../../xray_flags.cc
-  ../../xray_flags.h
-  ../../xray_function_call_trie.h
-  ../../xray_init.cc
-  ../../xray_interface.cc
-  ../../xray_interface_internal.h
-  ../../xray_log_interface.cc
-  ../../xray_mips64.cc
-  ../../xray_mips.cc
-  ../../xray_powerpc64.cc
-  ../../xray_profile_collector.cc
-  ../../xray_profile_collector.h
-  ../../xray_profiling_flags.cc
-  ../../xray_profiling_flags.h
-  ../../xray_recursion_guard.h
-  ../../xray_segmented_array.h
-  ../../xray_trampoline_powerpc64.cc
-  ../../xray_tsc.h
-  ../../xray_utils.cc
-  ../../xray_utils.h
-  ../../xray_x86_64.cc)
+# Sanity check XRAY_ALL_SOURCE_FILES_ABS_PATHS
+list(LENGTH XRAY_ALL_SOURCE_FILES_ABS_PATHS XASFAP_LENGTH)
+if (${XASFAP_LENGTH} EQUAL 0)
+  message(FATAL_ERROR "XRAY_ALL_SOURCE_FILES_ABS_PATHS cannot be empty")
+endif()
+unset(XASFAP_LENGTH)
+foreach (src_file ${XRAY_ALL_SOURCE_FILES_ABS_PATHS})
+  if (NOT EXISTS "${src_file}")
+    message(FATAL_ERROR "Source file \"${src_file}\" does not exist")
+  endif()
+endforeach()
 
 set(XRAY_UNITTEST_CFLAGS
   ${XRAY_CFLAGS}
   ${COMPILER_RT_UNITTEST_CFLAGS}
   ${COMPILER_RT_GTEST_CFLAGS}
   -I${COMPILER_RT_SOURCE_DIR}/include
   -I${COMPILER_RT_SOURCE_DIR}/lib/xray
   -I${COMPILER_RT_SOURCE_DIR}/lib)
foreach(arch ${XRAY_TEST_ARCH})
       get_xray_lib_for_arch(${arch} XRAY_RUNTIME_LIBS)
       generate_compiler_rt_tests(TEST_OBJECTS
         XRayUnitTests "${testname}-${arch}-Test" "${arch}"
         SOURCES ${TEST_SOURCES} ${COMPILER_RT_GTEST_SOURCE}
         # Note that any change in the implementations will cause all the unit
         # tests to be re-built. This is by design, but may be cumbersome during
         # the build/test cycle.
         COMPILE_DEPS ${TEST_SOURCES} ${COMPILER_RT_GTEST_SOURCE}
-        ${XRAY_HEADERS} ${XRAY_IMPL_FILES}
+        ${XRAY_HEADERS} ${XRAY_ALL_SOURCE_FILES_ABS_PATHS}
         RUNTIME "${XRAY_RUNTIME_LIBS}"
         DEPS gtest xray
         CFLAGS ${XRAY_UNITTEST_CFLAGS}
         LINK_FLAGS ${TARGET_LINK_FLAGS} ${XRAY_UNITTEST_LINK_FLAGS})
       set_target_properties(XRayUnitTests
         PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
     endforeach()
   endif()