[CMake] Some cleanup around test preparations

[sgraenitz]

Summary

Fix some issues in the code around D56399:

• remove outdated comments
• streamline empty-string conditions
• remove 2 uses of LLDB_TEST_USE_CUSTOM_C(XX)_COMPILER -- there are two more in lit/CMakeLists.txt that should follow
• turn FATAL_ERROR no test compilers into WARNING (plus add quotes around uses in lit/CMakeLists.txt)

[sgraenitz]

CMakeLists.txt

45

I think these would be all good to fix, but I am not sure I can do it now. Should we keep remaining ones as FIXME?
Currently investigating the LLVM_BINARY_DIR issue.

[sgraenitz]

CMakeLists.txt

42

This was here already when the file was created:
https://github.com/llvm-mirror/lldb/blob/98c187f2613fd8e2f5cdf5f8f35503a102a54f11/cmake/modules/LLDBStandalone.cmake#L67

Apparently the below default paths for dsymutil, FileCheck and clang never worked in standalone builds..

[JDevlieghere]

CMakeLists.txt

41

Why is this unused? Are we only using LLDB_TEST_C_COMPILER and if so, why?

42

Instead of this FIXME, should we explicitly unset this variable to prevent mistakes in the future? Or is this fixable?

[labath]

This looks fine to me. I recommend including Stella in changes relating to this, as I recall some subtleties here wrt. multi-config (e.g., MSVC) generators.

CMakeLists.txt

41

I think that's because this only gets passed to dotest, that it has it's own logic for deducing the C++ compiler name.

[sgraenitz]

CMakeLists.txt

41

Yes, we only ever pass on LLDB_TEST_C_COMPILER and it should be sufficient.
https://github.com/llvm-mirror/lldb/blob/2b03c2512ebf1999e330e85d60b7f93703483485/test/CMakeLists.txt#L53

https://lldb.llvm.org/build.html reads: Note that MSVC is not supported here, it must be a path to a clang executable.
As clang == clang++ ⇒ q.e.d :)

I think what we really want here is a LLDB_TEST_COMPILER setting, but this means: update docs and build bots, potentially phase out the setting? I am not sure about their popularity.

42

Good point. Not sure what the consequences would be. I will try and see what happens.

[sgraenitz]

Act on FIXME's in subsequent commits and remove comments here

[sgraenitz]

CMakeLists.txt

41

There was a previous initiative for cleanup in D39215. My follow-up patch D56440 aims to finish this.

42

See followup patch D56443.

[stella.stamenova]

CMakeLists.txt

49

Nit: it looks like your if statements lack a space before the parenthesis, while all the existing ones have a space. Please be consistent with the existing convention.

54

You are making a significant change to the compiler selection logic in your second change. I think it would be better to not make any changes here in order to have all of them in a single change.

[sgraenitz]

I will split off the "dead code elimination" part and close both reviews in favor of a more comprehensive fix.

CMakeLists.txt

49

That doesn't seem to be the most popular convention ;-)
BTW do we have documentation for CMake coding style?

CMakeLists.txt

 
 add_subdirectory(docs)
 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})
+option(LLDB_INCLUDE_TESTS "Generate build targets for the LLDB unit tests." ${LLVM_INCLUDE_TESTS})
 option(LLDB_TEST_USE_CUSTOM_C_COMPILER "Use the C compiler provided via LLDB_TEST_C_COMPILER for building test inferiors (instead of the just-built compiler). Defaults to OFF." OFF)
 option(LLDB_TEST_USE_CUSTOM_CXX_COMPILER "Use the C++ compiler provided via LLDB_TEST_CXX_COMPILER for building test inferiors (instead of the just-built compiler). Defaults to OFF." OFF)
-if(LLDB_INCLUDE_TESTS)
 
-  # Set the path to the default lldb test executable. Make the path relative to
-  # LLVM_RUNTIME_OUTPUT_INTDIR: this will be correct even when LLVM and LLDB
-  # have separate binary directories.
+if(LLDB_INCLUDE_TESTS)
+  # Set the path to the default lldb test executable.
   set(LLDB_DEFAULT_TEST_EXECUTABLE "${LLVM_RUNTIME_OUTPUT_INTDIR}/lldb${CMAKE_EXECUTABLE_SUFFIX}")
 
