Do not use -Wno-expansion-to-defined unless GCC actually supports it. It should only be used for GCC 7 and newer.
Created attachment 315397 [details] Patch
Comment on attachment 315397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315397&action=review Informal review: r-, please check the comments. > Source/cmake/OptionsCommon.cmake:74 > + NOT (COMPILER_IS_GNUCXX AND ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 7.0)) Are you sure that only GCC 7+ supports this flag? Do you have any reference (for example to GCC release notes) which justify using 7.0 for comparison? I have just tried the following with a GCC 6: % ./output/host/bin/arm-buildroot-linux-gnueabihf-gcc -dumpversion 6.4.0 % ./output/host/bin/arm-buildroot-linux-gnueabihf-gcc \ -Wno-expansion-to-defined -E -x c++ /dev/null > /dev/null There command does not print anything to stderr, which means it is happily accepting “-Wno-expansion-to-defined”.
It seems like it's safe to pass unknown -Wno-* options to GCC: "When an unrecognized warning option is requested (e.g., -Wunknown-warning), GCC emits a diagnostic stating that the option is not recognized. However, if the -Wno- form is used, the behavior is slightly different: no diagnostic is produced for -Wno-unknown-warning unless other diagnostics are being produced. This allows the use of new -Wno- options with old compilers, but if something goes wrong, the compiler warns that an unrecognized option is present."
(In reply to Konstantin Tokarev from comment #3) > It seems like it's safe to pass unknown -Wno-* options to GCC: Yeah, it doesn't hurt anything, I'm just trying to shut up that warning from the build logs on our bots since it's annoying. E.g. https://bugs.webkit.org/show_bug.cgi?id=174463#c11. (In reply to Adrian Perez from comment #2) > There command does not print anything to stderr, which means it is happily > accepting “-Wno-expansion-to-defined”. You're sure it doesn't print anything to stderr? It does on our bots, otherwise I would never have noticed this. It's indeed new in GCC 7: https://gcc.gnu.org/gcc-7/changes.html
>I'm just trying to shut up that warning from the build logs on our bots since it's annoying. It prints to logs only when there are other warnings in the same file. Did you read the quote above?
(In reply to Konstantin Tokarev from comment #5) > >I'm just trying to shut up that warning from the build logs on our bots since it's annoying. > > It prints to logs only when there are other warnings in the same file. Did > you read the quote above? Ah, no. I did not. Reading is good. :) Anyway, I still don't see why we should not ensure the flag is valid before using it. I'm going to look into using proper compiler flag checks, but I think we should still do this in the meantime.
>I'm going to look into using proper compiler flag checks, but I think we should still do this in the meantime. It's not that hard actually, and I would like it more than this monstrous condition that checks versions of two compilers. Example: https://bugs.webkit.org/attachment.cgi?id=314562&action=prettypatch
(In reply to Konstantin Tokarev from comment #7) > >I'm going to look into using proper compiler flag checks, but I think we should still do this in the meantime. > > It's not that hard actually, and I would like it more than this monstrous > condition that checks versions of two compilers. Example: > https://bugs.webkit.org/attachment.cgi?id=314562&action=prettypatch This looks indeed much better! Provided that GCC will silently ignore the “-Wno-foo” flags, the correct thing to do when we want to use one of those would be to check that “-Wfoo” is supported, and only in that case we would add “-Wno-foo” to the compiler flags.
(In reply to Konstantin Tokarev from comment #7) > >I'm going to look into using proper compiler flag checks, but I think we should still do this in the meantime. > > It's not that hard actually, and I would like it more than this monstrous > condition that checks versions of two compilers. Example: > https://bugs.webkit.org/attachment.cgi?id=314562&action=prettypatch OK. I'll create a new bug for this.
Let's reuse this one. This turned out to be a massive pain. I didn't want to merely check options before using them: I also wanted to organize the code to avoid setting similar flags in different places. Right now we set a bunch of global flags in OptionsCommon.cmake, and a bunch more flags in WEBKIT_SET_EXTRA_COMPILER_FLAGS on a per-target basis. Setting flags per-target seems better in general, e.g. because it makes it very easy to disable warnings for particular ThirdParty targets. But it turns out that all the flags set on a per-target basis get passed to both the C compiler and the C++ compiler, so it's impossible to pass C++-only flags there. That's terrible. It's possible to make the flags language-conditional using generator expressions, but that doesn't work for the Visual Studio backend, so we would have to drop support for that (not going to happen). The CMake documentation suggests that C and C++ files ought to be built in separate targets to avoid this. It's a mess, basically. So I've wound up gutting WEBKIT_SET_EXTRA_COMPILER_FLAGS and adding those flags to CMAKE_C_FLAGS and CMAKE_CXX_FLAGS instead. Really the only disadvantage of this is we now have to suppress individual warnings when building ANGLESupport in WebCore. That's not the end of the world.
Next step is to remove WEBKIT_SET_EXTRA_COMPILER_FLAGS.
Created attachment 315641 [details] Patch
This was pretty hard. This is the best approach I could come up with. Thoughts? I think we can now replace WEBKIT_SET_EXTRA_COMPILER_FLAGS by just turning on position independent code project-wide (I assume all ports probably want that?), but I've saved this for a follow-up.
(This will probably add a ton of warnings for all ports except GTK. They will just need to be suppressed.)
Comment on attachment 315641 [details] Patch Attachment 315641 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4134213 New failing tests: http/tests/css/border-image-loading.html
Created attachment 315647 [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
Konstantin, Alex, are you OK with this approach?
Comment on attachment 315641 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315641&action=review > Source/PlatformGTK.cmake:27 > + COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=${CMAKE_C_FLAGS}\\ -Wno-unused-parameter ${CMAKE_SOURCE_DIR}/Tools/gtk/generate-gtkdoc ${_extra_args} "${CMAKE_C_FLAGS} -Wno-unused-parameter" Escaped scpace is highly confusing, IMO > Source/cmake/WebKitCommon.cmake:41 > + include(WebKitCompilerFlags) Alphabetic order broken > Source/cmake/WebKitCompilerFlags.cmake:118 > + add_definitions(-D{CMAKE_CXX_FLAGS} -mno-ms-bitfields -Wno-unknown-pragmas) This line needs to be removed, it accidentally crept in r218900
Comment on attachment 315641 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315641&action=review > Source/cmake/OptionsCommon.cmake:39 > + WEBKIT_PREPEND_COMPILER_FLAG_IF_SUPPORTED(-mfix-cortex-a53-835769) WEBKIT_PREPEND_GLOBAL_COMPILER_FLAG?
(In reply to Konstantin Tokarev from comment #18) > "${CMAKE_C_FLAGS} -Wno-unused-parameter" > > Escaped scpace is highly confusing, IMO I'll see if that works. I'd like to think I tried that first. > > Source/cmake/WebKitCommon.cmake:41 > > + include(WebKitCompilerFlags) > > Alphabetic order broken OK, I see I can reorder them now. At one point the order was significant because WebKitCompilerFlags needed to use macros from WebKitMacros, but I replaced use of WEBKIT_ADD_TARGET_PROPERTIES with target_compile_options, so that's not needed anymore. > > Source/cmake/WebKitCompilerFlags.cmake:118 > > + add_definitions(-D{CMAKE_CXX_FLAGS} -mno-ms-bitfields -Wno-unknown-pragmas) > > This line needs to be removed, it accidentally crept in r218900 Yay! (In reply to Konstantin Tokarev from comment #19) > > Source/cmake/OptionsCommon.cmake:39 > > + WEBKIT_PREPEND_COMPILER_FLAG_IF_SUPPORTED(-mfix-cortex-a53-835769) > > WEBKIT_PREPEND_GLOBAL_COMPILER_FLAG? Good catch.
Comment on attachment 315641 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315641&action=review > Source/PlatformGTK.cmake:29 > WORKING_DIRECTORY "${CMAKE_BINARY_DIR}" Oh, VERBATIM is missing. That's why you had problem with space.
Comment on attachment 315641 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315641&action=review > Source/cmake/WebKitCompilerFlags.cmake:56 > +endmacro() macro(WEBKIT_APPEPEND_GLOBAL_COMPILER_FLAGS) foreach (_flag ${ARGN}) ... > Source/cmake/WebKitCompilerFlags.cmake:131 > + WEBKIT_PREPEND_GLOBAL_COMPILER_FLAG(-Wwrite-strings) It may be a good idea to change WEBKIT_PREPEND_GLOBAL_COMPILER_FLAG to accept list, or make separate macro, so that it shouldn't be repeated again for every flag
(In reply to Konstantin Tokarev from comment #22) > > Source/cmake/WebKitCompilerFlags.cmake:56 > > +endmacro() > > macro(WEBKIT_APPEPEND_GLOBAL_COMPILER_FLAGS) > foreach (_flag ${ARGN}) > ... Are you saying I should move the code that sets the compiler flags into a macro, and just call it exactly once? I'm not sure there would be much value to doing that? > > Source/cmake/WebKitCompilerFlags.cmake:131 > > + WEBKIT_PREPEND_GLOBAL_COMPILER_FLAG(-Wwrite-strings) > > It may be a good idea to change WEBKIT_PREPEND_GLOBAL_COMPILER_FLAG to > accept list, or make separate macro, so that it shouldn't be repeated again > for every flag I considered doing that, and I'll try that if you tell me to. But I decided I kind of like putting each flag on its own line, since it's easier to read a long list of flags (no horizontal scrolling).
>it's easier to read a long list of flags (no horizontal scrolling). Well, this works too and looks less cluttered: WEBKIT_APPEND_GLOBAL_COMPILER_FLAGS( -Wall -Wextra -Wcast-align ...
Comment on attachment 315641 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315641&action=review >> Source/cmake/WebKitCompilerFlags.cmake:56 >> +endmacro() > > macro(WEBKIT_APPEPEND_GLOBAL_COMPILER_FLAGS) > foreach (_flag ${ARGN}) > ... It was just illustration how WEBKIT_APPEND_GLOBAL_COMPILER_FLAG could be turned into WEBKIT_APPEND_GLOBAL_COMPILER_FLAGS where ... is old body
(In reply to Konstantin Tokarev from comment #24) > >it's easier to read a long list of flags (no horizontal scrolling). > > Well, this works too and looks less cluttered: > > WEBKIT_APPEND_GLOBAL_COMPILER_FLAGS( > -Wall > -Wextra > -Wcast-align > ... OK, I agree.
OK Konstantin, sorry for the delay. Please review take three. I added a macro that takes a list of arguments (not a CMake list ;) for the global flags helpers, but not for the target helpers since it's bad to pass a long list of target-specific flags.
Sorry, I'm too trigger-happy. Let me finish a test build before I upload.
(In reply to Konstantin Tokarev from comment #21) > Comment on attachment 315641 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315641&action=review > > > Source/PlatformGTK.cmake:29 > > WORKING_DIRECTORY "${CMAKE_BINARY_DIR}" > > Oh, VERBATIM is missing. That's why you had problem with space. The double backslash is still the best I could come up with. CMAKE_C_FLAGS is surrounded with double quotes, and the only way to avoid that is to make a new list and append -Wno-unused-parameter to it. Of course that's possible, but it's several extra lines of code relative to just adding a couple of backslashes.
Created attachment 316851 [details] Patch
No cq? because I am thinking I should add an argument list version of the target compiler flags commands as well. Perhaps.
Comment on attachment 316851 [details] Patch Seems I accidentally deleted the new file.
Comment on attachment 316851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316851&action=review > Source/JavaScriptCore/b3/testb3.cpp:8484 > + CHECK(compileAndRun<int>(proc) == static_cast<int>((things.size() * (things.size() - 1)) / 2)); I think the cast should go the other way. Cast the left side to size_t rather than casting the right side to int. Narrowing could hide a difference, while widening will make sure we correctly compare. > Source/PlatformGTK.cmake:27 > + COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=${CMAKE_C_FLAGS}\\ -Wno-unused-parameter ${CMAKE_SOURCE_DIR}/Tools/gtk/generate-gtkdoc ${_extra_args} Why are the two backslashes needed?
Sorry, still need to recover WebKitCompilerFlags.cmake. Pretty critical missing file there. (In reply to Darin Adler from comment #33) > I think the cast should go the other way. Cast the left side to size_t > rather than casting the right side to int. Narrowing could hide a > difference, while widening will make sure we correctly compare. OK! > > Source/PlatformGTK.cmake:27 > > + COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=${CMAKE_C_FLAGS}\\ -Wno-unused-parameter ${CMAKE_SOURCE_DIR}/Tools/gtk/generate-gtkdoc ${_extra_args} > > Why are the two backslashes needed? The first backslash escapes the second, so only one actually gets emitted in the shell command. And that one backslash is needed for the following space. It's not nice, but I had trouble coming up with something better.
Comment on attachment 316851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316851&action=review >> Source/PlatformGTK.cmake:27 >> + COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=${CMAKE_C_FLAGS}\\ -Wno-unused-parameter ${CMAKE_SOURCE_DIR}/Tools/gtk/generate-gtkdoc ${_extra_args} > > Why are the two backslashes needed? Because add_custom_command is used without VERBATIM (it shouldn't be)
I could not find any way to make it work when I added VERBATIM. Perhaps I was missing something obvious.
(In reply to Michael Catanzaro from comment #36) > I could not find any way to make it work when I added VERBATIM. Perhaps I > was missing something obvious. Try this with VERBATIM COMMAND ${CMAKE_COMMAND} -E env "CC=${CMAKE_C_COMPILER} CFLAGS=${CMAKE_C_FLAGS}" ${CMAKE_SOURCE_DIR}/Tools/gtk/generate-gtkdoc ${_extra_args} I'm strongly opposed to use of any custom commands without VERBATIM as it is potentialy non-portable between different CMake generators
(In reply to Konstantin Tokarev from comment #37) > (In reply to Michael Catanzaro from comment #36) > > I could not find any way to make it work when I added VERBATIM. Perhaps I > > was missing something obvious. > > Try this with VERBATIM > > COMMAND ${CMAKE_COMMAND} -E env "CC=${CMAKE_C_COMPILER} > CFLAGS=${CMAKE_C_FLAGS}" ${CMAKE_SOURCE_DIR}/Tools/gtk/generate-gtkdoc > ${_extra_args} > > I'm strongly opposed to use of any custom commands without VERBATIM as it is > potentialy non-portable between different CMake generators Sorry, I've meant COMMAND ${CMAKE_COMMAND} -E env "CC=${CMAKE_C_COMPILER}" "CFLAGS=${CMAKE_C_FLAGS} -Wno-unused-parameter" ${CMAKE_SOURCE_DIR}/Tools/gtk/generate-gtkdoc ${_extra_args}
Comment on attachment 316851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316851&action=review >>> Source/PlatformGTK.cmake:27 >>> + COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=${CMAKE_C_FLAGS}\\ -Wno-unused-parameter ${CMAKE_SOURCE_DIR}/Tools/gtk/generate-gtkdoc ${_extra_args} >> >> Why are the two backslashes needed? > > Because add_custom_command is used without VERBATIM (it shouldn't be) I understand now. Originally I didn’t get that -Wno-unused-parameter is supposed to be part of CFLAGS, which now seem obvious.
(In reply to Konstantin Tokarev from comment #38) > Sorry, I've meant > > COMMAND ${CMAKE_COMMAND} -E env "CC=${CMAKE_C_COMPILER}" > "CFLAGS=${CMAKE_C_FLAGS} -Wno-unused-parameter" > ${CMAKE_SOURCE_DIR}/Tools/gtk/generate-gtkdoc ${_extra_args} Sorry for the delay. That works.
Created attachment 317430 [details] Patch
Created attachment 317431 [details] Patch
Created attachment 317433 [details] Patch
I missed a couple places that need to use the new macros. Need to fix these: Source/WebCore/CMakeLists.txt: WEBKIT_ADD_TARGET_PROPERTIES(WebCore COMPILE_FLAGS "-fno-tree-sra") Source/WebCore/CMakeLists.txt: WEBKIT_ADD_TARGET_PROPERTIES(ANGLESupport COMPILE_FLAGS "-Wno-null-conversion") Tools/DumpRenderTree/TestNetscapePlugIn/CMakeLists.txt:WEBKIT_ADD_TARGET_PROPERTIES(TestNetscapePlugIn COMPILE_FLAGS "-Wno-unused-parameter")
Also need to fix these: [5395/5654] Building CXX object Source...bDriver.dir/glib/SessionHostGlib.cpp.o ../../Source/WebDriver/glib/SessionHostGlib.cpp: In lambda function: ../../Source/WebDriver/glib/SessionHostGlib.cpp:65:25: warning: unused parameter ‘connection’ [-Wunused-parameter] [](GDBusConnection* connection, const gchar* sender, const gchar* objectPath, const gchar* interfaceName, const gchar* methodName, GVariant* parameters, GDBusMethodInvocation* invocation, gpointer userData) { ^~~~~~~~~~ ../../Source/WebDriver/glib/SessionHostGlib.cpp:65:50: warning: unused parameter ‘sender’ [-Wunused-parameter] [](GDBusConnection* connection, const gchar* sender, const gchar* objectPath, const gchar* interfaceName, const gchar* methodName, GVariant* parameters, GDBusMethodInvocation* invocation, gpointer userData) { ^~~~~~ ../../Source/WebDriver/glib/SessionHostGlib.cpp:65:71: warning: unused parameter ‘objectPath’ [-Wunused-parameter] [](GDBusConnection* connection, const gchar* sender, const gchar* objectPath, const gchar* interfaceName, const gchar* methodName, GVariant* parameters, GDBusMethodInvocation* invocation, gpointer userData) { ^~~~~~~~~~ ../../Source/WebDriver/glib/SessionHostGlib.cpp:65:96: warning: unused parameter ‘interfaceName’ [-Wunused-parameter] [](GDBusConnection* connection, const gchar* sender, const gchar* objectPath, const gchar* interfaceName, const gchar* methodName, GVariant* parameters, GDBusMethodInvocation* invocation, gpointer userData) { ^~~~~~~~~~~~~ ../../Source/WebDriver/glib/SessionHostGlib.cpp: At global scope: ../../Source/WebDriver/glib/SessionHostGlib.cpp:97:1: warning: missing initializer for member ‘_GDBusInterfaceVTable::padding’ [-Wmissing-field-initializers] }; ^
*** Bug 175020 has been marked as a duplicate of this bug. ***
Created attachment 317448 [details] Patch
Created attachment 317449 [details] Patch
Konstantin, want to give this r+ again? The only significant change is that I've remove WEBKIT_SET_EXTRA_COMPILER_FLAGS. It's no longer needed if we just set the CMAKE_POSITION_INDEPENDENT_CODE variable.
Comment on attachment 317449 [details] Patch And the iOS EWS failure is unrelated.
I wish it was possible to see diff between patch versions, like in Gerrit
(In reply to Konstantin Tokarev from comment #51) > I wish it was possible to see diff between patch versions, like in Gerrit The only changes are: * Fix warnings in SessionHostGlib.cpp * Remove WEBKIT_SET_EXTRA_COMPILER_FLAGS and every call to it * Set CMAKE_POSITION_INDEPENDENT_CODE in toplevel CMakeLists.txt, except on Windows
Comment on attachment 317449 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317449&action=review > CMakeLists.txt:129 > +if (NOT WIN32) No need to exclude Windows, cmake will handle it correctly > CMakeLists.txt:130 > + set(CMAKE_POSITION_INDEPENDENT_CODE True) This also makes all exectuables to be compiled with -fPIE
Comment on attachment 317449 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317449&action=review > ChangeLog:33 > + build target exclusively contains targets built with just one of those compilers. Yuck. If this is the case, we shouldn't really have separate WEBKIT_ADD_TARGET_C_FLAGS and WEBKIT_ADD_TARGET_CXX_FLAGS macros, but replace them tih WEBKIT_ADD_TARGET_FLAGS that checks option with both compilers
(In reply to Konstantin Tokarev from comment #53) > > CMakeLists.txt:130 > > + set(CMAKE_POSITION_INDEPENDENT_CODE True) > > This also makes all exectuables to be compiled with -fPIE That wasn't intended, but it seems desirable anyway as it's a better default, right?
(In reply to Konstantin Tokarev from comment #54) > If this is the case, we shouldn't really have separate > WEBKIT_ADD_TARGET_C_FLAGS and WEBKIT_ADD_TARGET_CXX_FLAGS macros, but > replace them tih WEBKIT_ADD_TARGET_FLAGS that checks option with both > compilers I wish, but it's not possible unfortunately. Well, for CFLAGS it would be fine, but for CXXFLAGS we often need to pass flags that are not going to pass the C compiler check. Currently in all such cases, the target is composed exclusively of C++ source files, so there is no problem.
(In reply to Michael Catanzaro from comment #55) > (In reply to Konstantin Tokarev from comment #53) > > > CMakeLists.txt:130 > > > + set(CMAKE_POSITION_INDEPENDENT_CODE True) > > > > This also makes all exectuables to be compiled with -fPIE > > That wasn't intended, but it seems desirable anyway as it's a better > default, right? Right, for WK2 processes this may count as hardening measure if ASLR is working. For other kinds of executables (like unit tests) it is not really needed, but is harmless
Committed r220403: <http://trac.webkit.org/changeset/220403>
(In reply to Michael Catanzaro from comment #58) > Committed r220403: <http://trac.webkit.org/changeset/220403> This broke the build with GCC 5.4 (Ubuntu 16.04) https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Ubuntu%20LTS%20%28Build%29/builds/4351/steps/compile-webkit/logs/stdio
What a useful bot! I will try to fix it.
Committed r220425: <http://trac.webkit.org/changeset/220425>