[CMake][WinMsvc] Fix user passed compiler/linker flags

[ychen]

Summary

Users could pass flags by environment variables like CFLAGS/CXXFLAGS/LDFLAGS
or by using CMAKE_<LANG>_FLAGS_INIT/CMAKE_<t>_LINKER_FLAGS_INIT. So this
toolchain file should append to INIT flags instead. Otherwise, user
flags would be discarded here by assigning to CMAKE_<LANG>_FLAGS
directly.

[ychen]

• update. ready for review

[mstorsjo]

User could pass flags by environment variables like CFLAGS/CXXFLAGS/LDFLAGS or by using CMAKE_<LANG>_FLAGS_INIT/CMAKE_<t>_LINKER_FLAGS_INIT. So this toolchain file should append to INIT flags instead. Otherwise, user flags would be discarded here by assigning to CMAKE_<LANG>_FLAGS directly.

Can you elaborate a bit more here so I follow properly (with more explanation of exactly what cmake does here)? If the user has set e.g. CXXFLAGS, that does end up picked up by cmake and set in CMAKE_CXX_FLAGS_INIT - is that correct? What happened before, when we appended things into CMAKE_CXX_FLAGS without taking CMAKE_CXX_FLAGS_INIT into account - were the flags from CXXFLAGS and/or CMAKE_CXX_FLAGS_INIT missed/forgotten/overridden? Can you explain the implicit cmake behavious that are at play here?

Also, for the native tool build, target flags should not be used.

Flags specified to the main cmake build (CMAKE_<LANG>_FLAGS*) shouldn't be propagated automatically to the inner native cmake build, or am I missing something?

[ychen]

In D116709#3224870, @mstorsjo wrote:

User could pass flags by environment variables like CFLAGS/CXXFLAGS/LDFLAGS or by using CMAKE_<LANG>_FLAGS_INIT/CMAKE_<t>_LINKER_FLAGS_INIT. So this toolchain file should append to INIT flags instead. Otherwise, user flags would be discarded here by assigning to CMAKE_<LANG>_FLAGS directly.

Can you elaborate a bit more here so I follow properly (with more explanation of exactly what cmake does here)? If the user has set e.g. CXXFLAGS, that does end up picked up by cmake and set in CMAKE_CXX_FLAGS_INIT - is that correct? What happened before, when we appended things into CMAKE_CXX_FLAGS without taking CMAKE_CXX_FLAGS_INIT into account - were the flags from CXXFLAGS and/or CMAKE_CXX_FLAGS_INIT missed/forgotten/overridden? Can you explain the implicit cmake behavious that are at play here?

cmake uses CMAKE_<LANG>_FLAGS_INIT (INIT flags themselves are initialized by environment variables like CFLAGS/CXXFLAGS/LDFLAGS, or by a cmake toolchain file) to initialize CMAKE_<LANG>_FLAGS. If CMAKE_<LANG>_FLAGS is set by the user directly, cmake will not initialize it from CMAKE_<LANG>_FLAGS_INIT anymore, so the user-specified flags are lost. Docs on CMAKE_<LANG>_FLAGS_INIT suggests that the preferred behavior is for a toolchain file to manipulate INIT flags.

Also, for the native tool build, target flags should not be used.

Flags specified to the main cmake build (CMAKE_<LANG>_FLAGS*) shouldn't be propagated automatically to the inner native cmake build, or am I missing something?

That's right except when users specify flags by environment vars which will be picked up by sub-cmake through the above-described initialization process. Setting CMAKE_<LANG>_FLAGS to empty here is to avoid that.

[mstorsjo]

Thanks for the explanations! LGTM.

[smeenai]

llvm/cmake/platforms/WinMsvc.cmake

254–258

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?

282–283

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).

[ychen]

• Use include guard
• Add comment about NATIVE cmake build flags.

[ychen]

llvm/cmake/platforms/WinMsvc.cmake

254–258

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.

282–283

Thanks. This works great.

[smeenai]

Looks great! Thanks for cleaning this up :)

[glandium]

Somehow this change broke cross compiling for 32-bit windows using WinMsvc.cmake. Cmake ends up adding /machine:x64 to the linker flags during one of its first checks, and that fails.

[ychen]

In D116709#3374131, @glandium wrote:

Somehow this change broke cross compiling for 32-bit windows using WinMsvc.cmake. Cmake ends up adding /machine:x64 to the linker flags during one of its first checks, and that fails.

Thanks for reporting this. What's your env output look like during config and build time? How do I reproduce it locally?

[smeenai]

In D116709#3374131, @glandium wrote:

Somehow this change broke cross compiling for 32-bit windows using WinMsvc.cmake. Cmake ends up adding /machine:x64 to the linker flags during one of its first checks, and that fails.

