[CMake][MSVC] Compile with `/permissive-`

[MehdiChinoune]

Summary

see: https://docs.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance

[MehdiChinoune]

This comment was removed by MehdiChinoune.

[MehdiChinoune]

ping!

[efriedma]

This isn't a perfect mapping. In clang/gcc, -pedantic just turns on additional warnings. MSVC /permissive- actually changes the behavior of the compiler, so code that compiles with /permissive- might not compile without it, or vice versa. If you really want an option to turn on MSVC strict conformance options, I think we should call it something else.

Does LLVM build successfully with /permissive-?

[MehdiChinoune]

> Does LLVM build successfully with /permissive-?
Yes, It does. You could check the windows build yourself
There was a failure with flang that we fixed it.

[MehdiChinoune]

ping!

[efriedma]

Like I said before, I think it makes sense to have this option, but I don't think it makes sense to call this LLVM_ENABLE_PEDANTIC because of the behavior differences.

[MehdiChinoune]

Address reviewer comment

[efriedma]

Turning this on by default probably makes sense. The documentation does say "By default, the /permissive- option is set in new projects created by Visual Studio 2017 version 15.5 and later versions", so it seems like Microsoft is recommending users do this.

llvm/cmake/modules/HandleLLVMOptions.cmake

524–525

I think if we're adding /permissive-, we don't need to explicitly pass Zc:strictStrings or Zc:rvalueCast.

557

Maybe write out a bit more; something like "Enable standards conformance mode. This ensures handling of various C/C++ constructs is more similar to other compilers."

[MehdiChinoune]

Apply suggestions from reviewer

[efriedma]

LGTM

[MehdiChinoune]

Could someone push it for me, Please!

[MehdiChinoune]

Thanks

[paulkirth]

Hi, we're seeing a build failure on windows that we've traced back to this revision.

You can find the failing bot here: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-windows-x64/b8810806780048763729/overview

The error is as follows:

FAILED: tools/polly/lib/CMakeFiles/obj.Polly.dir/Transform/ScheduleTreeTransform.cpp.obj 
C:\b\s\w\ir\x\w\cipd\bin\clang-cl.exe  /nologo -TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GNU_SOURCE -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools\polly\lib -IC:\b\s\w\ir\x\w\llvm-llvm-project\polly\lib -Itools\polly\include -IC:\b\s\w\ir\x\w\llvm-llvm-project\polly\lib\External -IC:\b\s\w\ir\x\w\llvm-llvm-project\polly\lib\External\pet\include -IC:\b\s\w\ir\x\w\llvm-llvm-project\polly\lib\External\isl\include -Itools\polly\lib\External\isl\include -IC:\b\s\w\ir\x\w\llvm-llvm-project\polly\include -IC:\b\s\w\ir\cache\vpython\a74aab\Lib\site-packages\tensorflow\include -Iinclude -IC:\b\s\w\ir\x\w\llvm-llvm-project\llvm\include -IC:\b\s\w\ir\x\w\staging\zlib_install\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:__cplusplus /Oi /Brepro /bigobj /permissive- /W4  -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation /Gw -no-canonical-prefixes /EHs-c- /MT /O2 /Ob2    /EHs-c- /GR- -UNDEBUG -std:c++14 /showIncludes /Fotools\polly\lib\CMakeFiles\obj.Polly.dir\Transform\ScheduleTreeTransform.cpp.obj /Fdtools\polly\lib\CMakeFiles\obj.Polly.dir\ -c C:\b\s\w\ir\x\w\llvm-llvm-project\polly\lib\Transform\ScheduleTreeTransform.cpp
In file included from C:\b\s\w\ir\x\w\llvm-llvm-project\polly\lib\Transform\ScheduleTreeTransform.cpp:13:
In file included from C:\b\s\w\ir\x\w\llvm-llvm-project\polly\include\polly/ScheduleTreeTransform.h:16:
C:\b\s\w\ir\x\w\llvm-llvm-project\polly\include\polly/Support/ISLTools.h(59,36): error: no member named 'max' in namespace 'std'
      : List(&List), Position(std::max(List.size().release(), 0)) {}
                              ~~~~~^
1 error generated.

While this at first appeared to be a change in header dependencies, we bisected the failure down to this commit. It looks like there may be some subtle differences in the way the flags are used or propagated in clang-cl.

Can you please take a look, and revert if it cannot be easily fixed?

[aeubanks]

In D125263#3600114, @paulkirth wrote:

Hi, we're seeing a build failure on windows that we've traced back to this revision.

You can find the failing bot here: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-windows-x64/b8810806780048763729/overview

The error is as follows:

FAILED: tools/polly/lib/CMakeFiles/obj.Polly.dir/Transform/ScheduleTreeTransform.cpp.obj 
C:\b\s\w\ir\x\w\cipd\bin\clang-cl.exe  /nologo -TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GNU_SOURCE -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools\polly\lib -IC:\b\s\w\ir\x\w\llvm-llvm-project\polly\lib -Itools\polly\include -IC:\b\s\w\ir\x\w\llvm-llvm-project\polly\lib\External -IC:\b\s\w\ir\x\w\llvm-llvm-project\polly\lib\External\pet\include -IC:\b\s\w\ir\x\w\llvm-llvm-project\polly\lib\External\isl\include -Itools\polly\lib\External\isl\include -IC:\b\s\w\ir\x\w\llvm-llvm-project\polly\include -IC:\b\s\w\ir\cache\vpython\a74aab\Lib\site-packages\tensorflow\include -Iinclude -IC:\b\s\w\ir\x\w\llvm-llvm-project\llvm\include -IC:\b\s\w\ir\x\w\staging\zlib_install\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:__cplusplus /Oi /Brepro /bigobj /permissive- /W4  -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation /Gw -no-canonical-prefixes /EHs-c- /MT /O2 /Ob2    /EHs-c- /GR- -UNDEBUG -std:c++14 /showIncludes /Fotools\polly\lib\CMakeFiles\obj.Polly.dir\Transform\ScheduleTreeTransform.cpp.obj /Fdtools\polly\lib\CMakeFiles\obj.Polly.dir\ -c C:\b\s\w\ir\x\w\llvm-llvm-project\polly\lib\Transform\ScheduleTreeTransform.cpp
In file included from C:\b\s\w\ir\x\w\llvm-llvm-project\polly\lib\Transform\ScheduleTreeTransform.cpp:13:
In file included from C:\b\s\w\ir\x\w\llvm-llvm-project\polly\include\polly/ScheduleTreeTransform.h:16:
C:\b\s\w\ir\x\w\llvm-llvm-project\polly\include\polly/Support/ISLTools.h(59,36): error: no member named 'max' in namespace 'std'
      : List(&List), Position(std::max(List.size().release(), 0)) {}
                              ~~~~~^
1 error generated.

While this at first appeared to be a change in header dependencies, we bisected the failure down to this commit. It looks like there may be some subtle differences in the way the flags are used or propagated in clang-cl.

Can you please take a look, and revert if it cannot be easily fixed?

I've pushed https://reviews.llvm.org/rGc80b88ee29f34078d2149de94e27600093e6c7c0 as a speculative fix, perhaps that will fix it?

[paulkirth]

@aeubanks That was my first thought too, but I think there may be something more subtle going on here with the /permissive- flag that is causing the underlying issue. AFAICT from the documentation linked in the summary, it shouldn't change how things are included, and this is compiling fine on other platforms. My guess is that clang-cl isn't handling this flag the same way as MSVC, and thus things like /Zc:rvalueCast aren't set correctly.

Windows isn't a platform I'm that well versed on, so I'm happy to defer, but if there is a subtle thing happening here, I'd rather us look into it now than try to diagnose some subtle thing down the line.

Anyway, if that makes our bots green, I'll let you know.

[aeubanks]

In D125263#3600222, @paulkirth wrote:

@aeubanks That was my first thought too, but I think there may be something more subtle going on here with the /permissive- flag that is causing the underlying issue. AFAICT from the documentation linked in the summary, it shouldn't change how things are included, and this is compiling fine on other platforms. My guess is that clang-cl isn't handling this flag the same way as MSVC, and thus things like /Zc:rvalueCast aren't set correctly.

Windows isn't a platform I'm that well versed on, so I'm happy to defer, but if there is a subtle thing happening here, I'd rather us look into it now than try to diagnose some subtle thing down the line.

Anyway, if that makes our bots green, I'll let you know.

/Zc:rvalueCast seems to be ignored within clang-cl so I don't think that's the issue.
https://github.com/llvm/llvm-project/blob/1c2b756cd6f9f9408863fb0e91f55731f81b46d9/clang/include/clang/Driver/Options.td#L6730

[paulkirth]

Well, that fixed our bots, so I'm going to guess that we just made an error when bisecting. Thanks @aeubanks for the quick and simple patch.

[dyung]

We are seeing failures in our internal Windows build with an older version of the Visual C++ compiler that I bisected back to this commit. The errors are all similar to this:

C:\src\upstream\llvm_clean_git\clang\include\clang/Basic/Sanitizers.def(41,1): error C2131: expression did not evaluate to a constant [C:\src\upstream\df6291a666d3-PS4-Release\tools\clang\lib\Basic\obj.clangBasic.vcxproj]
C:\src\upstream\llvm_clean_git\clang\include\clang/Basic/Sanitizers.def(41,1): message : failure was caused by call of undefined function or one not declared 'constexpr' [C:\src\upstream\df6291a666d3-PS4-Release\tools\clang\lib\Basic\obj.clangBasic.vcxproj]
C:\src\upstream\llvm_clean_git\clang\include\clang/Basic/Sanitizers.def(41,1): message : see usage of 'clang::SanitizerMask::bitPosToMask' [C:\src\upstream\df6291a666d3-PS4-Release\tools\clang\lib\Basic\obj.clangBasic.vcxproj]

Our build machines are using version 19.28.29915.0 of the C++ compiler (corresponding to Visual Studio 2019 Version 16.8-9) which I believe should still be supported. Can you take a look?

[efriedma]

We are seeing failures in our internal Windows build with an older version of the Visual C++ compiler that I bisected back to this commit. The errors are all similar to this:

I don't see any obvious reason for this to fail, or be affected by /permissive-. Can you attach and/or send me preprocessed source for the failing file?

---

I assume the issue with std::max was that "/permissive-" turns on two-phase lookup... but clang-cl and cl have slightly different views about what exactly two-phase lookup means. So cl is fine, but clang-cl prints an error (and other platforms are fine because <algorithm> is somehow included anyway).

[dyung]

In D125263#3602731, @efriedma wrote:

We are seeing failures in our internal Windows build with an older version of the Visual C++ compiler that I bisected back to this commit. The errors are all similar to this:

I don't see any obvious reason for this to fail, or be affected by /permissive-. Can you attach and/or send me preprocessed source for the failing file?

I've worked around the issue locally by disabling the call to /permissive-. I'll see what I can do to get a preprocessed source.

[dyung]

In D125263#3602731, @efriedma wrote:

We are seeing failures in our internal Windows build with an older version of the Visual C++ compiler that I bisected back to this commit. The errors are all similar to this:

I don't see any obvious reason for this to fail, or be affected by /permissive-. Can you attach and/or send me preprocessed source for the failing file?

Hi, I have create github issue #56173 with a preprocessed source that should show the error when built using an older version of Visual C++.

llvm/cmake/modules/HandleLLVMOptions.cmake

if (CLANG_CL AND LINKER_IS_LLD)
     # linking. This flag is orthogonal from /Zi, /Z7, and other flags that
     # enable debug info emission, and only has an effect if those are also in
     # use.
     string(FIND "${all_linker_flags_uppercase}" "/DEBUG" linker_flag_idx)
     if (${linker_flag_idx} GREATER -1)
       add_flag_if_supported("-gcodeview-ghash" GCODEVIEW_GHASH)
     endif()
   endif()
 
-  # Disable string literal const->non-const type conversion.
-  # "When specified, the compiler requires strict const-qualification
-  # conformance for pointers initialized by using string literals."
-  append("/Zc:strictStrings" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
-
   # "Generate Intrinsic Functions".
   append("/Oi" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 
-  # "Enforce type conversion rules".
-  append("/Zc:rvalueCast" CMAKE_CXX_FLAGS)
-
   if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND NOT LLVM_ENABLE_LTO)
     # clang-cl and cl by default produce non-deterministic binaries because
     # link.exe /incremental requires a timestamp in the .obj file.  clang-cl
     # has the flag /Brepro to force deterministic binaries. We want to pass that
     # whenever you're building with clang unless you're passing /incremental
     # or using LTO (/Brepro with LTO would result in a warning about the flag
     # being unused, because we're not generating object files).
     # This checks CMAKE_CXX_COMPILER_ID in addition to check_cxx_compiler_flag()
if (SUPPORTS_BREPRO)
         message(WARNING "/Brepro not compatible with /INCREMENTAL linking - builds will be non-deterministic")
       endif()
     endif()
   endif()
   # By default MSVC has a 2^16 limit on the number of sections in an object file,
   # but in many objects files need more than that. This flag is to increase the
   # number of sections.
   append("/bigobj" CMAKE_CXX_FLAGS)
+
+  # Enable standards conformance mode.
+  # This ensures handling of various C/C++ constructs is more similar to other compilers.
+  append("/permissive-" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 endif( MSVC )
 
 # Warnings-as-errors handling for GCC-compatible compilers:
 if ( LLVM_COMPILER_IS_GCC_COMPATIBLE )
   append_if(LLVM_ENABLE_WERROR "-Werror" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
   append_if(LLVM_ENABLE_WERROR "-Wno-error" CMAKE_REQUIRED_FLAGS)
 endif( LLVM_COMPILER_IS_GCC_COMPATIBLE )