Default to using in-tree clang for building test executables

[labath]

Summary

Using the in-tree clang should be the default test configuration as that
is the one compiler that we can be sure everyone has (better
reproducibility of test results). Also, it should hopefully reduce the
impact of pr35040.

[zturner]

There is already a CMake variable called LLDB_TEST_COMPILER. This LLDB_TEST_CLANG was introduced later, I guess unaware of the presence of LLDB_TEST_COMPILER. Would it be possible to standardize on one variable? This would also mean deleting the TEST_C_COMPILER and TEST_CXX_COMPILER` variables. I don't see why we should ever have both. Run the test suite a second time if you need to test with different C and C++ compilers.

[davide]

I was going to propose the same :)
Maybe an heads up on lldb-dev?

Thanks!

[davide]

I think we should just use the _COMPILER variant and default to the clang in tree (wheter possible)

[zturner]

Yea. Right now there's FOUR different variables to specify the compiler, and they can all conflict with each other. You can theoretically set

LLDB_TEST_COMPILER=<path-to-out-of-tree-clang>
LLDB_TEST_CLANG=On
LLDB_TEST_C_COMPILER=/usr/bin/cc
LLDB_TEST_CXX_COMPILER=/usr/bin/clang++

This is a completely nonsensical configuration, and it shouldn't be allowed. One variable to rule them, LLDB_TEST_COMPILER=<path>. If it's present, use it. Otherwise, default to in-tree clang. Deduce cxx and c compilers from the given path.

[labath]

In D39215#904510, @zturner wrote:

There is already a CMake variable called LLDB_TEST_COMPILER. This LLDB_TEST_CLANG was introduced later, I guess unaware of the presence of LLDB_TEST_COMPILER. Would it be possible to standardize on one variable? This would also mean deleting the TEST_C_COMPILER and TEST_CXX_COMPILER` variables. I don't see why we should ever have both. Run the test suite a second time if you need to test with different C and C++ compilers.

