[PATCH] D55056: [CMake] Default options for faster executables on MSVC

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added inline comments.


================
Comment at: llvm/trunk/cmake/modules/ChooseMSVCCRT.cmake:70-71
+
+	  # Make /MT the default in Release builds to make them faster
+	  # and avoid the DLL function thunking.
+	  if ((${build} STREQUAL "MINSIZEREL") OR
----------------
rnk wrote:
> Isn't it a bad practice to statically link the CRT into DLLs? We have a few (LLDB), and this will get picked up by it. However, I notice that we set this option in the official `build_llvm_package.bat` script that @hans uses to make the Windows releases:
> https://github.com/llvm-git-prototype/llvm/blob/master/llvm/utils/release/build_llvm_package.bat#L47
> 
> Anyway, this is just the default, and it's reasonable for people building clang, lld, and other static executables, which is most people.
> Isn't it a bad practice to statically link the CRT into DLLs?

Yes, only if you dynamically load (third-party) DLLs in your executable at runtime (plugins say) The EXE and DLLs might be compiled with a different CRTs and you might end up accessing wrong class/structure layouts in memory.

However only LTO seems to be compiled as a DLL. I'm not sure why, and how it's used? Is it supposed to be dynamically loaded by LLD?


================
Comment at: llvm/trunk/cmake/modules/HandleLLVMOptions.cmake:387
+      foreach(FLAG EXE MODULE SHARED STATIC)
+	    string(REGEX REPLACE "[-/](INCREMENTAL:YES|INCREMENTAL:NO|INCREMENTAL)" "/INCREMENTAL:NO" CMAKE_${FLAG}_LINKER_FLAGS_${CONFIG} "${CMAKE_${FLAG}_LINKER_FLAGS_${CONFIG}}")
+	  endforeach()
----------------
zturner wrote:
> rnk wrote:
> > Looks like it needs one more indentation level to be in the loop over target types. We don't have a formatter tool for cmake, but it would be easier to read if this was broken up onto multiple lines.
> Instead of replacing all the options in an existing command line, why don't we just have something like:
> 
> ```
> if (CMAKE_CXX_COMPILER_ID MATCHES "MSVC" AND NOT LLVM_ENABLE_INCREMENTAL_LINK AND CMAKE_BUILD_TYPE != "Debug")
>   append("/INCREMENTAL:NO" CMAKE_LINKER_FLAGS)
> endif()
> ```
(note: `CMAKE_LINKER_FLAGS` doesn't exist, rather `CMAKE_EXE_LINKER_FLAGS CMAKE_MODULE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS CMAKE_STATIC_LINKER_FLAGS`)

I've tried [[ https://stackoverflow.com/questions/21569478/use-cmake-to-set-enable-incremental-linking-to-no-in-vs2012 | several approches ]] including the one you suggest, however replacing the flag (REGEX REPLACE) is the only one that works with my CMake 3.12.2. Probably because how `/INCREMENTAL` is [[ https://github.com/Kitware/CMake/blob/bdc5618e18b9868f46e48641cdd35f361199f28e/Modules/Platform/Windows-MSVC.cmake#L288 | forcibly ]] set by CMake, and probably because how flags are then [[ https://github.com/Kitware/CMake/blob/07cfb18f9d29cfc0588ede928846a03ec5599c48/Source/cmVS141LinkFlagTable.h#L143 | rendered ]] into the .vcxproj by the generator.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55056