Bug 155047 – [CMake] Add option to compile with warnings as errors

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Michael Catanzaro]

We should default ENABLE_WERROR to the value of DEVELOPER_MODE is set, to force developers to fix warnings rather than just ignoring them. This should be changed by editing WEBKIT_SET_EXTRA_COMPILER_FLAGS. Currently its default value is that of IGNORECXX_WARNINGS, which doesn't make much sense to me.

Before making this change, we need to:

 * Fix current warnings affecting the compilers we care about (at the very least: Clang 3.7, GCC 5.x, and GCC 6.x).
 * Change the bots to pass -DENABLE_WERROR=OFF, because we *really* don't want bots to break unnecessarily.

[Konstantin Tokarev]

I would rather select a small set of warnings we want to be fatal, and apply -Werror only for them, e.g. -Werror=return-type -Werror=non-virtual-dtor. Later we could extend this list gradually.

[Michael Catanzaro]

(In reply to comment #1)
> I would rather select a small set of warnings we want to be fatal, and apply
> -Werror only for them, e.g. -Werror=return-type -Werror=non-virtual-dtor.
> Later we could extend this list gradually.

TBH I don't see value in this, I would like to enforce either fixing or disabling all warnings. (For developers performing development builds.)

We have very few warnings right now, so this shouldn't be any problem.

[Konstantin Tokarev]

(In reply to comment #2)
> (In reply to comment #1)
> > I would rather select a small set of warnings we want to be fatal, and apply
> > -Werror only for them, e.g. -Werror=return-type -Werror=non-virtual-dtor.
> > Later we could extend this list gradually.
> 
> TBH I don't see value in this, I would like to enforce either fixing or
> disabling all warnings. (For developers performing development builds.)
> 
> We have very few warnings right now, so this shouldn't be any problem.

New version of gcc or clang may introduce new warning by default, resulting in compilation errors in case -Werror is used (e.g., -Winconsistent-missing-override become enabled by -Wall in clang 3.5)

[Michael Catanzaro]

Which is why we should only enable it in developer mode, so developers are forced to fix or silence the new warnings to keep the build clean (unless they pass -DENABLE_WERROR=OFF), but users aren't affected either way.

New warnings will also be introduced by folks using a different compiler than you.

[Konstantin Tokarev]

Another source of warnings is unused arguments in case function contains #if guards. Silencing that would require addition of lots of source lines

[Michael Catanzaro]

(In reply to comment #5)
> Another source of warnings is unused arguments in case function contains #if
> guards. Silencing that would require addition of lots of source lines

You shouldn't see any of these, that's what the UNUSED_PARAM macro is for! Yes, it is often annoying to silence these, but we made a project decision not to disable the warning, though there's a good argument to be made for disabling that warning. (I'm currently planning to do so for DerivedSources only, as that's currently the only place we see any of these, at least on the GTK port.)

But it's irrelevant to whether we should turn warnings into errors, as we don't want preexisting warnings preventing developers from detecting new warnings. Enforcing -Werror in development builds is the best way to ensure the build stays clean and shiny. If you don't want to fix them, you can just use -DENABLE_WERROR=OFF.

[Michael Catanzaro]

Let's enable it even on the bots for now, to match what EFL has been doing for years without causing too much trouble. But we should keep an eye on this and disable it on the bots if it becomes an issue.

[Michael Catanzaro]

Created attachment 278993 [details]
Patch

[Michael Catanzaro]

(In reply to comment #0)
> We should default ENABLE_WERROR to the value of DEVELOPER_MODE is set, to
> force developers to fix warnings rather than just ignoring them. This should
> be changed by editing WEBKIT_SET_EXTRA_COMPILER_FLAGS. Currently its default
> value is that of IGNORECXX_WARNINGS, which doesn't make much sense to me.

FYI I misunderstood how this works; it's not creating a CMake option inside the macro, rather it's defining a list of optional parameters. So ENABLE_WERROR and IGNORECXX_WARNINGS are different optional parameters to WEBKIT_SET_EXTRA_COMPILER_FLAGS. IGNORECXX_WARNINGS is not the default value of ENABLE_WERROR.

[Csaba Osztrogonác]

(In reply to comment #7)
> Let's enable it even on the bots for now, to match what EFL has been doing
> for years without causing too much trouble. But we should keep an eye on
> this and disable it on the bots if it becomes an issue.

I suggest disabling werror on your ARM bot, because 
there are zillion warnings on ARM long time ago.

clean build on ARM GTK bot warnings:
https://build.webkit.org/builders/GTK%20Linux%20ARM%20Release/builds/11162

Today I submitted many patches fix a dozen ARM warnings,
but there are many cast-align warnings remaining. Maybe we 
should use Wno-error=cast-align until somebody can fix them.

[Michael Catanzaro]

I'd still like to do this, and for WPE as well. Developers must not ignore compiler warnings, they must be fixed or suppressed.

[Michael Catanzaro]

Let's start with GTK only for now.

[Michael Catanzaro]

Created attachment 315365 [details]
Patch

[Michael Catanzaro]

It's not perfect as it does not apply to ThirdParty libs which don't use this macro, but I think that's fine for now.

[Michael Catanzaro]

(In reply to Csaba Osztrogonác from comment #10)
> I suggest disabling werror on your ARM bot, because 
> there are zillion warnings on ARM long time ago.

We should add -Wno-error to its CMAKE_C_FLAGS and CMAKE_CXX_FLAGS.

[Build Bot]

Comment on attachment 315365 [details]
Patch

Attachment 315365 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4114987

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html

[Build Bot]

Created attachment 315371 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Michael Catanzaro]

Note the EWS failure is fixed in bug #174463.

[Carlos Garcia Campos]

I don't want this. Sometimes just upgrading GCC makes new warnings to appear, or even in other cases new code using new C++ features introduces warnings in some compilers, that it could even be a compiler bug. Also harmless warnings like unused parameters that depend on an #ifdef and someone forgot to use UNUSED_PARAM. Let's not make so easy to break our build, please. I don't want the bots to be red for the whole (European) night because of a harmless warning.

[Michael Catanzaro]

We could add -Wno-error to CMAKE_C_FLAGS and CMAKE_CXX_FLAGS on all our bots. Would you be OK with it then? That will override because it will come later on the command line.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #20)
> We could add -Wno-error to CMAKE_C_FLAGS and CMAKE_CXX_FLAGS on all our
> bots. Would you be OK with it then? That will override because it will come
> later on the command line.

I'll do the same in my build script, then.

[Michael Catanzaro]

So, four years later! I want -Werror on EWS at least to avoid excessive newly-introduced warnings. I proposed -Werror for EWS in https://lists.webkit.org/pipermail/webkit-dev/2021-May/031867.html with no objections. But for sure we do not want -Werror in use on the buildbots, because that will inevitably cause us to lose test results due to silly warnings. So goal is to enable -Werror on EWS, but not on buildbots.

We could implement it in two ways:

 (a) Add -Werror specially to the EWS config and nowhere else
 (b) Add -Werror when using ENABLE_DEVELOPER_MODE, and add -Wno-error to our buildbot configs to override it there

(b) is this bug, and I prefer (b). WebKit developers should fix warnings, not ignore them. Understanding that you might choose to use -Wno-error locally, are you OK with that, Carlos?

[Michael Catanzaro]

Reopening.

[Michael Catanzaro]

Created attachment 430499 [details]
Patch

[Michael Catanzaro]

Comment on attachment 430499 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430499&action=review

> Source/cmake/WebKitCompilerFlags.cmake:326
> +            # Since -Werror will cause the checks to fail, we've got to prepend it
> +            # it manually, without checking it. Also note we intentionally add
> +            # this at the bottom of this file, after testing all the other flags,
> +            # since it may break any subsequent compiler tests.

So here's the problem: this doesn't actually work. WebKitCompilerFlags.cmake is not the last place we add compiler flags. We do it in every WebKit framework project as well. It breaks things like this:

WEBKIT_ADD_TARGET_CXX_FLAGS(gtest -Wno-undef
                                                                             -Wno-stringop-truncation
                                                                            -Wno-suggest-attribute=format)

That one's from Source/ThirdParty/gtest/CMakeLists.txt, but we have similar code in most of our CMakeLists.txt.

After appending -Werror, basically all CMake compiler flags tests start failing. In CMakeError.log the failure looks like this:

Source file was:
int main() { return 0; }
Performing C++ SOURCE FILE Test CXX_COMPILER_SUPPORTS_-Wno-undef failed with the following output:
Change Dir: /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/CMakeFiles/CMakeTmp

Run Build Command(s):/usr/bin/ninja-build cmTC_8fffa && [1/2] Building CXX object CMakeFiles/cmTC_8fffa.dir/src.cxx.o
FAILED: CMakeFiles/cmTC_8fffa.dir/src.cxx.o 
/usr/lib64/ccache/c++   -Werror -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-noexcept-type -Wno-psabi -Wno-misleading-indentation -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wno-tautological-compare -g -O0 -fno-strict-aliasing -fno-exceptions -fno-rtti  -fPIE   -DCXX_COMPILER_SUPPORTS_-Wno-undef -Wno-undef -std=c++17 -o CMakeFiles/cmTC_8fffa.dir/src.cxx.o -c src.cxx
<command-line>: error: ISO C++11 requires whitespace after the macro name [-Werror]
cc1plus: all warnings being treated as errors
ninja: build stopped: subcommand failed.

To make this work we're going to have to figure out how to find the source code that cmake_check_cxx_compiler_flag() is trying to build and figure out how to make it not trip this macro whitespace problem. Then hope that CMake's test never trips any other warnings at any point in the future. That might very well be too much to expect....

CMake should probably use -Wno-error for its compiler flags tests, but that might be controversial.

[Michael Catanzaro]

OK I don't think we can do this using CMAKE_CXX_FLAGS since it interferes with cmake_check_cxx_compiler_flag(). We'll have to add it in separately when building targets....

[Michael Catanzaro]

Created attachment 453403 [details]
Patch

[Michael Catanzaro]

Note that EWS will fail because (1) patch in bug #237275 has not landed yet, and (2) I haven't tested the same build configuration used by the EWS bots, so they're very likely to have unfixed warnings.

[Michael Catanzaro]

Created attachment 453746 [details]
Patch

[Don Olmstead]

My main concern here is that each clang update always brings with it a set of new warnings so whenever our SDK updates with a new clang release our builds will definitely go red.

[Basuke Suzuki]

(In reply to Don Olmstead from comment #30)
> My main concern here is that each clang update always brings with it a set
> of new warnings so whenever our SDK updates with a new clang release our
> builds will definitely go red.

For bots, we shouldn't enable developer mode.

[Don Olmstead]

(In reply to Basuke Suzuki from comment #31)
> (In reply to Don Olmstead from comment #30)
> > My main concern here is that each clang update always brings with it a set
> > of new warnings so whenever our SDK updates with a new clang release our
> > builds will definitely go red.
> 
> For bots, we shouldn't enable developer mode.

By default build-webkit uses developer mode.

https://github.com/WebKit/WebKit/blob/main/Tools/Scripts/webkitdirs.pm#L2555-L2556

PlayStation port isn't listed there but it should be.

[Michael Catanzaro]

(In reply to Don Olmstead from comment #30)
> My main concern here is that each clang update always brings with it a set
> of new warnings so whenever our SDK updates with a new clang release our
> builds will definitely go red.

Yup, that's the goal. Fixing new warnings is an expected part of updating to a newer compiler.

(In reply to Basuke Suzuki from comment #31)
> For bots, we shouldn't enable developer mode.

Although all bots necessarily use developer mode, the non-EWS bots will use --no-fatal-warnings.

[Don Olmstead]

(In reply to Michael Catanzaro from comment #33)
> (In reply to Don Olmstead from comment #30)
> > My main concern here is that each clang update always brings with it a set
> > of new warnings so whenever our SDK updates with a new clang release our
> > builds will definitely go red.
> 
> Yup, that's the goal. Fixing new warnings is an expected part of updating to
> a newer compiler.
> 
> (In reply to Basuke Suzuki from comment #31)
> > For bots, we shouldn't enable developer mode.
> 
> Although all bots necessarily use developer mode, the non-EWS bots will use
> --no-fatal-warnings.

That seems reasonable to me. Carry on.

[Don Olmstead]

Pull request: https://github.com/WebKit/WebKit/pull/4437

[EWS]

Committed 254583@main (1460c1507d3b): <https://commits.webkit.org/254583@main>

Reviewed commits have been landed. Closing PR #4437 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/100058793>