[CMake] Phase out LLDB_TEST_C/CXX_COMPILER in favor of single LLDB_TEST_COMPILER

[sgraenitz]

Summary

There was a previous attempt to do this in D39215. Back then the LLDB_TEST_CXX_COMPILER was apparently still in use. Today it's unused and we could use the chance and finish the work.

Following up from D56400:

• add LLDB_TEST_COMPILER setting, while still supporting LLDB_TEST_C/CXX_COMPILER
• remove LLDB_TEST_USE_CUSTOM_C/CXX_COMPILER: overriding LLDB_TEST_COMPILER alone is sufficient
• use LLDB_TEST_COMPILER_IS_DEFAULT to determine whether or not to replace configuration names
• update documentation to reflect changes

As we import targets like clang and dsymutil anyway, we might use generator expressions to fetch paths of the required tools.
The problem is not that generator expressions don't work in the multi-config case. We just need a generation-time step for each lit.site.cfg (or at least those that use LLDB_DOTEST_ARGS). That may like this:
https://github.com/llvm-mirror/lldb/blob/5febeff671748655213b74bbd71e7d2efc1a9efe/test/CMakeLists.txt#L169

[sgraenitz]

In test/CMakeLists.txt pass on LLDB_TEST_COMPILER_USED instead of LLDB_TEST_COMPILER

[sgraenitz]

Remove remove unused LLDB_HAVE_LLD and ENABLE_SHARED in lit/CMakeLists.txt

[JDevlieghere]

It looks like LLDB_TEST_COMPILER_IS_DEFAULT is set but never read. Why do we need it exactly?

[sgraenitz]

In D56440#1349892, @JDevlieghere wrote:

It looks like LLDB_TEST_COMPILER_IS_DEFAULT is set but never read. Why do we need it exactly?

Right, good you found that.

I first used it to determine whether the replacement in LLDB_TEST_C/CXX_COMPILER needs to be done in lit/CMakeLists.txt, but then it turned out the result is unused (since D54567, which is another verbose piece of history).
For reference please see: https://reviews.llvm.org/rL347216#change-H2HV4zA8ol05

It basically does what LLDB_TEST_USE_CUSTOM_C/CXX_COMPILER so far pretended to do (but in fact they were both broken). It somehow might makes sense to keep it as long as we have the manual string replacements in the dotest CMakeLists, even though the compiler paths are not used there (surprisingly). I will check why dotest doesn't need them and either fix it (which adds a use case) or remove it.

[stella.stamenova]

lit/CMakeLists.txt

14

Your change is removing the logic to setup this path correctly. This will cause failures in the tests.

41

It is better to do this in a separate change (the shared libs also) since they are not related to the compiler properties

[sgraenitz]

Move BUILD_SHARED_LIBS and LLDB_HAVE_LLD to a separate commit

[sgraenitz]

CMakeLists.txt

73

Note: Using a different variable here to avoid force-overwriting the cached value of LLDB_TEST_COMPILER. If we did that, it would appear as if it was set explicitly by the user, when re-running CMake. Doing it this way (and leaving the default empty) allows to distinguish an explicit setting from the default. LLDB_TEST_COMPILER_IS_DEFAULT tracks this info but is currently not used.

lit/CMakeLists.txt

14

That should be fine. It only changed the value of LLDB_TEST_C/CXX_COMPILER in the scope of this CMakeLists.txt/directory. The value in the parent CMakeLists.txt as well as the one in the cache remained unchanged.

Thus it is unused since https://reviews.llvm.org/rC347216#change-H2HV4zA8ol05 removed their usage from lit/lit.site.cfg.py.in that gets configured below. Before this change the local value was passed to the config file like this:

config.cc = "@LLDB_TEST_C_COMPILER@"
config.cxx = "@LLDB_TEST_CXX_COMPILER@"

They seem to be inferred via LLDB_LIT_TOOLS_DIR now.

[stella.stamenova]

CMakeLists.txt

61

If it's true that we don't need the CMAKE_CFG_INTDIR replacement in the test cmake file, then is this comment still correct?

lit/CMakeLists.txt

14

Have you tested it with a tool set that supports multiple configurations to make sure that is the case? I believe XCode and VS both support multiple configurations.

[xiaobai]

CMakeLists.txt

72

Is this check needed? compiler_used should be set always here because of the way the above block is written.

[sgraenitz]

In D56440#1351033, @sgraenitz wrote:

In D56440#1349892, @JDevlieghere wrote:

It looks like LLDB_TEST_COMPILER_IS_DEFAULT is set but never read. Why do we need it exactly?

[...] I will check why dotest doesn't need them and either fix it (which adds a use case) or remove it.

It's again more complicated than I had hoped. As explained in the inline comment, we should not do the CMAKE_CFG_INTDIR replacement in LLDB_TEST_COMPILER if it's out-of-tree. However, the value currently ends up in LLDB_DOTEST_ARGS like all the other arguments. The replacement then is applied for all of them together. I think fixing this workaround doesn't make sense. We should either leave it broken as is or we do it right with generator expressions.

