Page MenuHomePhabricator

[flang] Use LLVM's flags
AcceptedPublic

Authored by isuruf on Apr 16 2020, 9:23 AM.

Details

Summary

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
tskeith added inline comments.
flang/CMakeLists.txt
239

-pedantic should still be included in the compilation options.

isuruf marked an inline comment as done.Apr 16 2020, 10:40 AM
isuruf added inline comments.
flang/CMakeLists.txt
239

There's an option LLVM_ENABLE_PEDANTIC which is enabled by default.

ChinouneMehdi added inline comments.
flang/CMakeLists.txt
70

LLVM_ENABLE_WARNINGS is enabled by default.

71

Are you sure this could be enabled with MSVC.
Even LLVM can't be built with it.

flang/unittests/Evaluate/CMakeLists.txt
96

add_flang_executable should be sufficient.

tskeith added inline comments.Apr 16 2020, 11:18 AM
flang/CMakeLists.txt
239

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.

isuruf marked an inline comment as done.Apr 16 2020, 11:23 AM
isuruf added inline comments.
flang/unittests/Evaluate/CMakeLists.txt
96

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)
isuruf updated this revision to Diff 258115.Apr 16 2020, 11:43 AM

Enable pedantic on standalone build and disable werror on msvc

isuruf marked an inline comment as done.Apr 16 2020, 11:44 AM
isuruf added inline comments.
flang/CMakeLists.txt
239

Can you check now?

isuruf updated this revision to Diff 258191.Apr 16 2020, 3:41 PM

Add option for enable exceptions and rtti

tskeith added inline comments.Apr 16 2020, 3:44 PM
flang/CMakeLists.txt
239

Yes, it's there now. So is -Wno-long-long. Is that intended?

isuruf updated this revision to Diff 258205.Apr 16 2020, 4:47 PM
isuruf marked an inline comment as done.

Remove last change

isuruf added inline comments.Apr 16 2020, 9:25 PM
flang/CMakeLists.txt
239

I don't think -Wlong-long is enabled in C++11 or above with -pedantic. So, that's harmless.

From gcc docs,

Warn if long long type is used. This is enabled by either -Wpedantic or -Wtraditional in ISO C90 and C++98 modes. To inhibit the warning messages, use -Wno-long-long.

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.

isuruf updated this revision to Diff 258748.Apr 20 2020, 8:12 AM

Don't enable -Werror by default even on standalone builds

Please give an example of system where this is a problem.

If Windows, are we able to make this change conditional on Windows? Are we able to fix the warnings instead?

If Windows, are we able to make this change conditional on Windows?

Yes, that's what I had before the last change.

Let me know what to do here (I can revert to the commit where Werror is turned on by default to non-MSVC platforms)

isuruf updated this revision to Diff 258803.Apr 20 2020, 11:34 AM

Revert to werror on for standalone 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 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.

It looks like you're still removing -Werror for in-tree builds. Is that correct? That should also not be done without discussion.

That's fair. Let me know what you think should happen here.

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.

DavidTruby accepted this revision.Apr 22 2020, 6:45 AM

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.

This revision is now accepted and ready to land.Apr 22 2020, 6:45 AM
Meinersbur added inline comments.
flang/CMakeLists.txt
70

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.

Please make sure LLVM_ENABLE_WERROR, LLVM_ENABLE_PEDANTIC, etc work for out-of-tree builds.

flang/CMakeLists.txt
70

Flang is built in tree with Clang, so such a change would not escape.

Meinersbur added inline comments.Apr 23 2020, 12:41 AM
flang/CMakeLists.txt
70

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.

@isuruf ping on this revision? I was about to start doing a similar cleanup.

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.

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?

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.)

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?

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.

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?

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.

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

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?

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.

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:

  • We should split the flags fixes (which are agreed) from the flipping of the default for -Werror into separate patches. Suggest this patch be modified to fix the issues seen on Windows without changing the default value of -Werror.
  • 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?
  • 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.

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?

isuruf updated this revision to Diff 264673.Mon, May 18, 10:11 AM

Turn on -Werror by default on non MSVC platforms

isuruf updated this revision to Diff 264676.Mon, May 18, 10:15 AM

Fix MSVC flag

I've added a FLANG_ENABLE_WERROR which is turned on by default on non MSVC builds.

Please test.

isuruf updated this revision to Diff 264679.Mon, May 18, 10:25 AM

Simplify logic

What is the plan for incompatible host compiler version (clang vs gcc-9 that I mentioned before for example)?

@mehdi_amini, I'm sorry, what issue is this?

@mehdi_amini, I'm sorry, what issue is this?

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.

@mehdi_amini, after this is merged, I will make a new differential to change the default and people can argue there.
I'd really like to get this merged and this doesn't change the status quo.

sscalpone added inline comments.Mon, May 18, 10:16 PM
flang/CMakeLists.txt
70

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.

71

How about only default to ON if the user has not already set LLVM_ENABLE_PEDANTIC?

79

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.

239

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.

251

Is there a tradition of documenting a single-letter option like /X?

flang/lib/Optimizer/CMakeLists.txt
1

Why not include this under LLVM_COMPILER_IS_GCC_COMPATIBLE?

flang/unittests/Evaluate/CMakeLists.txt
92

Is rtti really needed here?

flang/unittests/Runtime/CMakeLists.txt
5

Is rtti really needed here?

@mehdi_amini, after this is merged, I will make a new differential to change the default and people can argue there.
I'd really like to get this merged and this doesn't change the status quo.

Yes absolutely! Thanks for fixing this.

isuruf marked 7 inline comments as done.Mon, May 18, 11:02 PM
isuruf added inline comments.
flang/CMakeLists.txt
70

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.

71

Same as before

79

On a in-tree build, this option is already defined. In an out-of-tree build, this option is defined in Line 70

239

-Wall -Wextra -Wcast-qual -Wimplicit-fallthrough -Wdelete-non-virtual-dtor are all covered by LLVM_ENABLE_WARNINGS=ON which is the default.
-fno-rtti is covered by LLVM_ENABLE_RTTI=OFF which is the default.
-fno-exceptions is covered by LLVM_ENABLE_EH=OFF which is the default.

flang/lib/Optimizer/CMakeLists.txt
1

-Wno-unused-parameter is added by LLVM_ENABLE_WARNINGS

flang/unittests/Evaluate/CMakeLists.txt
92
flang/unittests/Runtime/CMakeLists.txt
5

Same as before

@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?

DavidTruby accepted this revision.Wed, Jun 3, 8:23 AM