search

Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

Wrong install path expansion in multi-configuration generated build (VS 2017) #40000

Open Quuxplusone opened 5 years ago

Quuxplusone commented 5 years ago
Bugzilla Link PR41030
Status NEW
Importance P normal
Reported by Richard Musil (richard.musil@gmail.com)
Reported on 2019-03-11 09:01:12 -0700
Last modified on 2020-01-25 18:52:22 -0800
Version trunk
Hardware PC Windows NT
CC llvm-bugs@lists.llvm.org, yuanfang.chen@sony.com
Fixed by commit(s)
Attachments 0001-fix-for-correct-folder-expansion-for-install-scripts.patch (2861 bytes, application/mbox)
Blocks
Blocked by
See also 
Created attachment 21583
Patch file

I am using Visual Studio 2017 build tools (cmake version 3.12.18081601-MSVC_2)
to build LLVM project(s) by running this commands:

> cmake -G "Visual Studio 15 2017" -A x64 -Thost=x64 -
DCMAKE_INSTALL_PREFIX=D:\Dev\LLVM-ng -DLLVM_ENABLE_PROJECTS=clang;clang-tools-
extra;compiler-rt;debuginfo-
tests;libclc;libcxx;libcxxabi;libunwind;lld;llvm;parallel-libs;polly;pstl ..
> cmake --build . --target INSTALL --config Release

Build phase passes alright (--target ALL_BUILD) but install phase bugs out with
an error about missing file source(s). This happens at two occasions when
compiler-rt libs and clang headers are going to be installed.

When looking at corresponding cmake_install.cmake file for compiler-rt libs I
noticed following snippets:

<build-1>/projects/compiler-rt/lib/asan/cmake_install.cmake

> if("x${CMAKE_INSTALL_COMPONENT}x" STREQUAL "xasanx" OR NOT
CMAKE_INSTALL_COMPONENT)
>   if("${CMAKE_INSTALL_CONFIG_NAME}" MATCHES "^([Dd][Ee][Bb][Uu][Gg])$")
>     file(INSTALL DESTINATION
"${CMAKE_INSTALL_PREFIX}/lib/clang/9.0.0/lib/windows" TYPE STATIC_LIBRARY FILES
"D:/Work_OSS/llvm-project/llvm/build-1/$(Configuration)/lib/clang/9.0.0/lib/windows/clang_rt.asan-x86_64.lib")
>   elseif("${CMAKE_INSTALL_CONFIG_NAME}" MATCHES
"^([Rr][Ee][Ll][Ee][Aa][Ss][Ee])$")
>     file(INSTALL DESTINATION
"${CMAKE_INSTALL_PREFIX}/lib/clang/9.0.0/lib/windows" TYPE STATIC_LIBRARY FILES
"D:/Work_OSS/llvm-project/llvm/build-1/$(Configuration)/lib/clang/9.0.0/lib/windows/clang_rt.asan-x86_64.lib")
>   elseif("${CMAKE_INSTALL_CONFIG_NAME}" MATCHES
"^([Mm][Ii][Nn][Ss][Ii][Zz][Ee][Rr][Ee][Ll])$")
>     file(INSTALL DESTINATION
"${CMAKE_INSTALL_PREFIX}/lib/clang/9.0.0/lib/windows" TYPE STATIC_LIBRARY FILES
"D:/Work_OSS/llvm-project/llvm/build-1/$(Configuration)/lib/clang/9.0.0/lib/windows/clang_rt.asan-x86_64.lib")
>   elseif("${CMAKE_INSTALL_CONFIG_NAME}" MATCHES
"^([Rr][Ee][Ll][Ww][Ii][Tt][Hh][Dd][Ee][Bb][Ii][Nn][Ff][Oo])$")
>     file(INSTALL DESTINATION
"${CMAKE_INSTALL_PREFIX}/lib/clang/9.0.0/lib/windows" TYPE STATIC_LIBRARY FILES
"D:/Work_OSS/llvm-project/llvm/build-1/$(Configuration)/lib/clang/9.0.0/lib/windows/clang_rt.asan-x86_64.lib")
>   endif()
> endif()

where the problem is $(Configuration) in the path. So even though the multiple
configurations (Release, Debug, etc.) are properly expanded in the script, they
are all set with the same (wrong) path.

The corresponding script responsible for this is compiler-
rt/cmake/Modules/AddCompilerRT.cmake, this function in particular:

> function(set_target_output_directories target output_dir)
>   # For RUNTIME_OUTPUT_DIRECTORY variable, Multi-configuration generators
>   # append a per-configuration subdirectory to the specified directory.
>   # To avoid the appended folder, the configuration specific variable must be
>   # set 'RUNTIME_OUTPUT_DIRECTORY_${CONF}':
>   # RUNTIME_OUTPUT_DIRECTORY_DEBUG, RUNTIME_OUTPUT_DIRECTORY_RELEASE, ...
>   if(CMAKE_CONFIGURATION_TYPES)
>     foreach(build_mode ${CMAKE_CONFIGURATION_TYPES})
>       string(TOUPPER "${build_mode}" CONFIG_SUFFIX)"${output_dir}")
>       set_target_properties("${target}" PROPERTIES
>           "ARCHIVE_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${output_dir}
>           "LIBRARY_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${output_dir}
>           "RUNTIME_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${output_dir})
>     endforeach()
>   else()
>     set_target_properties("${target}" PROPERTIES
>         ARCHIVE_OUTPUT_DIRECTORY ${output_dir}
>         LIBRARY_OUTPUT_DIRECTORY ${output_dir}
>         RUNTIME_OUTPUT_DIRECTORY ${output_dir})
>   endif()
> endfunction()

Where ${output_dir} contains the path with "$(Configuration)". It seems that
the author's idea was that this will get expanded, but when looking at cmake
doc, and in particular at target properties
(https://cmake.org/cmake/help/latest/prop_tgt/RUNTIME_OUTPUT_DIRECTORY.html) it
says only 'generator expression' gets expanded, but not platform specific build
time variables.

It seems that some cmake scripts authors were already aware of that as for
example llvm/cmake/modules/AddLLVM.cmake, which took this approach:

> if(NOT "${CMAKE_CFG_INTDIR}" STREQUAL ".")
>   foreach(build_mode ${CMAKE_CONFIGURATION_TYPES})
>     string(TOUPPER "${build_mode}" CONFIG_SUFFIX)
>     if(ARG_BINARY_DIR)
>       string(REPLACE ${CMAKE_CFG_INTDIR} ${build_mode} bi ${ARG_BINARY_DIR})
>       set_target_properties(${target} PROPERTIES
"RUNTIME_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${bi})
>     endif()
>     if(ARG_LIBRARY_DIR)
>       string(REPLACE ${CMAKE_CFG_INTDIR} ${build_mode} li ${ARG_LIBRARY_DIR})
>       set_target_properties(${target} PROPERTIES
"ARCHIVE_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${li})
>     endif()
>     if(module_dir)
>       string(REPLACE ${CMAKE_CFG_INTDIR} ${build_mode} mi ${module_dir})
>       set_target_properties(${target} PROPERTIES
"LIBRARY_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${mi})
>     endif()
>   endforeach()
> else()

I.e. generating configuration specific target properties and replacing
"$(Configuration)" by the desired config.

When looking at the cmake doc, I also noticed that while non multi-
configuration target properties (e.g. RUNTIME_OUTPUT_DIRECTORY) are supposed
to be suffixed by multi-configuration generator with the configuration name,
this _does not_ happen, when the property contains expression generator.

In short, the original function can be fixed by using "$<CONFIG>" expression
generator, like this:

> function(set_target_output_directories target output_dir)
>   # If we are using multi-configuration generator ensure proper folder
>   # expansion by using $<CONFIG> generator expression
>   if(CMAKE_CONFIGURATION_TYPES)
>     string(REPLACE "${CMAKE_CFG_INTDIR}" "$<CONFIG>" output_dir
"${output_dir}")
>   endif()
>   set_target_properties("${target}" PROPERTIES
>       ARCHIVE_OUTPUT_DIRECTORY ${output_dir}
>       LIBRARY_OUTPUT_DIRECTORY ${output_dir}
>       RUNTIME_OUTPUT_DIRECTORY ${output_dir})
> endfunction()

The other occurrence related to clang headers is of the similar kind (applied
to "install" cmake command), so I am not going to dissect it here, only
interesting point is that it was not present in 7.0.1 release and apparently
crept into the current branch later.

As the bug only manifests itself in the install phase I assume the install
phase is not normally validated before release. I would suggest adding it to
the validation.

Proposed patch attached is generated against current master.
Quuxplusone commented 5 years ago

Attached 0001-fix-for-correct-folder-expansion-for-install-scripts.patch (2861 bytes, application/mbox): Patch file