[CMake] Don't pass in MSVC warning flags as definitions

[gbedwell]

Summary

A version of this change was originally part of my patch at http://reviews.llvm.org/D7828 where it was suggested to split this part out into a separate review. Currently this patch is effectively an NFC but is required as a prerequisite for my other changes to embed Windows version information into executable and dll files as the warning flags are not recognised by the Microsoft resource compiler and the CMake Ninja generator does not filter out invalid flags from the definitions.

Here is my previous comment on this change (from http://reviews.llvm.org/D7828 ):
"I've had to make small change to HandleLLVMOptions.cmake to support the CMake Ninja generator. Previously the MSVC-specific warning flags were being added from add_llvm_definitions. The Visual Studio generators use a filter to make sure that only valid flags are passed through to the resource compiler, however the Ninja generator expects that this will only ever contain definitions and passes everything through unfiltered leading to errors from the resource compiler when given invalid flags. I've changed the behaviour so these warning flags are now added to CMAKE_C_FLAGS and CMAKE_CXX_FLAGS directly."

Thanks!

-Greg

[rnk]

cmake/modules/HandleLLVMOptions.cmake

297 ↗

(On Diff #21527)

Does this actually work in the VS generator? I think you need to tweak CMAKE_C_DEBUG_FLAGS, CMAKE_C_RELEASE_FLAGS, etc. You can search around for instances of for loops over CMAKE_BUILD_TYPES to see how this is done.

[rnk]

BTW, this seems like an overall reasonable idea.

cmake/modules/HandleLLVMOptions.cmake

296 ↗

(On Diff #21527)

It might also be faster to string-ify the list like this:

string(REPLACE ";" " " msvc_warning_flags "${msvc_warning_flags}")

And append that one string to every CMAKE_*_FLAGS var we need to touch.

[gbedwell]

Thanks for looking at my patch!

cmake/modules/HandleLLVMOptions.cmake

297 ↗

(On Diff #21527)

With the VS2013 generator 'Visual Studio 12 Win64' and CMake 3.1.3 I can see these flags in the solution properties for all build configurations so it seems to work on my setup. I'll test on CMake 2.8.12.2 on the plain 'Visual Studio 12' generator too just to be sure. If these tests show it to work, does this implementation seem reasonable? Are there other reasons why it is preferable for them to appear in the individual build config flag lists?

[gbedwell]

cmake/modules/HandleLLVMOptions.cmake

297 ↗

(On Diff #21527)

I've now verified that with this patch the expected warnings are also disabled/promoted for all build configs with CMake 2.8.12.2 and the plain Visual Studio 12 generator. I'm happy to change it, but I don't think it's required unless there's another reason I've not not considered.

[rnk]

lgtm

cmake/modules/HandleLLVMOptions.cmake

296 ↗

(On Diff #21527)

No need to implement this, given that we are only appending twice.

297 ↗

(On Diff #21527)

Nope, this seems good to me.

[gbedwell]

Thanks for the review! I committed at r231924 but got a new failure on the sanitizer-windows bot so reverted at r231925 to give me time to investigate.

The errors introduced were in the form:
clang.exe: error: unknown argument: '-wd4146'

I can see now that in compiler-rt, CMAKE_C_FLAGS gets passed into clang.exe command lines even when guarded by (MSVC).

My first thought was to make use of HandleLLVMOptions.cmake's add_flag_if_supported macro to ensure that the flag is actually supported by the compiler. I had to modify it slightly to make it work for MSVC as it implicitly expects the compiler to support the -Werror flag, but sadly no luck. It performs the test against cl.exe but still then still adds it to the clang.exe command line later on.

It seems like a potential solution may be to add a filter to CompilerRTCompile's clang_compile macro to remove MSVC specific flags like this at the same time that it replaces '/' with '-'. I'll proceed to try that solution in the meantime, but any other suggestions are more than welcome.

To re-summarize the problem I'm trying to solve:

• currently MSVC warning flags are added as definitions rather than as flags
• I want to make a change to use the Microsoft resource compiler to embed Windows version metadata in our exe and dll files.
• With CMake's Visual Studio generators definitions are automatically filtered to remove anything invalid when using the resource compiler
• With CMake's Ninja generator and MSVC all definitions are passed through to the resource compiler unfiltered
• The warning flags are not valid for the resource compiler causing it to fail with an unknown flag error.
• This patch adds the warning flags as flags rather than as definitions which fixes the above problem, but introduces a new problem in compiler-rt as these flags are then passed into both cl.exe and clang.exe, for the latter of which the flags are invalid, causing an error.

Thanks,

-Greg

[rnk]

I think clang_compile only needs to pass down some of CMAKE_C_FLAGS in MSVC mode. For example, /D, /U, and /O2 are all translated correctly here. Other than those flags, I don't think it needs to do anything. Let me try to write a patch for that.

[gbedwell]

In D8188#140580, @rnk wrote:

I think clang_compile only needs to pass down some of CMAKE_C_FLAGS in MSVC mode. For example, /D, /U, and /O2 are all translated correctly here. Other than those flags, I don't think it needs to do anything. Let me try to write a patch for that.

Ah, thanks! I just created http://reviews.llvm.org/D8326 to filter out the above problematic flags (and would then make a small modification to this patch to use the '/flag' form for them here, but if you think a more general solution would be better I'm very happy to defer to you.

Thanks again!

-Greg

[gbedwell]

Thanks again. Committed at r232727.

llvm/trunk/cmake/modules/HandleLLVMOptions.cmake

if( NOT (${CMAKE_VERSION} VERSION_LESS 2.8.11) )
     # value (1 MB) which is not enough for us in tasks such as parsing recursive
     # C++ templates in Clang.
     set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} /STACK:10000000")
   endif()
 
   if( MSVC11 )
     add_llvm_definitions(-D_VARIADIC_MAX=10)
   endif()
-  
   # Add definitions that make MSVC much less annoying.
   add_llvm_definitions(
     # For some reason MS wants to deprecate a bunch of standard functions...
     -D_CRT_SECURE_NO_DEPRECATE
     -D_CRT_SECURE_NO_WARNINGS
     -D_CRT_NONSTDC_NO_DEPRECATE
     -D_CRT_NONSTDC_NO_WARNINGS
     -D_SCL_SECURE_NO_DEPRECATE
     -D_SCL_SECURE_NO_WARNINGS
+    )
 
+  set(msvc_warning_flags
     # Disabled warnings.
     -wd4146 # Suppress 'unary minus operator applied to unsigned type, result still unsigned'
     -wd4180 # Suppress 'qualifier applied to function type has no meaning; ignored'
     -wd4244 # Suppress ''argument' : conversion from 'type1' to 'type2', possible loss of data'
     -wd4258 # Suppress ''var' : definition from the for loop is ignored; the definition from the enclosing scope is used'
     -wd4267 # Suppress ''var' : conversion from 'size_t' to 'type', possible loss of data'
     -wd4291 # Suppress ''declaration' : no matching operator delete found; memory will not be freed if initialization throws an exception'
     -wd4345 # Suppress 'behavior change: an object of POD type constructed with an initializer of the form () will be default-initialized'
     -wd4351 # Suppress 'new behavior: elements of array 'array' will be default initialized'
     -wd4355 # Suppress ''this' : used in base member initializer list'
     -wd4456 # Suppress 'declaration of 'var' hides local variable'
     -wd4457 # Suppress 'declaration of 'var' hides function parameter'
     -wd4458 # Suppress 'declaration of 'var' hides class member'
     -wd4459 # Suppress 'declaration of 'var' hides global declaration'
     -wd4503 # Suppress ''identifier' : decorated name length exceeded, name was truncated'
     -wd4624 # Suppress ''derived class' : destructor could not be generated because a base class destructor is inaccessible'
     -wd4722 # Suppress 'function' : destructor never returns, potential memory leak
     -wd4800 # Suppress ''type' : forcing value to bool 'true' or 'false' (performance warning)'
-    
     # Promoted warnings.
     -w14062 # Promote 'enumerator in switch of enum is not handled' to level 1 warning.
 
     # Promoted warnings to errors.
     -we4238 # Promote 'nonstandard extension used : class rvalue used as lvalue' to error.
     )
 
   # Enable warnings
   if (LLVM_ENABLE_WARNINGS)
-    add_llvm_definitions( /W4 )
+    append("/W4" msvc_warning_flags)
     if (LLVM_ENABLE_PEDANTIC)
       # No MSVC equivalent available
     endif (LLVM_ENABLE_PEDANTIC)
   endif (LLVM_ENABLE_WARNINGS)
   if (LLVM_ENABLE_WERROR)
-    add_llvm_definitions( /WX )
+    append("/WX" msvc_warning_flags)
   endif (LLVM_ENABLE_WERROR)
+
+  foreach(flag ${msvc_warning_flags})
+    append("${flag}" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+  endforeach(flag)
+
 elseif( LLVM_COMPILER_IS_GCC_COMPATIBLE )
   if (LLVM_ENABLE_WARNINGS)
     append("-Wall -W -Wno-unused-parameter -Wwrite-strings" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
     append("-Wcast-qual" CMAKE_CXX_FLAGS)
 
     # Turn off missing field initializer warnings for gcc to avoid noise from
     # false positives with empty {}. Turn them on otherwise (they're off by
     # default for clang).