Differential D71695
[cmake] Include the llvm-locstats target when utils and tools are not being built. Authored by DavidSpickett on Dec 19 2019, 2:29 AM.
Details This was uncovered by: https://reviews.llvm.org/D71611 Previously the build target was only added if you
Diff Detail
Event TimelineComment Actions Hm...actually...this is not good.. The llvm-locstats tool depends on the llvm-dwarfdump tool. This can not be run if the llvm-dwarfdump is not not being built. Comment Actions Sorry for the confusion. May be the llvm/CMakeLists.txt is better place to address this? llvm/CMakeLists.txt: if (LLVM_INCLUDE_UTILS AND LLVM_INCLUDE_TOOLS) add_subdirectory(utils/llvm-locstats) endif() Comment Actions The llvm-dwarfdump tool target will still be there. Unless that also has this BUILD vs INCLUDE issue. There's two variables at play here: My expectation is that INCLUDE means the build targets are there but if BUILD is off they're not actually built unless you use their name. That makes sense to me, but I'm not super familiar with it. Comment Actions Changing it to: if (LLVM_INCLUDE_UTILS AND LLVM_INCLUDE_TOOLS AND LLVM_BUILD_TOOLS) add_subdirectory(utils/llvm-locstats) Doesn't help the issue, but maybe I'm missing your point here? Comment Actions Yes that was my point. Should we add the AND LLVM_BUILD_TOOLS part in both of the llvm/CMakeLists.txt and llvm/utils/llvm-locstats/CMakeLists.txt? Comment Actions Ah I see. Well, I think the build target existing even if it's not added to "all" is fine. Which is what BUILD_TOOLS controls: macro(add_llvm_tool name)
if( NOT LLVM_BUILD_TOOLS )
set(EXCLUDE_FROM_ALL ON)
endif()What could trip someone up is: $ cmake -G Ninja ../llvm-project/llvm/ -DLLVM_BUILD_TOOLS=OFF $ ninja llvm-locstats [1/1] Copying llvm-locstats into /work/open_source/llvm-project-build/./bin $ bin/llvm-locstats <stuff> Since building llvm-locstats doesn't implicitly build llvm-dwarfdump. Is that one of the scenarios you were thinking of? Comment Actions This won't solve the problem you reported.. The problem with this patch is that the llvm-locstats will be created even the llvm-dwarfdump is not being built. The llvm-locstats tool is using JSON generated by llvm-dwarfdump --statistics and there is no point to create the tool if we did not make the dwarfdump. Comment Actions Something like the diff bellow would address the concerns. diff --git a/llvm/utils/llvm-locstats/CMakeLists.txt b/llvm/utils/llvm-locstats/CMakeLists.txt index d5366f99050..82099fed314 100644 --- a/llvm/utils/llvm-locstats/CMakeLists.txt +++ b/llvm/utils/llvm-locstats/CMakeLists.txt @@ -2,6 +2,7 @@ if (LLVM_INCLUDE_UTILS AND LLVM_INCLUDE_TOOLS) add_custom_command( OUTPUT ${LLVM_TOOLS_BINARY_DIR}/llvm-locstats DEPENDS ${LLVM_MAIN_SRC_DIR}/utils/llvm-locstats/llvm-locstats.py + DEPENDS llvm-dwarfdump COMMAND ${CMAKE_COMMAND} -E copy ${LLVM_MAIN_SRC_DIR}/utils/llvm-locstats/llvm-locstats.py ${LLVM_TOOLS_BINARY_DIR}/llvm-locstats COMMENT "Copying llvm-locstats into ${LLVM_TOOLS_BINARY_DIR}" ) Comment Actions Actually, the combination of your recommendation and this one will be the appropriate way to resolve this. Please let me know what do you think. diff --git a/llvm/utils/llvm-locstats/CMakeLists.txt b/llvm/utils/llvm-locstats/CMakeLists.txt index d5366f99050..1dbb9da92e2 100644 --- a/llvm/utils/llvm-locstats/CMakeLists.txt +++ b/llvm/utils/llvm-locstats/CMakeLists.txt @@ -2,11 +2,15 @@ if (LLVM_INCLUDE_UTILS AND LLVM_INCLUDE_TOOLS) add_custom_command( OUTPUT ${LLVM_TOOLS_BINARY_DIR}/llvm-locstats DEPENDS ${LLVM_MAIN_SRC_DIR}/utils/llvm-locstats/llvm-locstats.py + DEPENDS llvm-dwarfdump COMMAND ${CMAKE_COMMAND} -E copy ${LLVM_MAIN_SRC_DIR}/utils/llvm-locstats/llvm-locstats.py ${LLVM_TOOLS_BINARY_DIR}/llvm-locstats COMMENT "Copying llvm-locstats into ${LLVM_TOOLS_BINARY_DIR}" ) add_custom_target(llvm-locstats ALL DEPENDS ${LLVM_TOOLS_BINARY_DIR}/llvm-locstats ) + if (NOT LLVM_BUILD_TOOLS) + set_target_properties(llvm-locstats PROPERTIES EXCLUDE_FROM_ALL ON) + endif() set_target_properties(llvm-locstats PROPERTIES FOLDER "Tools") endif() Comment Actions That looks good to me and it works for our use case. Took some time to think through the BUILD_TOOLS vs BUILD_UTILS difference. I think what you have is correct. It means that with BUILD_UTILS ON "all" doesn't include llvm-locstats, but to even use it you'd need to build dwarfdump. And if we just made drawfdump build if BUILD_UTILS is on, that would certainly be unexpected given that dwarfdump is a "tool" itself. |