Page MenuHomePhabricator

Default to using in-tree clang for building test executables
ClosedPublic

Authored by labath on Oct 23 2017, 4:34 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Oct 23 2017, 4:34 PM
zturner edited edge metadata.Oct 23 2017, 4:38 PM

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 accepted this revision.Oct 23 2017, 4:38 PM

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

Thanks!

This revision is now accepted and ready to land.Oct 23 2017, 4:38 PM

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

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.

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.

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.

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'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 added a comment.EditedOct 23 2017, 6:15 PM

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.

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.

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 added a subscriber: ted.Oct 24 2017, 8:58 AM

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.

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 updated this revision to Diff 120102.Oct 24 2017, 11:06 AM

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.

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?

This revision was automatically updated to reflect the committed changes.