[Lldb-commits] [PATCH] D56440: [CMake] Phase out LLDB_TEST_C/CXX_COMPILER in favor of single LLDB_TEST_COMPILER

Thu Jan 10 08:55:40 PST 2019

sgraenitz marked 6 inline comments as done.
sgraenitz added a comment.

In D56440#1351033 <https://reviews.llvm.org/D56440#1351033>, @sgraenitz wrote:

> In D56440#1349892 <https://reviews.llvm.org/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`.

================
Comment at: CMakeLists.txt:61
+  # 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)
----------------
stella.stamenova wrote:
> 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?
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`.

================
Comment at: CMakeLists.txt:72
   endif()
+  if (compiler_used)
+    set(LLDB_TEST_COMPILER_USED ${compiler_used})
----------------
xiaobai wrote:
> Is this check needed? `compiler_used` should be set always here because of the way the above block is written.
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.

================
Comment at: lit/CMakeLists.txt:14

-if (NOT LLDB_TEST_USE_CUSTOM_C_COMPILER)
-  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_C_COMPILER "${LLDB_TEST_C_COMPILER}")
----------------
stella.stamenova wrote:
> sgraenitz wrote:
> > stella.stamenova wrote:
> > > Your change is removing the logic to setup this path correctly. This will cause failures in the tests.
> > 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.
> 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.
Yes, tested with Xcode.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56440/new/

https://reviews.llvm.org/D56440