Bug 167643

Summary: Consider enabling -Wno-expansion-to-defined for gcc 7
Product: WebKit Reporter: Tomas Popela <tpopela>
Component: Web Template FrameworkAssignee: Adrian Perez <aperez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, annulen, aperez, bugs-noreply, buildbot, clopez, darin, loic.yhuel, mcatanzaro, thakis, webkit-bug-importer, ysuzuki, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch mcatanzaro: review+

Description Tomas Popela 2017-01-31 07:48:29 PST
In gcc 7 and (and in clang as well https://reviews.llvm.org/D15866 ) there was a new warning added -Wexpansion-to-defined (pasting the description from the clang patch):

//[cpp.cond]p4:
//   Prior to evaluation, macro invocations in the list of preprocessing
//   tokens that will become the controlling constant expression are replaced
//   (except for those macro names modified by the 'defined' unary operator),
//   just as in normal text. If the token 'defined' is generated as a result
//   of this replacement process or use of the 'defined' unary operator does
//   not match one of the two specified forms prior to macro replacement, the
//   behavior is undefined.
// This isn't an idle threat, consider this program:
//   #define FOO
//   #define BAR defined(FOO)
//   #if BAR
//   ...
//   #else
//   ...
//   #endif
// clang and gcc will pick the #if branch while Visual Studio will take the
// #else branch.  Emit a warning about this undefined behavior.

This applies for all of our USE, PLATFORM, CPU, ENABLE, ... macros. I would suggest to add -Wno-expansion-to-defined to our CFLAGS in OptionsCommon.cmake to get rid off the warning as it's more easier than rewriting the macro definitions. Also I'm not sure if the warning will be triggered on newer clang as per:

"there is no easy way to rewrite a function-like macro like #define FOO(x) (defined __foo_##x && __foo_##x). Function-like macros like this are used in practice, and compilers seem to not have differing behavior in that case. So make this a default-on warning only for object-like macros and an extension warning that only shows up with pedantic for function-like macros. (But it's undefined behavior in both cases.)"

that we are using in the USE, PLATFORM, CPU, ENABLE, ... macros. So I'm curious if it's actually not a gcc bug.
Comment 1 Michael Catanzaro 2017-05-09 06:11:43 PDT
So we have to decide whether or not we care that our USE, PLATFORM, CPU, and ENABLE macros trigger undefined behavior. This is probably the most pervasive reliance on undefined behavior I've ever seen. On the other hand, we know it works fine on all major platforms, and I don't see any way to fix this. So I'm fine with adding -Wno-expansion-to-defined, but I'd like more opinions.

Anyway, it looks like you came up with this patch, we've been carrying this in Fedora for several months to get WebKit to build with GCC 7:

diff -up webkitgtk-2.15.90/Source/cmake/OptionsCommon.cmake.gcc7 webkitgtk-2.15.90/Source/cmake/OptionsCommon.cmake
--- webkitgtk-2.15.90/Source/cmake/OptionsCommon.cmake.gcc7	2017-02-21 09:57:13.168916004 +0100
+++ webkitgtk-2.15.90/Source/cmake/OptionsCommon.cmake	2017-02-21 09:58:12.811563156 +0100
@@ -41,6 +41,8 @@ if (COMPILER_IS_GCC_OR_CLANG)
     set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fno-exceptions -fno-strict-aliasing")
     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-exceptions -fno-strict-aliasing -fno-rtti")
     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++1y")
+    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-expansion-to-defined")
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-expansion-to-defined")
 endif ()
 
 if (COMPILER_IS_CLANG AND CMAKE_GENERATOR STREQUAL "Ninja")
Comment 2 Michael Catanzaro 2017-05-25 19:57:32 PDT
*** Bug 172618 has been marked as a duplicate of this bug. ***
Comment 3 Adrian Perez 2017-05-26 02:01:57 PDT
Created attachment 311344 [details]
Patch
Comment 4 Build Bot 2017-05-26 03:25:19 PDT
Comment on attachment 311344 [details]
Patch

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

New failing tests:
fast/events/before-unload-returnValue.html
fast/css/target-fragment-match.html
Comment 5 Build Bot 2017-05-26 03:25:22 PDT
Created attachment 311347 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 6 Zan Dobersek 2017-05-31 04:57:55 PDT
Comment on attachment 311344 [details]
Patch

I don't think the GTK+ EWS failure is due to this patch.
Comment 7 Adrian Perez 2017-05-31 07:08:16 PDT
(In reply to Zan Dobersek from comment #6)
> Comment on attachment 311344 [details]
> Patch
> 
> I don't think the GTK+ EWS failure is due to this patch.

Yep, I have the same impression here. Let's land this :)
Comment 8 Adrian Perez 2017-05-31 07:09:09 PDT
Committed r217601: <http://trac.webkit.org/changeset/217601>
Comment 9 Carlos Alberto Lopez Perez 2017-05-31 07:14:24 PDT
(In reply to Adrian Perez from comment #8)
> Committed r217601: <http://trac.webkit.org/changeset/217601>

This has made my build painful noisy:

I get thousands of warnings printed like this:
warning: unknown warning option '-Wno-expansion-to-defined'; did you mean '-Wno-macro-redefined'? [-Wunknown-warning-option]

Using clang-3.8 with ccache.
Comment 10 Adrian Perez 2017-05-31 12:18:49 PDT
(In reply to Carlos Alberto Lopez Perez from comment #9)
> (In reply to Adrian Perez from comment #8)
> > Committed r217601: <http://trac.webkit.org/changeset/217601>
> 
> This has made my build painful noisy:
> 
> I get thousands of warnings printed like this:
> warning: unknown warning option '-Wno-expansion-to-defined'; did you mean
> '-Wno-macro-redefined'? [-Wunknown-warning-option]
> 
> Using clang-3.8 with ccache.

Ouch, that is as inconvenient as getting multiple warnings due to
“-Wexpansion-to-defined”. I have filed a bug #172750 to solve the
new issue with Clang, and posted a patch. Let's continue the talk
there :)
Comment 11 Loïc Yhuel 2017-07-14 16:48:39 PDT
-Wno-expansion-to-defined is only supported in gcc >= 7, so older versions would print a warning as soon as there is another warning in the file ("no diagnostic is produced for -Wno-unknown-warning unless other diagnostics are being produced"):
cc1plus: warning: unrecognized command line option ‘-Wno-expansion-to-defined’
Comment 12 Adrian Perez 2017-07-15 03:43:18 PDT
(In reply to Loïc Yhuel from comment #11)
> -Wno-expansion-to-defined is only supported in gcc >= 7, so older versions
> would print a warning as soon as there is another warning in the file ("no
> diagnostic is produced for -Wno-unknown-warning unless other diagnostics are
> being produced"):
> cc1plus: warning: unrecognized command line option
> ‘-Wno-expansion-to-defined’

There is some discussion about this in bug #174490

In short: We are aware of this issue, and we will be adding logic in the
CMake build files to detect which flags the compiler supports, and use
only the supported ones instead of relying on comparisons against the
version of the compiler.
Comment 13 Nico Weber 2017-09-15 11:58:35 PDT
Hi, I implemented -Wexpansion-to-defined in clang, and I used to WebKit a while back.

It sounds like gcc ported the warning over from clang, which is great, but made it also warn on function-like macros, which makes it useless. I designed the warning in clang to actually be useful in practice, see the commit message of http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160118/147239.html (the warning flag was renamed from -Wexpansion-to-undefined to -Wexpansion-to-defined very shortly after that commit). WebKit's macros where one of the things I actually looked at while designing this warning.

I think by disabling the warning for clang too, you're doing the project a disservice, since for object-style macros the clang warning does flag instances where MSVC and gcc _actually produce different output_.

So I'd suggest:
1. Only disable the warning for gcc, not clang, and fix instances that clang flags. These are real bugs, and they're easy to fix (see above-linked commit message).

2. File a bug on gcc to make their warning have useful behavior.
Comment 14 Nico Weber 2017-09-15 11:58:50 PDT
*used to _work_ on WebKit...
Comment 15 Konstantin Tokarev 2017-09-15 12:48:09 PDT
Thanks for explanation, Nico! I'll make a patch.

"To WebKit or not to WebKit, that is the question" :)
Comment 16 Konstantin Tokarev 2017-09-15 12:54:59 PDT
Reopening to attach new patch.
Comment 17 Konstantin Tokarev 2017-09-15 12:55:01 PDT
Created attachment 320949 [details]
Patch
Comment 18 Adrian Perez 2017-09-15 13:14:11 PDT
Comment on attachment 320949 [details]
Patch

Informal review: r=me :-)
Comment 19 Michael Catanzaro 2017-09-15 13:36:02 PDT
Comment on attachment 320949 [details]
Patch

If you don't add a comment, then somebody's going to come along and move it right back to where it used to be. Might be me if I forget about this two years from now!
Comment 20 Konstantin Tokarev 2017-09-15 14:23:13 PDT
Committed r222112: <http://trac.webkit.org/changeset/222112>
Comment 21 Konstantin Tokarev 2017-09-15 14:36:25 PDT
Is anyone here registered in GCC's bugzilla?
Comment 22 Radar WebKit Bug Importer 2017-09-27 12:53:52 PDT
<rdar://problem/34694234>