cmake: use llvm dir variables for clang/utils/hmaptool

[mizvekov]

Summary

Copy hmaptool using the paths for CURRENT_TOOLS_DIR, so
everything goes in the right place in case llvm is included
from a top level CMakeLists.txt.

Signed-off-by: Matheus Izvekov <[e-mail]>

[tstellar]

This

clang/utils/hmaptool/CMakeLists.txt

5

Should this be LLVM_TOOLS_BINARY_DIR ?

[stephenneuendorffer]

LGTM... Does LLVM_TOOLS_BINARY_DIR include CMAKE_CFG_INTDIR? Is this actually NFC?

clang/utils/hmaptool/CMakeLists.txt

1

Unused elsewhere, I assume?

[mizvekov]

In D126308#3538064, @stephenneuendorffer wrote:

LGTM... Does LLVM_TOOLS_BINARY_DIR include CMAKE_CFG_INTDIR? Is this actually NFC?

On a normal build where the CMakelists in the llvm subdir is used as the top level one, this should be NFC.

If the llvm CMakeLists is added from another CMakeLists, then mostly everything works, except that some odd cases like this one get copied somewhere the lit tests do not know how to find.

clang/utils/hmaptool/CMakeLists.txt

1

Yeah, as far as I understand, set by default will only create / modify variables in the current scope, where it will be accessible only from this CMakeLists.txt and any others which you add_subdirectory from here on recursively. Variables are inherited from scope to scope by copy, not reference.

You need to otherwise pass PARENT_SCOPE argument into it so it will modify the copy in the enclosing scope, which sadly will not modify the copy on the current scope if one existed.

5

I think LLVM_UTILS_INSTALL_DIR is the path where the programs will be installed, and LLVM_TOOLS_BINARY_DIR is where they are located in the build tree.

So I think the idea is that this install invocation will install the file, for the packaging for example, while add_custom_command above will just copy this program into the build tree so that llvm-lit will find it when run from the build tree.

[tstellar]

clang/utils/hmaptool/CMakeLists.txt

5

OK, I see. Makes sense.

[Ericson2314]

clang/utils/hmaptool/CMakeLists.txt

5

That is right about INSTALL_DIR vs BINARY_DIR, but LLVM_UTILS_INSTALL_DIR and LLVM_TOOLS_BINARY_DIR both exist.

At least after https://reviews.llvm.org/D117977 CLANG_ ones would exist, which would make more sense for this.

[nikic]

It looks like this change breaks the standalone build for clang. From what I can see, while LLVM_TOOLS_BINARY_DIR and LLVM_TOOLS_INSTALL_DIR are exported in llvm/cmake/modules/LLVMConfig.cmake.in, LLVM_UTILS_INSTALL_DIR is not, resulting in an "install PROGRAMS given no DESTINATION" error. Presumably we either need to export that variable, or it's not intended to be used outside LLVM.

[mizvekov]

In D126308#3545410, @nikic wrote:

It looks like this change breaks the standalone build for clang. From what I can see, while LLVM_TOOLS_BINARY_DIR and LLVM_TOOLS_INSTALL_DIR are exported in llvm/cmake/modules/LLVMConfig.cmake.in, LLVM_UTILS_INSTALL_DIR is not, resulting in an "install PROGRAMS given no DESTINATION" error. Presumably we either need to export that variable, or it's not intended to be used outside LLVM.

Interesting, had not realized we need to export them for standalone builds.

llvm/cmake/modules/AddLLVM.cmake exports a macro add_llvm_utility that uses this variable, and this file included from clang CMakeLists.

Since this macro would not be usable without this variable, then either add_llvm_utility is not intended to be exported, or it's intended and it was an oversight to not export LLVM_UTILS_INSTALL_DIR .

[Ericson2314]

There is a lot of cruft behind the signs. I would just use LLVM_TOOLS_INSTALL_DIR then which is exported to get this unblocked, and I'll rebase D117977 to try to clean these things up later.

[mizvekov]

In D126308#3545937, @Ericson2314 wrote:

There is a lot of cruft behind the signs. I would just use LLVM_TOOLS_INSTALL_DIR then which is exported to get this unblocked, and I'll rebase D117977 to try to clean these things up later.

Done in https://reviews.llvm.org/D126671

[nikic]

In D126308#3546235, @mizvekov wrote:

In D126308#3545937, @Ericson2314 wrote:

There is a lot of cruft behind the signs. I would just use LLVM_TOOLS_INSTALL_DIR then which is exported to get this unblocked, and I'll rebase D117977 to try to clean these things up later.

Done in https://reviews.llvm.org/D126671

Thanks for the fix! Unfortunately I'm still seeing a build failure related to this change:

[176/1855] cd /builddir/build/BUILD/clang-15.0.0.src/redhat-linux-build/utils/hmaptool && /usr/bin/cmake -E copy /builddir/build/BUILD/clang-15.0.0.src/utils/hmaptool/hmaptool /usr/bin
FAILED: /usr/bin/hmaptool
cd /builddir/build/BUILD/clang-15.0.0.src/redhat-linux-build/utils/hmaptool && /usr/bin/cmake -E copy /builddir/build/BUILD/clang-15.0.0.src/utils/hmaptool/hmaptool /usr/bin
Error copying file "/builddir/build/BUILD/clang-15.0.0.src/utils/hmaptool/hmaptool" to "/usr/bin".

