[Lldb-commits] [PATCH] D43096: [lit] Update how clang and other binaries are found in per-configuration directories

Stella Stamenova via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Feb 8 20:51:15 PST 2018


stella.stamenova added inline comments.


================
Comment at: lit/CMakeLists.txt:10-13
+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_CXX_COMPILER ${LLDB_TEST_CXX_COMPILER})
+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})
----------------
stella.stamenova wrote:
> zturner wrote:
> > This only works if you're using a just-built clang, which might not be the case.  In fact, it's usually not the case, because it's common to want to run the test suite against a debug build of lldb but using a release build of clang (otherwise you'll be there all day waiting for it to finish).
> > 
> > I feel like if the user specifies an absolute path to the test compiler on the command line, that should be what it uses -- always.  If we want to use a just built toolchain, maybe we need something else, like `-DLLDB_TEST_BUILT_CLANG=ON`, which would trigger this logic.
> > 
> > As I don't use this configuration though, I'm interested in hearing your thoughts.
> It actually does work in the case when a user specifies a compiler on the command line as well as when the just-built clang is used and the default today is to use the just-built clang.
> 
> As far as I can tell, you can specify the compiler with LLDB_TEST_C_COMPILER (or LLDB_TEST_CXX_COMPILER) when calling CMake or by passing a value to the tests directly with -C. I assume that you are concerned about the first case - when passing the property to CMake?
> 
> In that case LLDB_TEST_C_COMPILER is set to /path/to/compiler and these lines won't actually affect it - unless for some reason the path contained ${CMAKE_CFG_INTDIR}. If ${CMAKE_CFG_INTDIR} is a period, which is the likely scenario, it will just be replaced by another period since the build mode would be the same.
> 
> The only case when it might not work is if ${CMAKE_CFG_INTDIR} is in the path but not a period - but that is unlikely since the scenario would mean that ${CMAKE_CFG_INTDIR} is, say, $Configuration and the path that the user specified contains $Configuration AND it is different than the one they're using for LLDB.
> 
> On the other hand, without this change it is not possible to use the just-built compiler when using Visual Studio, for example, because the path to the just built compiler is not being set correctly and this particular substitution needs to happen here.
> 
> The way to make sure that both scenarios work always is to guard against the substitution when the user explicitly specifies a path or to enforce the sibstitution when they're using the just-built clang so a property like LLDB_TEST_BUILD_CLANG would work. If you think that the error scenario is likely enough, then we should add the property.
> 
Actually, I misspoke. There is another way to do it without an extra property that the user has to pass.

Right now, in the main CMakeLists.txt for LLDB, we create a property LLDB_DEFAULT_TEST_C_COMPILER which overwrites LLDB_TEST_C_COMPILER if LLDB_TEST_C_COMPILER is not set by the user. We could at that point first check whether LLDB_TEST_C_COMPILER was explicitly set and internally create a property to tell us whether to overwrite later. I think that may actually be better since it maintains the current behavior and there's no need for a new explicit property.

Thoughts?


https://reviews.llvm.org/D43096





More information about the lldb-commits mailing list