Page MenuHomePhabricator

[cmake] Include the llvm-locstats target when utils and tools are not being built.
ClosedPublic

Authored by DavidSpickett on Dec 19 2019, 2:29 AM.

Details

Summary

This was uncovered by: https://reviews.llvm.org/D71611
Which added llvm-locstats to the test dependencies.

Previously the build target was only added if you
were building tools. This meant that you couldn't
configure at all if you had LLVM_BUILD_TOOLS=OFF.

Diff Detail

Event Timeline

DavidSpickett created this revision.Dec 19 2019, 2:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 19 2019, 2:29 AM
djtodoro accepted this revision.Dec 19 2019, 2:37 AM

Thanks!

This revision is now accepted and ready to land.Dec 19 2019, 2:37 AM
djtodoro requested changes to this revision.Dec 19 2019, 2:44 AM

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.

This revision now requires changes to proceed.Dec 19 2019, 2:44 AM
This revision was not accepted when it landed; it landed in state Needs Revision.Dec 19 2019, 2:45 AM
This revision was automatically updated to reflect the committed changes.

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()

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:
LLVM_BUILD_TOOLS
LLVM_INCLUDE_TOOLS

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.

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()

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?

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()

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?

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?

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?

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()

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?

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?

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.
Perhaps, we should add the llvm-dwarfdump target as dependency for the llvm-locstats target. WDYT?

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}"
     )

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}"
     )

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()

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.