-  # Set the paths to default llvm tools. Make these paths relative to the LLVM
-  # binary directory.
+  # Set the paths to default llvm tools.
   set(LLDB_DEFAULT_TEST_DSYMUTIL "${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/dsymutil${CMAKE_EXECUTABLE_SUFFIX}")
   set(LLDB_DEFAULT_TEST_FILECHECK "${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/FileCheck${CMAKE_EXECUTABLE_SUFFIX}")
-
-  if (NOT LLDB_TEST_USE_CUSTOM_C_COMPILER AND TARGET clang)
-    set(LLDB_DEFAULT_TEST_C_COMPILER "${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/clang${CMAKE_EXECUTABLE_SUFFIX}")
-  else()
-    set(LLDB_DEFAULT_TEST_C_COMPILER "")
-  endif()
-
-  if (NOT LLDB_TEST_USE_CUSTOM_CXX_COMPILER AND TARGET clang)
-    set(LLDB_DEFAULT_TEST_CXX_COMPILER "${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/clang++${CMAKE_EXECUTABLE_SUFFIX}")
-  else()
-    set(LLDB_DEFAULT_TEST_CXX_COMPILER "")
+  if(TARGET clang)
+    set(LLDB_DEFAULT_TEST_COMPILER "${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/clang${CMAKE_EXECUTABLE_SUFFIX}")
   endif()
 
   set(LLDB_TEST_EXECUTABLE "${LLDB_DEFAULT_TEST_EXECUTABLE}" CACHE PATH "lldb executable used for testing")
-  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")
+  set(LLDB_TEST_C_COMPILER "${LLDB_DEFAULT_TEST_COMPILER}" CACHE PATH "C Compiler to use for building LLDB test inferiors")
+  set(LLDB_TEST_CXX_COMPILER "${LLDB_DEFAULT_TEST_COMPILER}" CACHE PATH "C++ Compiler to use for building LLDB test inferiors")
   set(LLDB_TEST_DSYMUTIL "${LLDB_DEFAULT_TEST_DSYMUTIL}" CACHE PATH "dsymutil used for generating dSYM bundles")
   set(LLDB_TEST_FILECHECK "${LLDB_DEFAULT_TEST_FILECHECK}" CACHE PATH "FileCheck used for testing purposes")
 
-  if (("${LLDB_TEST_C_COMPILER}" STREQUAL "") OR
-      ("${LLDB_TEST_CXX_COMPILER}" STREQUAL ""))
-    message(FATAL_ERROR "LLDB test compilers not specified.  Tests will not run")
+  if((NOT LLDB_TEST_C_COMPILER) OR (NOT LLDB_TEST_CXX_COMPILER))
+    message(WARNING "LLDB test compilers not specified. Tests will not run.")
   endif()
 
   set(LLDB_TEST_DEPS lldb)
 
   # darwin-debug is an hard dependency for the testsuite.
   if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
     list(APPEND LLDB_TEST_DEPS darwin-debug)
   endif()

lit/CMakeLists.txt

 # Test runner infrastructure for LLDB. This configures the LLDB test trees
 # for use by Lit, and delegates to LLVM's lit test handlers.
 
 if (CMAKE_CFG_INTDIR STREQUAL ".")
   set(LLVM_BUILD_MODE ".")
 else ()
   set(LLVM_BUILD_MODE "%(build_mode)s")
 endif ()
 
 if (CMAKE_SIZEOF_VOID_P EQUAL 8)
   set(LLDB_IS_64_BITS 1)
 endif()
 
 if (NOT LLDB_TEST_USE_CUSTOM_C_COMPILER)
-  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_C_COMPILER ${LLDB_TEST_C_COMPILER})
+  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_C_COMPILER "${LLDB_TEST_C_COMPILER}")
 endif ()
 
 if (NOT LLDB_TEST_USE_CUSTOM_CXX_COMPILER)
-  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_CXX_COMPILER ${LLDB_TEST_CXX_COMPILER})
+  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_CXX_COMPILER "${LLDB_TEST_CXX_COMPILER}")
 endif ()
 
 get_property(LLDB_DOTEST_ARGS GLOBAL PROPERTY LLDB_DOTEST_ARGS_PROPERTY)
 
 string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_LIBS_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
 string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TOOLS_DIR ${LLVM_RUNTIME_OUTPUT_INTDIR})
 string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_DOTEST_ARGS "${LLDB_DOTEST_ARGS}")