Summary: | Consider enabling -Wno-expansion-to-defined for gcc 7 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tomas Popela <tpopela> | ||||||||
Component: | Web Template Framework | Assignee: | 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
Tomas Popela
2017-01-31 07:48:29 PST
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") *** Bug 172618 has been marked as a duplicate of this bug. *** Created attachment 311344 [details]
Patch
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 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 on attachment 311344 [details]
Patch
I don't think the GTK+ EWS failure is due to this patch.
(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 :) Committed r217601: <http://trac.webkit.org/changeset/217601> (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. (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 :) -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’ (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. 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. *used to _work_ on WebKit... Thanks for explanation, Nico! I'll make a patch. "To WebKit or not to WebKit, that is the question" :) Reopening to attach new patch. Created attachment 320949 [details]
Patch
Comment on attachment 320949 [details]
Patch
Informal review: r=me :-)
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!
Committed r222112: <http://trac.webkit.org/changeset/222112> Is anyone here registered in GCC's bugzilla? |