[PATCH] D150752: [BOLT][CMake] Use correct output paths and passthrough necessary options

[Petr Hosek via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9e6e3375f12e: [BOLT][CMake] Use correct output paths and 
passthrough necessary options (authored by phosek).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150752/new/

https://reviews.llvm.org/D150752

Files:
  bolt/CMakeLists.txt
  bolt/runtime/CMakeLists.txt
  clang/cmake/caches/Fuchsia-stage2.cmake


Index: clang/cmake/caches/Fuchsia-stage2.cmake
===
--- clang/cmake/caches/Fuchsia-stage2.cmake
+++ clang/cmake/caches/Fuchsia-stage2.cmake
@@ -294,7 +294,6 @@
 set(LLVM_TOOLCHAIN_TOOLS
   dsymutil
   llvm-ar
-  llvm-bolt
   llvm-cov
   llvm-cxxfilt
   llvm-debuginfod-find
@@ -329,6 +328,7 @@
   CACHE STRING "")
 
 set(LLVM_Toolchain_DISTRIBUTION_COMPONENTS
+  bolt
   clang
   lld
   clang-apply-replacements
Index: bolt/runtime/CMakeLists.txt
===
--- bolt/runtime/CMakeLists.txt
+++ bolt/runtime/CMakeLists.txt
@@ -15,12 +15,12 @@
   instr.cpp
   ${CMAKE_CURRENT_BINARY_DIR}/config.h
   )
-set_target_properties(bolt_rt_instr PROPERTIES LIBRARY_OUTPUT_DIRECTORY 
"lib${LLVM_LIBDIR_SUFFIX}")
+set_target_properties(bolt_rt_instr PROPERTIES ARCHIVE_OUTPUT_DIRECTORY 
"${LLVM_LIBRARY_DIR}")
 add_library(bolt_rt_hugify STATIC
   hugify.cpp
   ${CMAKE_CURRENT_BINARY_DIR}/config.h
   )
-set_target_properties(bolt_rt_hugify PROPERTIES LIBRARY_OUTPUT_DIRECTORY 
"lib${LLVM_LIBDIR_SUFFIX}")
+set_target_properties(bolt_rt_hugify PROPERTIES ARCHIVE_OUTPUT_DIRECTORY 
"${LLVM_LIBRARY_DIR}")
 
 set(BOLT_RT_FLAGS
   -ffreestanding
@@ -44,7 +44,7 @@
 instr.cpp
 ${CMAKE_CURRENT_BINARY_DIR}/config.h
   )
-  set_target_properties(bolt_rt_instr_osx PROPERTIES LIBRARY_OUTPUT_DIRECTORY 
"lib${LLVM_LIBDIR_SUFFIX}")
+  set_target_properties(bolt_rt_instr_osx PROPERTIES ARCHIVE_OUTPUT_DIRECTORY 
"${LLVM_LIBRARY_DIR}")
   target_include_directories(bolt_rt_instr_osx PRIVATE 
${CMAKE_CURRENT_BINARY_DIR})
   target_compile_options(bolt_rt_instr_osx PRIVATE
 -target x86_64-apple-darwin19.6.0
Index: bolt/CMakeLists.txt
===
--- bolt/CMakeLists.txt
+++ bolt/CMakeLists.txt
@@ -84,6 +84,10 @@
 
 if (BOLT_ENABLE_RUNTIME)
   message(STATUS "Building BOLT runtime libraries for X86")
+  set(extra_args "")
+  if(CMAKE_SYSROOT)
+list(APPEND extra_args -DCMAKE_SYSROOT=${CMAKE_SYSROOT})
+  endif()
   ExternalProject_Add(bolt_rt
 SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/runtime"
 STAMP_DIR ${CMAKE_CURRENT_BINARY_DIR}/bolt_rt-stamps
@@ -92,8 +96,10 @@
-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}
-DCMAKE_BUILD_TYPE=Release
-DCMAKE_MAKE_PROGRAM=${CMAKE_MAKE_PROGRAM}
-   -DCMAKE_INSTALL_PREFIX=${LLVM_BINARY_DIR}
-DLLVM_LIBDIR_SUFFIX=${LLVM_LIBDIR_SUFFIX}
+   -DLLVM_LIBRARY_DIR=${LLVM_LIBRARY_DIR}
+   ${extra_args}
+INSTALL_COMMAND ""
 BUILD_ALWAYS True
 )
   install(CODE "execute_process\(COMMAND \${CMAKE_COMMAND} 
-DCMAKE_INSTALL_PREFIX=\${CMAKE_INSTALL_PREFIX} -P 
${CMAKE_CURRENT_BINARY_DIR}/bolt_rt-bins/cmake_install.cmake \)"


Index: clang/cmake/caches/Fuchsia-stage2.cmake
===
--- clang/cmake/caches/Fuchsia-stage2.cmake
+++ clang/cmake/caches/Fuchsia-stage2.cmake
@@ -294,7 +294,6 @@
 set(LLVM_TOOLCHAIN_TOOLS
   dsymutil
   llvm-ar
-  llvm-bolt
   llvm-cov
   llvm-cxxfilt
   llvm-debuginfod-find
@@ -329,6 +328,7 @@
   CACHE STRING "")
 
 set(LLVM_Toolchain_DISTRIBUTION_COMPONENTS
+  bolt
   clang
   lld
   clang-apply-replacements
Index: bolt/runtime/CMakeLists.txt
===
--- bolt/runtime/CMakeLists.txt
+++ bolt/runtime/CMakeLists.txt
@@ -15,12 +15,12 @@
   instr.cpp
   ${CMAKE_CURRENT_BINARY_DIR}/config.h
   )
-set_target_properties(bolt_rt_instr PROPERTIES LIBRARY_OUTPUT_DIRECTORY "lib${LLVM_LIBDIR_SUFFIX}")
+set_target_properties(bolt_rt_instr PROPERTIES ARCHIVE_OUTPUT_DIRECTORY "${LLVM_LIBRARY_DIR}")
 add_library(bolt_rt_hugify STATIC
   hugify.cpp
   ${CMAKE_CURRENT_BINARY_DIR}/config.h
   )
-set_target_properties(bolt_rt_hugify PROPERTIES LIBRARY_OUTPUT_DIRECTORY "lib${LLVM_LIBDIR_SUFFIX}")
+set_target_properties(bolt_rt_hugify PROPERTIES ARCHIVE_OUTPUT_DIRECTORY "${LLVM_LIBRARY_DIR}")
 
 set(BOLT_RT_FLAGS
   -ffreestanding
@@ -44,7 +44,7 @@
 instr.cpp
 ${CMAKE_CURRENT_BINARY_DIR}/config.h
   )
-  set_target_properties(bolt_rt_instr_osx PROPERTIES LIBRARY_OUTPUT_DIRECTORY "lib${LLVM_LIBDIR_SUFFIX}")
+  set_target_properties(bolt_rt_instr_osx PROPERTIES ARCHIVE_OUTPUT_DIRECTORY "${LLVM_LIBRARY_DIR}")
   target_include_directories(bolt_rt_instr_osx PRIVATE ${CMAKE_CURRENT_BINARY_DIR})
   target_compile_options(

[PATCH] D150752: [BOLT][CMake] Use correct output paths and passthrough necessary options

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.



Comment at: bolt/CMakeLists.txt:88
+  if(CMAKE_SYSROOT)
+list(APPEND extra_args -DCMAKE_SYSROOT=${CMAKE_SYSROOT})
+  endif()

michaelplatings wrote:
> phosek wrote:
> > michaelplatings wrote:
> > > michaelplatings wrote:
> > > > The [[ 
> > > > https://cmake.org/cmake/help/latest/variable/CMAKE_TOOLCHAIN_FILE.html 
> > > > | documentation for CMAKE_SYSROOT  ]] says "This variable may only be 
> > > > set in a toolchain file specified by the CMAKE_TOOLCHAIN_FILE variable."
> > > > 
> > > > Maybe pass CMAKE_TOOLCHAIN_FILE through instead?
> > > `list(APPEND variable_that_may_or_may_not_exist ...` is quite fragile. 
> > > Should someone have `-Dextra_args=X` on their cmake command line then 
> > > that will be included here, which I don't think is the intent. I suggest 
> > > adding `set(extra_args "")` above the `if`. This also improves 
> > > readability because otherwise you have to scan the whole file to figure 
> > > out that `extra_args` wasn't previously defined.
> > That documentation is inaccurate. `CMAKE_SYSROOT` can be set outside of the 
> > toolchain file, and thus needs to be passed through as is frequently done 
> > in LLVM, see for example 
> > https://github.com/llvm/llvm-project/blob/7ca1bcbc6043b7ab4635f04746d1b9480960e384/llvm/cmake/modules/LLVMExternalProjectUtils.cmake#L261
> >  or 
> > https://github.com/llvm/llvm-project/blob/7ca1bcbc6043b7ab4635f04746d1b9480960e384/compiler-rt/cmake/Modules/AddCompilerRT.cmake#L650.
> Deviating from what's documented as permitted could cause a problem with 
> future CMake versions. But evidently this pattern is already established in 
> LLVM, and if it becomes a problem in future then it can be fixed at that time.
I agree, ideally we would use the same helper function everywhere where we need 
to invoke CMake as a subbuild so the passthrough is handled uniformly, but that 
can be done as a future improvement.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150752/new/

https://reviews.llvm.org/D150752

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits