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
isuruf on Apr 16 2020, 9:23 AM.Authored by
There are a very large number of changes, so older changes are hidden. Show Older Changes
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.
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.
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.
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:
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.
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.