Page MenuHomePhabricator

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

Authored by sgraenitz on Jan 8 2019, 7:01 AM.

Details

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

Event Timeline

sgraenitz created this revision.Jan 8 2019, 7:01 AM
sgraenitz updated this revision to Diff 180662.Jan 8 2019, 7:06 AM

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

sgraenitz edited the summary of this revision. (Show Details)Jan 8 2019, 7:15 AM
sgraenitz updated this revision to Diff 180670.Jan 8 2019, 7:27 AM

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

sgraenitz edited the summary of this revision. (Show Details)Jan 8 2019, 8:23 AM

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

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 requested changes to this revision.Jan 9 2019, 9:42 AM
stella.stamenova added inline comments.
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

This revision now requires changes to proceed.Jan 9 2019, 9:42 AM
sgraenitz updated this revision to Diff 180890.Jan 9 2019, 11:26 AM
sgraenitz marked 5 inline comments as done.

Move BUILD_SHARED_LIBS and LLDB_HAVE_LLD to a separate commit

sgraenitz added inline comments.Jan 9 2019, 11:27 AM
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.

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 added inline comments.
CMakeLists.txt
72

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

sgraenitz marked 6 inline comments as done.Jan 10 2019, 8:53 AM

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 abandoned this revision.Jan 11 2019, 10:34 AM
sgraenitz marked 3 inline comments as done.

Closing this in favor of a more comprehensive fix.