TBH I don't really understand why this fails and I can't reproduce this when running the commands manually. I'd have said there is a race condition here due to a missing dependency, but as this is copying a file from the source directory, I don't really see how that could be.

However, seeing this copy command, I think the change being make here is fundamentally incorrect. This is copying hmaptool into LLVM_TOOLS_BINARY_DIR (as part of the normal build, not as part of an install command), where LLVM_TOOLS_BINARY_DIR is the location of the installed LLVM tools. This means we end up copying hmaptool into something like /usr/bin as part of a normal build, which definitely shouldn't be happening. The tool should only get copied into the right location in the cmake build directory (which is what the previous implementation did).

[mizvekov]

In D126308#3547435, @nikic wrote:

Thanks for the fix! Unfortunately I'm still seeing a build failure related to this change:

[176/1855] cd /builddir/build/BUILD/clang-15.0.0.src/redhat-linux-build/utils/hmaptool && /usr/bin/cmake -E copy /builddir/build/BUILD/clang-15.0.0.src/utils/hmaptool/hmaptool /usr/bin
FAILED: /usr/bin/hmaptool
cd /builddir/build/BUILD/clang-15.0.0.src/redhat-linux-build/utils/hmaptool && /usr/bin/cmake -E copy /builddir/build/BUILD/clang-15.0.0.src/utils/hmaptool/hmaptool /usr/bin
Error copying file "/builddir/build/BUILD/clang-15.0.0.src/utils/hmaptool/hmaptool" to "/usr/bin".

TBH I don't really understand why this fails and I can't reproduce this when running the commands manually. I'd have said there is a race condition here due to a missing dependency, but as this is copying a file from the source directory, I don't really see how that could be.

However, seeing this copy command, I think the change being make here is fundamentally incorrect. This is copying hmaptool into LLVM_TOOLS_BINARY_DIR (as part of the normal build, not as part of an install command), where LLVM_TOOLS_BINARY_DIR is the location of the installed LLVM tools. This means we end up copying hmaptool into something like /usr/bin as part of a normal build, which definitely shouldn't be happening. The tool should only get copied into the right location in the cmake build directory (which is what the previous implementation did).

On a unified build, this is what I get:
LLVM_TOOLS_BINARY_DIR: C:/Users/mizve/source/repos/llvm/build/dbg/llvm/./bin

If on a standalone build that points to the llvm install dir, then this is confusing, but I understand now.

I will try to get this resolved, but if this is blocking you then feel free to revert for now.

[Ericson2314]

I am sorry, I completely forgot one of my motivations for https://reviews.llvm.org/D117977 was that LLVM_TOOLS_BINARY_DIR was completely misnamed and just this sort of foot-gun!

[mizvekov]

• fix for standalone builds.

[mizvekov]

@nikic can you confirm this new patch is good for standalone?

@Ericson2314 FYI in case your patch is going to improve on this.

[mizvekov]

@nikic ping, can you confirm this current patch works for you?

[nikic]

In D126308#3568696, @mizvekov wrote:

@nikic ping, can you confirm this current patch works for you?

Sorry, missed the update. Just tried this in a local build and it seems to work fine!

[mizvekov]

In D126308#3569379, @nikic wrote:

Sorry, missed the update. Just tried this in a local build and it seems to work fine!

Thanks!

[dyung]

Hi @mizvekov, this change seems to have broken the location of the hmaptool when using the Visual Studio builder on Windows. After your commit, 7 clang tests fail because hmaptool cannot be found at the expected location. For example, the test clang/test/Preprocessor/headermap-rel.c:

FAIL: Clang :: Preprocessor/headermap-rel.c (1 of 1)
******************** TEST 'Clang :: Preprocessor/headermap-rel.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   rm -f D:\src\upstream\51608515faa7-PS4-Release\tools\clang\test\Preprocessor\Output\headermap-rel.c.tmp.hmap
: 'RUN: at line 2';   'C:/Program Files/Python310/python.exe' D:\src\upstream\51608515faa7-PS4-Release\Release\bin\hmaptool write D:\src\upstream\llvm_clean_git\clang\test\Preprocessor/Inputs/headermap-rel/foo.hmap.json D:\src\upstream\51608515faa7-PS4-Release\tools\clang\test\Preprocessor\Output\headermap-rel.c.tmp.hmap
: 'RUN: at line 3';   d:\src\upstream\51608515faa7-ps4-release\release\bin\clang.exe -cc1 -internal-isystem d:\src\upstream\51608515faa7-ps4-release\release\lib\clang\15.0.0\include -nostdsysteminc -E D:\src\upstream\llvm_clean_git\clang\test\Preprocessor\headermap-rel.c -o D:\src\upstream\51608515faa7-PS4-Release\tools\clang\test\Preprocessor\Output\headermap-rel.c.tmp.i -I D:\src\upstream\51608515faa7-PS4-Release\tools\clang\test\Preprocessor\Output\headermap-rel.c.tmp.hmap -F D:\src\upstream\llvm_clean_git\clang\test\Preprocessor/Inputs/headermap-rel
: 'RUN: at line 4';   d:\src\upstream\51608515faa7-ps4-release\release\bin\filecheck.exe D:\src\upstream\llvm_clean_git\clang\test\Preprocessor\headermap-rel.c -input-file D:\src\upstream\51608515faa7-PS4-Release\tools\clang\test\Preprocessor\Output\headermap-rel.c.tmp.i
--
Exit Code: 2

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "rm" "-f" "D:\src\upstream\51608515faa7-PS4-Release\tools\clang\test\Preprocessor\Output\headermap-rel.c.tmp.hmap"
$ ":" "RUN: at line 2"
$ "C:/Program Files/Python310/python.exe" "D:\src\upstream\51608515faa7-PS4-Release\Release\bin\hmaptool" "write" "D:\src\upstream\llvm_clean_git\clang\test\Preprocessor/Inputs/headermap-rel/foo.hmap.json" "D:\src\upstream\51608515faa7-PS4-Release\tools\clang\test\Preprocessor\Output\headermap-rel.c.tmp.hmap"
# command stderr:
C:/Program Files/Python310/python.exe: can't open file 'D:\\src\\upstream\\51608515faa7-PS4-Release\\Release\\bin\\hmaptool': [Errno 2] No such file or directory

error: command failed with exit status: 2

Before your change, in this particular case when doing a Release build with Visual Studio, hmaptool can be found at <build_dir>\Release\bin\hmaptool, but after your change, it is now located at <build_dir>\s\bin\hmaptool which is not where lit expects to find it causing the test to fail.

Can you take a look?

[mizvekov]

In D126308#3581240, @dyung wrote:

Can you take a look?

Hi @dyung , thanks for the report and sorry for the trouble!
Can you help me try to reproduce it?

Which version of Visual Studio, and what preset are you using?

[dyung]

In D126308#3582337, @mizvekov wrote:

In D126308#3581240, @dyung wrote:

Can you take a look?

Hi @dyung , thanks for the report and sorry for the trouble!
Can you help me try to reproduce it?

Which version of Visual Studio, and what preset are you using?

Sure, we are using Visual Studio 2019. The cmake command I use is:

cmake.exe -G "Visual Studio 16 2019" -DCLANG_ENABLE_ARCMT=OFF -DCMAKE_BUILD_TYPES=Release -DLLVM_BUILD_RUNTIME=OFF -DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-ps4-scei -DLLVM_ENABLE_TIMESTAMPS=OFF -DLLVM_INCLUDE_EXAMPLES=OFF -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_TOOL_LLD_BUILD=OFF -DLLVM_VERSION_SUFFIX= -DLLVM_LIT_ARGS="--verbose -j24" -Thost=x64 -DLLVM_ENABLE_PROJECTS=clang <path to llvm>

From the files that are generated, we then do a Release build and try to run the tests using that. (Although I believe any build type should be affected similarly.)

[mizvekov]

Hi @dyung, can you verify the solution I proposed in https://reviews.llvm.org/D127943 is good for you?

clang/utils/hmaptool/CMakeLists.txt

-set(CLANG_HMAPTOOL hmaptool)
+add_custom_command(OUTPUT "${LLVM_TOOLS_BINARY_DIR}/hmaptool"
+                   COMMAND "${CMAKE_COMMAND}" -E copy "${CMAKE_CURRENT_SOURCE_DIR}/hmaptool" "${LLVM_TOOLS_BINARY_DIR}"
+                   DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/hmaptool")
 
-add_custom_command(OUTPUT ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/${CLANG_HMAPTOOL}
-                   COMMAND ${CMAKE_COMMAND} -E make_directory
-                     ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin
-                   COMMAND ${CMAKE_COMMAND} -E copy
-                     ${CMAKE_CURRENT_SOURCE_DIR}/${CLANG_HMAPTOOL}
-                     ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/
-                   DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/${CLANG_HMAPTOOL})
-
-list(APPEND Depends ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/${CLANG_HMAPTOOL})
-install(PROGRAMS ${CLANG_HMAPTOOL}
-        DESTINATION "${CMAKE_INSTALL_BINDIR}"
-        COMPONENT hmaptool)
-
-add_custom_target(hmaptool ALL DEPENDS ${Depends})
+install(PROGRAMS hmaptool DESTINATION "${LLVM_UTILS_INSTALL_DIR}}" COMPONENT hmaptool)
+add_custom_target(hmaptool ALL DEPENDS "${LLVM_TOOLS_BINARY_DIR}/hmaptool")
 set_target_properties(hmaptool PROPERTIES FOLDER "Utils")
 
 if(NOT LLVM_ENABLE_IDE)
   add_llvm_install_targets("install-hmaptool"
                            DEPENDS hmaptool
                            COMPONENT hmaptool)
 endif()