For this review: I would leave it as is. I will polish the comments and remove LLDB_TEST_COMPILER_IS_DEFAULT.

CMakeLists.txt

61

Maybe my comments should explain the situation in more detail.

We do need the CMAKE_CFG_INTDIR replacement as long as we set the paths for test-related binaries at configuration time. This is what currently happens above for the lldb driver, dsymutil, FileCheck and the C/C++ compiler. Ideally we wouldn't need to do that, but it's another story.

We need to do the replacement for LLDB_TEST_COMPILER if and only if we use the in-tree clang. If the user passed a custom path, we don't want to touch it. This is broken in the current state. So far my commit doesn't change it for the better or worse, but it shows that the check is possible with LLDB_TEST_COMPILER_IS_DEFAULT.

72

You are right that compiler_used is always set at this point. However, the exact condition is if compiler_used not empty and this can be false, if all of LLDB_TEST_(C_/CXX_)COMPILER have their default values "" and LLDB_DEFAULT_TEST_COMPILER was not set (and therefore is empty too).

We run into the below warning, if the user didn't pass an explicit value for the test compiler and the clang target doesn't exist. The latter is the case if clang is missing altogether in our source tree (for in-tree builds) or in the build-tree we build against (for standalone builds).

In principle we should be able to build a subset of targets without clang (e.g. debugserver on darwin), but it's one more thing that is broken currently: a hack in AddLLDB.cmake adds clang-tablegen-targets as a dependency to all library targets. At some point it was configurable which clang-tblgen to use, but the respective CLANG_TABLEGEN parameter today only exists in the docs:
http://lldb.llvm.org/build.html

I think with a monorepo this will/would become much simpler: we either have all of llvm/clang/libcxx or none. The CMake code should be cleaned up once that's the default.

lit/CMakeLists.txt

14

Yes, tested with Xcode.

[sgraenitz]

Closing this in favor of a more comprehensive fix.

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_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.
   set(LLDB_DEFAULT_TEST_EXECUTABLE "${LLVM_RUNTIME_OUTPUT_INTDIR}/lldb${CMAKE_EXECUTABLE_SUFFIX}")
 
   # 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(TARGET clang)
     set(LLDB_DEFAULT_TEST_COMPILER "${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/clang${CMAKE_EXECUTABLE_SUFFIX}")
   endif()
 
+  # Phase out these settings. We only support clang, so we should have only one setting.
+  # dotest has its own logic to deal with C/C++ driver variants.
+  set(LLDB_TEST_C_COMPILER "" CACHE PATH "C Compiler to use for building LLDB test inferiors (deprecated)")
+  set(LLDB_TEST_CXX_COMPILER "" CACHE PATH "C++ Compiler to use for building LLDB test inferiors (deprecated)")
+
+  set(LLDB_TEST_COMPILER "" CACHE PATH "Compiler to use for building LLDB test inferiors (defaults to builtin clang)")
   set(LLDB_TEST_EXECUTABLE "${LLDB_DEFAULT_TEST_EXECUTABLE}" CACHE PATH "lldb executable used for testing")
-  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((NOT LLDB_TEST_C_COMPILER) OR (NOT LLDB_TEST_CXX_COMPILER))
-    message(WARNING "LLDB test compilers not specified. Tests will not run.")
+  # Use LLDB_TEST_COMPILER_IS_DEFAULT to determine whether or not to replace
+  # CMAKE_CFG_INTDIR with LLVM_BUILD_MODE for dotest.
+  if(LLDB_TEST_COMPILER)
+    set(compiler_used ${LLDB_TEST_COMPILER})
+  elseif(LLDB_TEST_C_COMPILER)
+    set(compiler_used ${LLDB_TEST_C_COMPILER})
+  elseif(LLDB_TEST_CXX_COMPILER)
+    set(compiler_used ${LLDB_TEST_CXX_COMPILER})
+  else()
+    set(LLDB_TEST_COMPILER_IS_DEFAULT ON)
+    set(compiler_used ${LLDB_DEFAULT_TEST_COMPILER})
   endif()
+  if (compiler_used)
+    set(LLDB_TEST_COMPILER_USED ${compiler_used})
+  else()
+    message(WARNING "LLDB test compiler not specified. Tests will not run.")
+  endif()
+
+  unset(LLDB_TEST_C_COMPILER CACHE)
+  unset(LLDB_TEST_CXX_COMPILER CACHE)
 
   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}")
-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}")
-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}")
 
 list(APPEND LLDB_TEST_DEPS
   LLDBUnitTests
   dsymutil
   llc
   lldb
   lldb-test
   llvm-config
   llvm-mc
   llvm-objcopy
   )
 
 if(TARGET lld)
   list(APPEND LLDB_TEST_DEPS lld)
   set(LLDB_HAVE_LLD 1)
 else()
   set(LLDB_HAVE_LLD 0)
 endif()
 
 if(BUILD_SHARED_LIBS)
   set(ENABLE_SHARED 1)
 else()
   set(ENABLE_SHARED 0)

test/CMakeLists.txt

set(LLDB_TEST_COMMON_ARGS
   -u CXXFLAGS
   -u CFLAGS
   )
 
 list(APPEND LLDB_TEST_COMMON_ARGS
   --executable ${LLDB_TEST_EXECUTABLE}
   --dsymutil ${LLDB_TEST_DSYMUTIL}
   --filecheck ${LLDB_TEST_FILECHECK}
-  -C ${LLDB_TEST_C_COMPILER}
+  -C ${LLDB_TEST_COMPILER_USED}
   )
 
 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

www/build.html

<div id="content">
                 <b>LLDB_RELOCATABLE_PYTHON</b> (Default=0): When this is 0, LLDB will bind statically to the location specified
                 in the PYTHON_HOME CMake variable, ignoring any value of PYTHONHOME set in the environment.  This is most useful for
                 developers who simply want to run LLDB after they build it.  If you wish to move a build of LLDB to a different
                 machine where Python will be in a different location, setting LLDB_RELOCATABLE_PYTHON to 1 will cause Python to
                 use its default mechanism for finding the python installation at runtime (looking for installed Pythons, or using
                 the PYTHONHOME environment variable if it is specified).
               </li>
               <li>
-                <b>LLDB_TEST_C_COMPILER</b> or <b>LLDB_TEST_CXX_COMPILER</b>: The test suite needs to be able to find a copy of clang.exe
+                <b>LLDB_TEST_COMPILER</b>: The test suite needs to be able to find a copy of clang.exe
                 that it can use to compile inferior programs.  Note that MSVC is not supported here, it <strong>must</strong> be a path to a
                 clang executable.  Note that using a release clang.exe is strongly recommended here, as it will make the test suite run much faster.
-                This can be a path to any recent clang.exe, including one you built yourself.  These variables are ignored unless the respective
-                <strong>LLDB_TEST_USE_CUSTOM_C_COMPILER</strong> and <strong>LLDB_TEST_USE_CUSTOM_CXX_COMPILER</strong> are set to ON.
+                This can be a path to any recent clang.exe, including one you built yourself.
               </li>
             </ul>
             Sample command line:<br/>
-            <code>cmake -G Ninja -DLLDB_TEST_DEBUG_TEST_CRASHES=1 -DPYTHON_HOME=C:\Python35 -DLLDB_TEST_USE_CUSTOM_C_COMPILER=ON -DLLDB_TEST_C_COMPILER=d:\src\llvmbuild\ninja_release\bin\clang.exe ..\..\llvm</code>
+            <code>cmake -G Ninja -DLLDB_TEST_DEBUG_TEST_CRASHES=1 -DPYTHON_HOME=C:\Python35 -DLLDB_TEST_COMPILER=d:\src\llvmbuild\ninja_release\bin\clang.exe ..\..\llvm</code>
             <h2>Working with both Ninja and MSVC</h2>
             <p>
               Compiling with <code>ninja</code> is both faster and simpler than compiling with MSVC, but chances are you still want
               to debug LLDB with MSVC (at least until we can debug LLDB on Windows with LLDB!).  One solution to this is to run 
               <code>cmake</code> twice and generate the output into two different folders.  One for compiling (the <code>ninja</code>
               folder), and one for editing / browsing / debugging (the MSVC folder).
             </p>
             <p>

www/test.html

<div id="content">
               <strong>Windows Note</strong>: In the examples that follow, any invocations of <code>python</code>
               should be replaced with <code>python_d</code>, the debug interpreter, when running the test
               suite against a debug version of LLDB.
             </p>
             <p>
               The easiest way to run the LLDB test suite is to use the <tt>check-lldb</tt> build
               target. By default, the <tt>check-lldb</tt> target builds the test programs with
               the same compiler that was used to build LLDB. To build the tests with a different
-              compiler, you can set the <strong>LLDB_TEST_C_COMPILER</strong> or the <strong>LLDB_TEST_CXX_COMPILER</strong> CMake variables.
-              These variables are ignored unless the respective <strong>LLDB_TEST_USE_CUSTOM_C_COMPILER</strong> and
-              <strong>LLDB_TEST_USE_CUSTOM_CXX_COMPILER</strong> are set to ON.
+              compiler, you can set the <strong>LLDB_TEST_COMPILER</strong> CMake variable.
               It is possible to customize the architecture of the test binaries and compiler used by appending -A
               and -C options respectively to the CMake variable <strong>LLDB_TEST_USER_ARGS</strong>. For
               example, to test LLDB against 32-bit binaries
               built with a custom version of clang, do:
             </p>
             <code>
               <br />&gt; cmake -DLLDB_TEST_USER_ARGS="-A i386 -C /path/to/custom/clang" -G Ninja
               <br />&gt; ninja check-lldb