This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by gbedwell on Mar 9 2015, 4:11 PM.

Details

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

Diff Detail

Event Timeline

gbedwell updated this revision to Diff 21527.Mar 9 2015, 4:11 PM
gbedwell retitled this revision from to [CMake] Don't pass in MSVC warning flags as definitions.
gbedwell updated this object.
gbedwell edited the test plan for this revision. (Show Details)
gbedwell added reviewers: Bigcheese, aaron.ballman.
gbedwell updated this object.
gbedwell added a subscriber: Unknown Object (MLST).
rnk added a subscriber: rnk.Mar 9 2015, 4:48 PM
rnk added inline comments.
cmake/modules/HandleLLVMOptions.cmake
297

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 added a comment.Mar 9 2015, 4:52 PM

BTW, this seems like an overall reasonable idea.

cmake/modules/HandleLLVMOptions.cmake
296

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.

Thanks for looking at my patch!

cmake/modules/HandleLLVMOptions.cmake
297

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 added inline comments.Mar 10 2015, 5:04 AM
cmake/modules/HandleLLVMOptions.cmake
297

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 accepted this revision.Mar 10 2015, 9:13 AM
rnk added a reviewer: rnk.

lgtm

cmake/modules/HandleLLVMOptions.cmake
296

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

297

Nope, this seems good to me.

This revision is now accepted and ready to land.Mar 10 2015, 9:13 AM
This revision was automatically updated to reflect the committed changes.

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 added a comment.Mar 13 2015, 11:29 AM

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.

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

Thanks again. Committed at r232727.