[PATCH] D116709: [CMake][WinMsvc] Fix user passed compiler/linker flags

[Yuanfang Chen via Phabricator via llvm-commits]

ychen added inline comments.


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:253-257
+list(APPEND _CTF_NATIVE_DEFAULT "-DCMAKE_C_FLAGS=\"\"")
+list(APPEND _CTF_NATIVE_DEFAULT "-DCMAKE_CXX_FLAGS=\"\"")
+list(APPEND _CTF_NATIVE_DEFAULT "-DCMAKE_EXE_LINKER_FLAGS=\"\"")
+list(APPEND _CTF_NATIVE_DEFAULT "-DCMAKE_MODULE_LINKER_FLAGS=\"\"")
+list(APPEND _CTF_NATIVE_DEFAULT "-DCMAKE_SHARED_LINKER_FLAGS=\"\"")
----------------
smeenai wrote:
> I'm not a fan of this. A pattern I like to follow is to pass a `CMAKE_TOOLCHAIN_FILE` to `CROSS_TOOLCHAIN_FLAGS_NATIVE`, so that the native build flags get configured via a toolchain file instead of ad-hoc variable setting. Pre-set cached copies of these variables will interfere with that toolchain file's ability to set flags, and in particular, it'll break the toolchain file's use of the *_INIT variables, which is kinda ironic given the purpose of this diff :)
> 
> Would unsetting the env vars in the toolchain file instead accomplish the same goal?
I see what happened now. The native cmake config time is cross-build's build time. I should make sure these env vars are not set during the cross-build's build time.


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:292-293
 # afterward.
-set(_CMAKE_C_FLAGS_INITIAL "${CMAKE_C_FLAGS}" CACHE STRING "")
-set(CMAKE_C_FLAGS "${_CMAKE_C_FLAGS_INITIAL} ${COMPILE_FLAGS}" CACHE STRING "" FORCE)
+set(_CMAKE_C_FLAGS_INITIAL "${CMAKE_C_FLAGS_INIT}" CACHE STRING "")
+set(CMAKE_C_FLAGS_INIT "${_CMAKE_C_FLAGS_INITIAL} ${COMPILE_FLAGS}" CACHE STRING "" FORCE)
 
----------------
smeenai wrote:
> I think we can avoid having the two separate variables, and just do a direct setting, if we make sure this toolchain file is only evaluated once per CMake invocation. Now that we've raised our minimum CMake version, just adding
> 
> ```
> include_guard(GLOBAL)
> ```
> 
> at the top ought to do the trick.
> 
> It's fine to leave that for a follow-up change (or not do it at all).
Thanks. This works great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116709