Details
- Reviewers
efriedma - Group Reviewers
Restricted Project - Commits
- rGdf6291a666d3: [CMake][MSVC] Compile with `/permissive-`
Diff Detail
Event Timeline
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-?
> 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.
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.
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 | ||
|---|---|---|
| 528 | I think if we're adding /permissive-, we don't need to explicitly pass Zc:strictStrings or Zc:rvalueCast. | |
| 565 | 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." | |
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?
@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
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.
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?
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).
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.
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++.
I think if we're adding /permissive-, we don't need to explicitly pass Zc:strictStrings or Zc:rvalueCast.