Page MenuHomePhabricator

[flang] Turn off FLANG_ENABLE_WERROR by default
AcceptedPublic

Authored by isuruf on Jun 11 2020, 3:11 PM.

Details

Summary

This is a follow up to https://reviews.llvm.org/D78306

Diff Detail

Event Timeline

isuruf created this revision.Jun 11 2020, 3:11 PM
Herald added a project: Restricted Project. · View Herald Transcript
isuruf added a project: Restricted Project.Jun 11 2020, 3:29 PM

These change worked in my environment.

DavidTruby accepted this revision.Mon, Jun 15, 6:36 AM

I support this change and have elaborated on why in https://reviews.llvm.org/D78306.
This patch also shows why having Werror on by default can be an issue.

This revision is now accepted and ready to land.Mon, Jun 15, 6:36 AM

@isuruf Are there some warning that cannot be removed with reasonable effort?

@isuruf Are there some warning that cannot be removed with reasonable effort?

I don't think this is about not willing to remove warnings, it is about what is the right default config.

Right for example now our MLIR bot does not show warnings with gcc-5 nor with clang-8, but for example clang-5 still wants extra braces:

warning: suggest braces around initialization of subobject [-Wmissing-braces]
 std::array<StringRef, 2> silentAttrNames{getIndexingMapsAttrName(),
                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~
                                          {

Having default to Werror may lead to broken build for users because we can't test every possible combination all the time.

There is another example here of our BuildBots failing for very spurious warnings due to -Werror:
http://lab.llvm.org:8014/builders/flang-aarch64-ubuntu-clang/builds/229/steps/build-unified-tree/logs/stdio

There is another example here of our BuildBots failing for very spurious warnings due to -Werror:
http://lab.llvm.org:8014/builders/flang-aarch64-ubuntu-clang/builds/229/steps/build-unified-tree/logs/stdio

The warning is not spurious, so this example shows -Werror in the best possible light:
On the positive side, it detected a real error that was immediately fixed and might not have been noticed so soon if it were just a warning.
On the negative side, it prevented the rest of the build from happening and tests from being run, as happens with any compilation error.

mehdi_amini added a comment.EditedTue, Jun 23, 12:45 PM

@isuruf Are there some warning that cannot be removed with reasonable effort?

Since I'm just building with gcc-9 for some reason right now, here is a tough warning:

llvm-project/llvm/include/llvm/ExecutionEngine/Orc/RPC/RPCUtils.h:1534:27: warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move]
 1534 |       return std::move(Err);

Clang could warn if we remove the std::move...

There is another example here of our BuildBots failing for very spurious warnings due to -Werror:
http://lab.llvm.org:8014/builders/flang-aarch64-ubuntu-clang/builds/229/steps/build-unified-tree/logs/stdio

The warning is not spurious, so this example shows -Werror in the best possible light:

Absolutely: this is why we should have bots with Werror enabled (but not with gcc-9 for example)
(This is separate to the default setting though)

Can we move ahead with this change before the llvm 11 branch?