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

Fri May 19 10:43:19 PDT 2023

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