The only difference is that LLVM_ENABLE_WERROR is set to OFF
by default, but we enable this in a standalone flang build
This commit fixes some windows issues with the flags
When I do an out-of-tree build of flang, this is a compilation command that I'm seeing:
clang++ -DGTEST_HAS_RTTI=0 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I../../include -Iinclude -isystem $LLVM/mlir/include -isystem $LLVM/build/Release-clang-9/tools/mlir/include -isystem $LLVM/llvm/include -isystem $LLVM/build/Release-clang-9/include -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -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 -Wstring-conversion -fdiagnostics-color -Wno-string-conversion -Wno-unused-command-line-argument -Wstring-conversion -Wcovered-switch-default -Wno-nested-anon-types -O2 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -fno-exceptions -fno-rtti -std=c++17 -MD -MT lib/Common/CMakeFiles/obj.FortranCommon.dir/Fortran.cpp.o -MF lib/Common/CMakeFiles/obj.FortranCommon.dir/Fortran.cpp.o.d -o lib/Common/CMakeFiles/obj.FortranCommon.dir/Fortran.cpp.o -c ../../lib/Common/Fortran.cpp
There is no -pedantic there.
That doesn't work. add_flang_executable is meant for executables to be installed. Not for test executables.
Fails with a bunch of
CMake Error at /home/isuru/projects/llvm-project/llvm/cmake/modules/LLVMProcessSources.cmake:112 (message): Found unknown source file uint128.cpp Please update /home/isuru/projects/llvm-project/flang/unittests/Evaluate/CMakeLists.txt Call Stack (most recent call first): /home/isuru/projects/llvm-project/llvm/cmake/modules/LLVMProcessSources.cmake:62 (llvm_check_source_file_list) /home/isuru/projects/llvm-project/llvm/cmake/modules/AddLLVM.cmake:766 (llvm_process_sources) /home/isuru/projects/llvm-project/flang/cmake/modules/AddFlang.cmake:101 (add_llvm_executable) /home/isuru/projects/llvm-project/flang/unittests/Evaluate/CMakeLists.txt:93 (add_flang_executable)
I don't think -Wlong-long is enabled in C++11 or above with -pedantic. So, that's harmless.
From gcc docs,
I don't think we should enable Werror by default anymore now that we're in LLVM. It often prevents building with newer compilers (LLVM sees issues with this a lot already) and can be enabled by hand anyway for developer builds.
I reverted back. After this is accepted I will make a new differential to change the default where it can be discussed.
This change allows MSVC builds to proceed further and I don't want this to be held back by a decision on the default for werror.
I'm in favour of not having Werror on by default so that we don't hard fail on new compiler releases. I'm not massively fussed right now while we're not heading towards a release, but once we're in the official releases of LLVM this is really important; we absolutely should not hard fail in released versions when new versions of other compilers are released. In my opinion, an example like LLVM 11 failing to compile with GCC 12 out of the box because that added new warnings would be a really big problem. I'd rather just remove the Werror now to stop this situation ever happening.
Projects having Werror enabled by default one of the reasons why it is hard to implement new warnings. Please don't.
For instance, I fixed false-negative warnings in Clang. The patch was reverted because it broke the Chrome build.
Flang and clang in the same repository gives the unique possibility that with adding a warning flag, I can fix the cause of the warning in flang. But generally, e.g. a new version of gcc emitting new warnings when compiling flang, this is not the case.
As I said before, my strong preference is also to not have Werror by default; however I'm happy to let this patch go ahead and open a separate patch for that if this fixes people's builds.
@tskeith, no. That can only be done in an out-of-tree build. LLVM doesn't have a mechanism to do it for a specific subproject and adding it here would mean copying that code from LLVM here.
(Using the current code in master is not desirable as the flags in master do not work for some compilers like MSVC.)
Could you let us know why you think this is necessary when LLVM_ENABLE_WERROR and LLVM_ENABLE_PEDANTIC exist?
It's perfectly acceptable to stand up build bots that enable these two flags and enforce that those build bots always pass in master; I just really don't think we want these flags by default when new compiler releases after our own releases (and therefore completely out of our control) can cause build failures with these flags.
I want to make sure that my flang changes (and everyone else's) build without warnings from the compilers I test with. LLVM_ENABLE_WERROR etc. is fine if LLVM and MLIR don't have any warnings, but that hasn't been my experience.
Note that we have a disjoint set of host environments, for me ninja check-mlir does not produce any warning locally and passes with -DLLVM_ENABLE_WERROR=ON (using clang-10 on Linux), but that does not mean it'll work for everyone else. Me building with Werror won't guarantee that your environment will be warning free.
Patches are welcome to fix warnings everywhere though if you encounter some! Folks have been sending patches to MLIR to fix warnings that they see on their platforms (Windows frequently, but also other Unix systems than Linux).
Looking at one clang invocation, these flags are used for me when building LLVM right now FYI: -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -fvisibility=hidden -fno-exceptions -fno-rtti -UNDEBUG -std=c++14
I'm not opposed to adding a FLANG_ENABLE_WERROR and FLANG_ENABLE_PEDANTIC, there's even precedent for this in other LLVM projects. That way we could fine-tune which projects should have -Werror and -pedantic turned on and only enable it for those we care about. Then we can add BuildBots with these flags turned on for the compilers we test with to ensure all changes pass that.
What I'm opposed to is having this on by default; this is bad for users that just download the source of x release and find they can't build it with y compiler that came out after that release because of warnings promoted to errors.
We discussed this on the technical call on 18 May - see https://docs.google.com/document/d/1Z2U5UAtJ-Dag5wlMaLaW1KRmNgENNAYynJqLW2j2AZQ
Summarising the discussion:
On the call we proposed a compromise of keeping -Werror on by default for Flang but having a local Flang CMake flag to disable it that folks can use that. @isuruf would that unblock you and fix the issue that caused you to submit the patch originally?
For out-of-tree builds, this is easy to do. For in-tree builds, I'll have to duplicate the logic of https://github.com/llvm/llvm-project/blob/e3e0367f9ba142b5a3d4c2362f11b3fcbacfc98d/llvm/cmake/modules/HandleLLVMOptions.cmake#L413-L415 and https://github.com/llvm/llvm-project/blob/e3e0367f9ba142b5a3d4c2362f11b3fcbacfc98d/llvm/cmake/modules/HandleLLVMOptions.cmake#L473-L477
If you are okay with duplicating code like this I'll add an option.
We wondered whether the way the Cmake files are written already supports this through adding -DLLVM_ENABLE_WARNINGS=0 to the build line. Pat McCormick is going to look and reply.
What's LLVM_ENABLE_WARNINGS got to do with anything?
Different compilers have different set of warnings: enabling Werror by default means it increases the likelihood that the build will just fails for a user that has a compiler version (+stdlib) you're not testing against continuously. This isn't a good experience to offer as a *default* settings.
How about only default to ON if the user has not already set LLVM_ENABLE_WARNING? That lets the user say LLVM_ENABLE_WARNING=OFF.
How about only default to ON if the user has not already set LLVM_ENABLE_PEDANTIC?
How about preserving this line if LLVM_ENABLE_WARNINGS isn't otherwise set? This way, people who don't want warnings can just -DLLVM_ENABLE_WARNING=OFF.
It would be nice if these options were preserved with LLVM_COMPILER_IS_GCC_COMPATIBLE, beside -Werror, which is controlled elsewhere, and also, maybe -Wpedantic.
Is there a tradition of documenting a single-letter option like /X?
Why not include this under LLVM_COMPILER_IS_GCC_COMPATIBLE?
Is rtti really needed here?
Is rtti really needed here?
That is how CMake options work. The value we are setting here is a default and the user can override it on the command line.
Same as before
On a in-tree build, this option is already defined. In an out-of-tree build, this option is defined in Line 70
-Wall -Wextra -Wcast-qual -Wimplicit-fallthrough -Wdelete-non-virtual-dtor are all covered by LLVM_ENABLE_WARNINGS=ON which is the default.
-Wno-unused-parameter is added by LLVM_ENABLE_WARNINGS
Otherwise, there's a warning. See https://github.com/llvm/llvm-project/blob/6701993027f8af172d7ba697884459261b00e3c6/llvm/cmake/modules/AddLLVM.cmake#L17
Same as before