Unlike msvc, where you just use CL to compile everything, on ***nix you have different compiler drivers for C (gcc, clang) and C++ (g++, clang++). The differences are minute, but we still need both (clang has --driver-mode=g++ argument to emulate g++, but I don't know of any gcc option like that).

However, I do see a bit of space for simplification. I can remove LLDB_TEST_CLANG altogether, and just have LLDB_TEST_???_COMPILER default to clang.

[labath]

In D39215#904536, @labath wrote:

In D39215#904510, @zturner wrote:

There is already a CMake variable called LLDB_TEST_COMPILER. This LLDB_TEST_CLANG was introduced later, I guess unaware of the presence of LLDB_TEST_COMPILER. Would it be possible to standardize on one variable? This would also mean deleting the TEST_C_COMPILER and TEST_CXX_COMPILER` variables. I don't see why we should ever have both. Run the test suite a second time if you need to test with different C and C++ compilers.

Unlike msvc, where you just use CL to compile everything, on ***nix you have different compiler drivers for C (gcc, clang) and C++ (g++, clang++). The differences are minute, but we still need both (clang has --driver-mode=g++ argument to emulate g++, but I don't know of any gcc option like that).

However, I do see a bit of space for simplification. I can remove LLDB_TEST_CLANG altogether, and just have LLDB_TEST_???_COMPILER default to clang.

Scratch that, dotest.py already has some magic to compute the c++ compiler, given the c version, so I guess one variable is really enough.

[labath]

I've played around with this, but I couldn't get the lit/lit.site.cfg.in file to see the expanded values of the $<TARGET_FILE:clang> generator expression (the reason it works now is because the file is special-casing LLDB_TEST_CLANG=True). Currently, I do not see how to work around that.

So, I propose to separate the functional change from the cleanup, and leave the cleanup part to someone with more cmake-fu. :P

[zturner]

In D39215#904578, @labath wrote:

I've played around with this, but I couldn't get the lit/lit.site.cfg.in file to see the expanded values of the $<TARGET_FILE:clang> generator expression (the reason it works now is because the file is special-casing LLDB_TEST_CLANG=True). Currently, I do not see how to work around that.

So, I propose to separate the functional change from the cleanup, and leave the cleanup part to someone with more cmake-fu. :P

I think we just need to do:

set(DEFAULT_LLDB_TEST_COMPILER "$<TARGET_FILE:clang>")

set(LLDB_TEST_COMPILER "${DEFAULT_LLDB_TEST_COMPILER}" CACHE PATH "Path to clang executable used to run LLDB tests")

Does this not work?

[zturner]

In D39215#904615, @zturner wrote:

In D39215#904578, @labath wrote:

I've played around with this, but I couldn't get the lit/lit.site.cfg.in file to see the expanded values of the $<TARGET_FILE:clang> generator expression (the reason it works now is because the file is special-casing LLDB_TEST_CLANG=True). Currently, I do not see how to work around that.

So, I propose to separate the functional change from the cleanup, and leave the cleanup part to someone with more cmake-fu. :P

I think we just need to do:

set(DEFAULT_LLDB_TEST_COMPILER "$<TARGET_FILE:clang>")

set(LLDB_TEST_COMPILER "${DEFAULT_LLDB_TEST_COMPILER}" CACHE PATH "Path to clang executable used to run LLDB tests")

Does this not work?

Oh, and then inside the lit.site.cfg.in, use %LLDB_TEST_COMPILER% everywhere.

[zturner]

Ok the issue is that you cant use CMake generator expressions in this way. This should work though:

if (TARGET clang)
  set(LLDB_DEFAULT_TEST_COMPILER "${LLVM_BINARY_DIR}/clang${CMAKE_EXECUTABLE_SUFFIX}")
else()
  set(LLDB_DEFAULT_TEST_COMPILER "")
endif()

set(LLDB_TEST_COMPILER "${LLDB_DEFAULT_TEST_COMPILER}" CACHE PATH "Compiler to use for building LLDB test inferiors")

if ("${LLDB_TEST_COMPILER}" STREQUAL "")
  message(FATAL_ERROR "LLDB test compiler not specified.  Tests will not run")
endif()

I think you might be able to do something like:

if (TARGET clang)
  get_property(LLDB_DEFAULT_TEST_COMPILER TARGET clang PROPERTY LOCATION)
endif()

as well, but I haven't tried.

[labath]

In D39215#904677, @zturner wrote:

Ok the issue is that you cant use CMake generator expressions in this way. This should work though:

if (TARGET clang)
  set(LLDB_DEFAULT_TEST_COMPILER "${LLVM_BINARY_DIR}/clang${CMAKE_EXECUTABLE_SUFFIX}")
else()
  set(LLDB_DEFAULT_TEST_COMPILER "")
endif()

That will work, if you don't mind that it will break on multi-configuration builds such as visual studio (though I am not sure how/if regular lit tests work in that scenario). We don't run those tests anyway, and I do not think they are ready to be used, so I definitely won't mind.

I think you might be able to do something like:

if (TARGET clang)
  get_property(LLDB_DEFAULT_TEST_COMPILER TARGET clang PROPERTY LOCATION)
endif()

This (although recommended by stack overflow), does not work on cmake 3.0+.

[ted]

We build lldb, clang and tools for Hexagon only, and call them hexagon-lldb, hexagon-clang, etc. The test infrastructure is smart enough to pick up hexagon-lldb-mi if we tell it to run with hexagon-lldb using --executable; will it be smart enough to run an in-tree hexagon-clang?

@labath, we run on Windows using hexagon-clang and hexagon-clang++; don't forget the embedded cases when choosing compilers and running tests.

I'm all for removing redundant variables.

[zturner]

In D39215#905259, @ted wrote:

We build lldb, clang and tools for Hexagon only, and call them hexagon-lldb, hexagon-clang, etc. The test infrastructure is smart enough to pick up hexagon-lldb-mi if we tell it to run with hexagon-lldb using --executable; will it be smart enough to run an in-tree hexagon-clang?

@labath, we run on Windows using hexagon-clang and hexagon-clang++; don't forget the embedded cases when choosing compilers and running tests.

I'm all for removing redundant variables.

I haven't checked the logic for deducing the cxx from the c, and vice versa. But if it's as simple as just adding or removing ++ from the end of the executable name, it should work.

[labath]

This removes the LLDB_TEST_CLANG and LLDB_TEST_COMPILER settings.

I've decided to keep the separate C and CXX variables because:
a) that's consistent with other cmake settings
b) in case of gcc, it's not easy to figure out the c++ compiler name

(potentially need to strip executable suffix and version number), and I did not
want to implement one more dodgy heuristic.

[probinson]

Has this gone in? I'm wondering because I starting playing with the monorepo, ran cmake with -DLLDB_TEST_COMPILER=$PWD/bin/clang, and today's test failure seems to be trying to build the test program with the system compiler (gcc) rather than my copy of clang. But it looks like you're deprecating LLDB_TEST_COMPILER?

CMakeLists.txt

if (NOT LLDB_DISABLE_PYTHON)
   add_subdirectory(scripts)
 endif ()
 add_subdirectory(source)
 add_subdirectory(tools)
 
 option(LLDB_INCLUDE_TESTS "Generate build targets for the LLDB unit tests."
   ${LLVM_INCLUDE_TESTS})
 if(LLDB_INCLUDE_TESTS)
+  if (TARGET clang)
+    set(LLDB_DEFAULT_TEST_C_COMPILER "${LLVM_BINARY_DIR}/bin/clang${CMAKE_EXECUTABLE_SUFFIX}")
+    set(LLDB_DEFAULT_TEST_CXX_COMPILER "${LLVM_BINARY_DIR}/bin/clang++${CMAKE_EXECUTABLE_SUFFIX}")
+  else()
+    set(LLDB_DEFAULT_TEST_C_COMPILER "")
+    set(LLDB_DEFAULT_TEST_CXX_COMPILER "")
+  endif()
+
+  set(LLDB_TEST_C_COMPILER "${LLDB_DEFAULT_TEST_C_COMPILER}" CACHE PATH "C Compiler to use for building LLDB test inferiors")
+  set(LLDB_TEST_CXX_COMPILER "${LLDB_DEFAULT_TEST_CXX_COMPILER}" CACHE PATH "C++ Compiler to use for building LLDB test inferiors")
+
+  if (("${LLDB_TEST_C_COMPILER}" STREQUAL "") OR
+      ("${LLDB_TEST_CXX_COMPILER}" STREQUAL ""))
+    message(FATAL_ERROR "LLDB test compilers not specified.  Tests will not run")
+  endif()
+
   add_subdirectory(test)
   add_subdirectory(unittests)
   add_subdirectory(lit)
 endif()
 
 if (NOT LLDB_DISABLE_PYTHON)
     # Add a Post-Build Event to copy over Python files and create the symlink
     # to liblldb.so for the Python API(hardlink on Windows)

lit/CMakeLists.txt

 set(LLVM_SOURCE_DIR "${LLVM_MAIN_SRC_DIR}")
 set(LLVM_BINARY_DIR "${LLVM_BINARY_DIR}")
 set(LLVM_BUILD_MODE "%(build_mode)s")
 set(LLVM_TOOLS_DIR "${LLVM_TOOLS_BINARY_DIR}/%(build_config)s")
 set(LLVM_LIBS_DIR "${LLVM_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX}/%(build_config)s")
 set(CLANG_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/..")
 set(CLANG_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/..")
 if(BUILD_SHARED_LIBS)
   set(ENABLE_SHARED 1)
 else()
   set(ENABLE_SHARED 0)
 endif(BUILD_SHARED_LIBS)
 
-option(LLDB_TEST_CLANG "Use in-tree clang when testing lldb" Off)
-set(LLDB_TEST_C_COMPILER "" CACHE STRING "C compiler to use when testing LLDB")
-set(LLDB_TEST_CXX_COMPILER "" CACHE STRING "C++ compiler to use when testing LLDB")
-
 configure_lit_site_cfg(
   ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.in
   ${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg)
 configure_lit_site_cfg(
   ${CMAKE_CURRENT_SOURCE_DIR}/Unit/lit.site.cfg.in
   ${CMAKE_CURRENT_BINARY_DIR}/Unit/lit.site.cfg
   )
 

 if (TARGET lldb-server)
   list(APPEND LLDB_TEST_DEPS lldb-server)
 endif()
   
 if(APPLE)
   list(APPEND LLDB_TEST_DEPS debugserver)
 endif()
 
-if(LLDB_TEST_CLANG)
-  if(LLDB_TEST_C_COMPILER OR LLDB_TEST_CXX_COMPILER)
-    message(SEND_ERROR "Cannot override LLDB_TEST_<LANG>_COMPILER and set LLDB_TEST_CLANG.")
-  endif()
+if(TARGET clang)
   list(APPEND LLDB_TEST_DEPS clang)
 endif()
 
 set(LLDB_TEST_PARAMS
   lldb_site_config=${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg
   )
 
 add_lit_testsuite(check-lldb-lit "Running lldb lit test suite"

lit/lit.site.cfg.in

 @LIT_SITE_CFG_IN_HEADER@
 
 config.llvm_src_root = "@LLVM_SOURCE_DIR@"
 config.llvm_obj_root = "@LLVM_BINARY_DIR@"
 config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"
 config.llvm_libs_dir = "@LLVM_LIBS_DIR@"
 config.lit_tools_dir = "@LLVM_LIT_TOOLS_DIR@"
 config.lldb_obj_root = "@LLDB_BINARY_DIR@"
 config.lldb_libs_dir = "@LLVM_LIBRARY_OUTPUT_INTDIR@"
 config.lldb_tools_dir = "@LLVM_RUNTIME_OUTPUT_INTDIR@"
 config.target_triple = "@TARGET_TRIPLE@"
 config.python_executable = "@PYTHON_EXECUTABLE@"
-config.cc = "@CMAKE_C_COMPILER@"
-config.cxx = "@CMAKE_CXX_COMPILER@"
-
-test_c_compiler = "@LLDB_TEST_C_COMPILER@"
-test_cxx_compiler = "@LLDB_TEST_CXX_COMPILER@"
-test_clang = "@LLDB_TEST_CLANG@".lower()
-test_clang = test_clang == "on" or test_clang == "true" or test_clang == "1"
-
-if len(test_c_compiler) > 0:
-  config.cc = test_c_compiler
-if len(test_c_compiler) > 0:
-  config.cxx = test_cxx_compiler
-
-if test_clang:
-  config.cc = 'clang'
-  config.cxx = 'clang++'
+config.cc = "@LLDB_TEST_C_COMPILER@"
+config.cxx = "@LLDB_TEST_CXX_COMPILER@"
 
 # Support substitution of the tools and libs dirs with user parameters. This is
 # used when we can't determine the tool dir at configuration time.
 try:
     config.llvm_tools_dir = config.llvm_tools_dir % lit_config.params
     config.llvm_libs_dir = config.llvm_libs_dir % lit_config.params
 except KeyError as e:
     key, = e.args
     lit_config.fatal("unable to find %r parameter, use '--param=%s=VALUE'" % (key,key))
 
 # Let the main config do the real work.
 lit_config.load_config(config, "@LLDB_SOURCE_DIR@/lit/lit.cfg")

test/CMakeLists.txt

 if(TARGET debugserver)
   list(APPEND LLDB_TEST_DEPS debugserver)
 endif()
 
 if(TARGET lldb-mi)
   list(APPEND LLDB_TEST_DEPS lldb-mi)
 endif()
 
-if ("${LLDB_TEST_COMPILER}" STREQUAL "")
-    string(REGEX REPLACE ".*ccache\ +" "" LLDB_TEST_COMPILER ${CMAKE_C_COMPILER} ${CMAKE_C_COMPILER_ARG1})
-endif()
-
 # The default architecture with which to compile test executables is the default LLVM target
 # architecture, which itself defaults to the host architecture.
 string(TOLOWER "${LLVM_TARGET_ARCH}" LLDB_DEFAULT_TEST_ARCH)
 if( LLDB_DEFAULT_TEST_ARCH STREQUAL "host" )
   string(REGEX MATCH "^[^-]*" LLDB_DEFAULT_TEST_ARCH ${LLVM_HOST_TRIPLE})
 endif ()
 
 # Allow the user to override the default by setting LLDB_TEST_ARCH
 set(LLDB_TEST_ARCH
   ${LLDB_DEFAULT_TEST_ARCH}
   CACHE STRING "Specify the architecture to run LLDB tests as (x86|x64).  Determines whether tests are compiled with -m32 or -m64")
 
-if(LLDB_TEST_CLANG)
-  set(LLDB_TEST_COMPILER $<TARGET_FILE:clang>)
-endif()
-
 # Users can override LLDB_TEST_USER_ARGS to specify arbitrary arguments to pass to the script
 set(LLDB_TEST_USER_ARGS
   ""
   CACHE STRING "Specify additional arguments to pass to test runner. For example: '-C gcc -C clang -A i386 -A x86_64'")
 
 set(LLDB_TEST_COMMON_ARGS
   --arch=${LLDB_TEST_ARCH}
   --executable $<TARGET_FILE:lldb>
   -s
   ${CMAKE_BINARY_DIR}/lldb-test-traces
   -S nm
   -u CXXFLAGS
   -u CFLAGS
-  -C ${LLDB_TEST_COMPILER}
+  -C ${LLDB_TEST_C_COMPILER}
   )
 
 if ( CMAKE_SYSTEM_NAME MATCHES "Windows" )
   # All tests are currently flaky on Windows, so rerun them all once when they fail.
   set(LLDB_TEST_COMMON_ARGS ${LLDB_TEST_COMMON_ARGS} --rerun-all-issues)
   
   set(LLDB_TEST_DEBUG_TEST_CRASHES
     0