Hmm. Are you passing any CMAKE_C_FLAGS or CMAKE_CXX_FLAGS directly as part of your configuration? That'd make the CMAKE_*_FLAGS_INIT in this file be ignored.

[glandium]

The only variables I'm passing: CMAKE_C_COMPILER_TARGET, CMAKE_CXX_COMPILER_TARGET, CMAKE_ASM_COMPILER_TARGET, CMAKE_BUILD_TYPE, LLVM_ENABLE_ASSERTIONS, LLVM_CONFIG_PATH, COMPILER_RT_DEFAULT_TARGET_ONLY, CMAKE_TOOLCHAIN_FILE, LLVM_NATIVE_TOOLCHAIN, MSVC_BASE, WINSDK_BASE, WINSDK_VER, HOST_ARCH.

CMakeCCompilerId.c ends up being compiled with no flags at all.

[glandium]

It still happens when I cut down to CMAKE_TOOLCHAIN_FILE, LLVM_NATIVE_TOOLCHAIN, MSVC_BASE, WINSDK_BASE, WINSDK_VER and HOST_ARCH.

[glandium]

It turns out CMAKE_*_FLAGS is only initialized from CMAKE_*_FLAGS_INIT _after_ CMAKE_DETERMINE_COMPILER_ID is done.

llvm/cmake/platforms/WinMsvc.cmake

 #           etc
 #
 # IMPORTANT: In order for this to work, you will need a valid copy of the Windows
 # SDK and C++ STL headers and libraries on your host.  Additionally, since the
 # Windows libraries and headers are not case-correct, this toolchain file sets
 # up a VFS overlay for the SDK headers and case-correcting symlinks for the
 # libraries when running on a case-sensitive filesystem.
 
+include_guard(GLOBAL)
 
 # When configuring CMake with a toolchain file against a top-level CMakeLists.txt,
 # it will actually run CMake many times, once for each small test program used to
 # determine what features a compiler supports.  Unfortunately, none of these
 # invocations share a CMakeCache.txt with the top-level invocation, meaning they
 # won't see the value of any arguments the user passed via -D.  Since these are
 # necessary to properly configure MSVC in both the top-level configuration as well as
 # all feature-test invocations, we set environment variables with the values so that

 # Even though we're cross-compiling, we need some native tools (e.g. llvm-tblgen), and those
 # native tools have to be built before we can start doing the cross-build.  LLVM supports
 # a CROSS_TOOLCHAIN_FLAGS_NATIVE argument which consists of a list of flags to pass to CMake
 # when configuring the NATIVE portion of the cross-build.  By default we construct this so
 # that it points to the tools in the same location as the native clang-cl that we're using.
 list(APPEND _CTF_NATIVE_DEFAULT "-DCMAKE_ASM_COMPILER=${LLVM_NATIVE_TOOLCHAIN}/bin/clang")
 list(APPEND _CTF_NATIVE_DEFAULT "-DCMAKE_C_COMPILER=${LLVM_NATIVE_TOOLCHAIN}/bin/clang")
 list(APPEND _CTF_NATIVE_DEFAULT "-DCMAKE_CXX_COMPILER=${LLVM_NATIVE_TOOLCHAIN}/bin/clang++")
 
+# These flags are used during build time. So if CFLAGS/CXXFLAGS/LDFLAGS is set
+# for the target, makes sure these are unset during build time.
 set(CROSS_TOOLCHAIN_FLAGS_NATIVE "${_CTF_NATIVE_DEFAULT}" CACHE STRING "")
 
 set(COMPILE_FLAGS
     -D_CRT_SECURE_NO_WARNINGS
     --target=${TRIPLE_ARCH}-windows-msvc
     -fms-compatibility-version=19.14
     -imsvc "${ATLMFC_INCLUDE}"
     -imsvc "${MSVC_INCLUDE}"
     -imsvc "${WINSDK_INCLUDE}/ucrt"
     -imsvc "${WINSDK_INCLUDE}/shared"
     -imsvc "${WINSDK_INCLUDE}/um"
     -imsvc "${WINSDK_INCLUDE}/winrt")
 
 if(case_sensitive_filesystem)
   # Ensure all sub-configures use the top-level VFS overlay instead of generating their own.
   init_user_prop(winsdk_vfs_overlay_path)
   if(NOT winsdk_vfs_overlay_path)
     set(winsdk_vfs_overlay_path "${CMAKE_BINARY_DIR}/winsdk_vfs_overlay.yaml")
     generate_winsdk_vfs_overlay("${WINSDK_INCLUDE}" "${winsdk_vfs_overlay_path}")
     init_user_prop(winsdk_vfs_overlay_path)
   endif()
   list(APPEND COMPILE_FLAGS
        -Xclang -ivfsoverlay -Xclang "${winsdk_vfs_overlay_path}")
 endif()
 
 string(REPLACE ";" " " COMPILE_FLAGS "${COMPILE_FLAGS}")
