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
Differential D78306
[flang] Use LLVM's flags isuruf on Apr 16 2020, 9:23 AM. Authored by
Details
The only difference is that LLVM_ENABLE_WERROR is set to OFF This commit fixes some windows issues with the flags
Diff Detail
Event Timeline
Comment Actions 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. Comment Actions If Windows, are we able to make this change conditional on Windows? Are we able to fix the warnings instead? Comment Actions
Yes, that's what I had before the last change. Comment Actions Let me know what to do here (I can revert to the commit where Werror is turned on by default to non-MSVC platforms) Comment Actions I reverted back. After this is accepted I will make a new differential to change the default where it can be discussed. Comment Actions It looks like you're still removing -Werror for in-tree builds. Is that correct? That should also not be done without discussion. Comment Actions 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. Comment Actions I'm still happy for this to go ahead without removing Werror, if we open another diff to remove Werror to discuss that separately. I do think the discussion needs having though.
Comment Actions Please make sure LLVM_ENABLE_WERROR, LLVM_ENABLE_PEDANTIC, etc work for out-of-tree builds.
Comment Actions This is ready from my end. Just needs a decision on what to do about werror. My preference is to not enable it by default, but others may disagree. Comment Actions With this change is it possible to enable -Werror for flang (in an in-tree build) without enabling it for all of LLVM? Also, -pedantic? Comment Actions 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. Comment Actions @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. Comment Actions 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. Comment Actions 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. Comment Actions 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 Comment Actions 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. Comment Actions We discussed this on the technical call on 18 May - see https://docs.google.com/document/d/1Z2U5UAtJ-Dag5wlMaLaW1KRmNgENNAYynJqLW2j2AZQ Summarising the discussion:
Comment Actions
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? Comment Actions I've added a FLANG_ENABLE_WERROR which is turned on by default on non MSVC builds. Please test. Comment Actions What is the plan for incompatible host compiler version (clang vs gcc-9 that I mentioned before for example)? Comment Actions 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. Comment Actions @mehdi_amini, after this is merged, I will make a new differential to change the default and people can argue there.
Comment Actions @isuruf I think you're ok to merge this? Are there any objections left as long as we open another separate ticket to discuss -Werror?
|
LLVM_ENABLE_WARNINGS is enabled by default.