-
-# We need to preserve any flags that were passed in by the user. However, we
-# can't append to CMAKE_C_FLAGS and friends directly, because toolchain files
-# will be re-invoked on each reconfigure and therefore need to be idempotent.
-# The assignments to the _INITIAL cache variables don't use FORCE, so they'll
-# only be populated on the initial configure, and their values won't change
-# 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_CXX_FLAGS_INITIAL "${CMAKE_CXX_FLAGS}" CACHE STRING "")
-set(CMAKE_CXX_FLAGS "${_CMAKE_CXX_FLAGS_INITIAL} ${COMPILE_FLAGS}" CACHE STRING "" FORCE)
+string(APPEND CMAKE_C_FLAGS_INIT " ${COMPILE_FLAGS}")
+string(APPEND CMAKE_CXX_FLAGS_INIT " ${COMPILE_FLAGS}")
 
 set(LINK_FLAGS
     # Prevent CMake from attempting to invoke mt.exe. It only recognizes the slashed form and not the dashed form.
     /manifest:no
 
     -libpath:"${ATLMFC_LIB}/${WINSDK_ARCH}"
     -libpath:"${MSVC_LIB}/${WINSDK_ARCH}"
     -libpath:"${WINSDK_LIB}/ucrt/${WINSDK_ARCH}"
     -libpath:"${WINSDK_LIB}/um/${WINSDK_ARCH}")
 
 if(case_sensitive_filesystem)
   # Ensure all sub-configures use the top-level symlinks dir instead of generating their own.
   init_user_prop(winsdk_lib_symlinks_dir)
   if(NOT winsdk_lib_symlinks_dir)
     set(winsdk_lib_symlinks_dir "${CMAKE_BINARY_DIR}/winsdk_lib_symlinks")
     generate_winsdk_lib_symlinks("${WINSDK_LIB}/um/${WINSDK_ARCH}" "${winsdk_lib_symlinks_dir}")
     init_user_prop(winsdk_lib_symlinks_dir)
   endif()
   list(APPEND LINK_FLAGS
        -libpath:"${winsdk_lib_symlinks_dir}")
 endif()
 
 string(REPLACE ";" " " LINK_FLAGS "${LINK_FLAGS}")
-
-# See explanation for compiler flags above for the _INITIAL variables.
-set(_CMAKE_EXE_LINKER_FLAGS_INITIAL "${CMAKE_EXE_LINKER_FLAGS}" CACHE STRING "")
-set(CMAKE_EXE_LINKER_FLAGS "${_CMAKE_EXE_LINKER_FLAGS_INITIAL} ${LINK_FLAGS}" CACHE STRING "" FORCE)
-
-set(_CMAKE_MODULE_LINKER_FLAGS_INITIAL "${CMAKE_MODULE_LINKER_FLAGS}" CACHE STRING "")
-set(CMAKE_MODULE_LINKER_FLAGS "${_CMAKE_MODULE_LINKER_FLAGS_INITIAL} ${LINK_FLAGS}" CACHE STRING "" FORCE)
-
-set(_CMAKE_SHARED_LINKER_FLAGS_INITIAL "${CMAKE_SHARED_LINKER_FLAGS}" CACHE STRING "")
-set(CMAKE_SHARED_LINKER_FLAGS "${_CMAKE_SHARED_LINKER_FLAGS_INITIAL} ${LINK_FLAGS}" CACHE STRING "" FORCE)
+string(APPEND CMAKE_EXE_LINKER_FLAGS_INIT " ${LINK_FLAGS}")
+string(APPEND CMAKE_MODULE_LINKER_FLAGS_INIT " ${LINK_FLAGS}")
+string(APPEND CMAKE_SHARED_LINKER_FLAGS_INIT " ${LINK_FLAGS}")
 
 # CMake populates these with a bunch of unnecessary libraries, which requires
 # extra case-correcting symlinks and what not. Instead, let projects explicitly
 # control which libraries they require.
 set(CMAKE_C_STANDARD_LIBRARIES "" CACHE STRING "" FORCE)
 set(CMAKE_CXX_STANDARD_LIBRARIES "" CACHE STRING "" FORCE)
 
 # Allow clang-cl to work with macOS paths.
 set(CMAKE_USER_MAKE_RULES_OVERRIDE "${CMAKE_CURRENT_LIST_DIR}/ClangClCMakeCompileRules.cmake")