Bug 174490

Summary: [CMake] Properly test if compiler supports compiler flags
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, annulen, aperez, bugs-noreply, buildbot, clopez, darin, mcatanzaro, tpopela
Priority: P2    
Version: Other   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 175020    
Attachments:
Description Flags
Patch
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch annulen: review+, mcatanzaro: commit-queue-

Description Michael Catanzaro 2017-07-13 21:07:54 PDT
Do not use -Wno-expansion-to-defined unless GCC actually supports it. It should only be used for GCC 7 and newer.
Comment 1 Michael Catanzaro 2017-07-13 21:08:57 PDT
Created attachment 315397 [details]
Patch
Comment 2 Adrian Perez 2017-07-14 06:22:34 PDT
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”.
Comment 3 Konstantin Tokarev 2017-07-14 06:31:43 PDT
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."
Comment 4 Michael Catanzaro 2017-07-14 08:33:28 PDT
(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
Comment 5 Konstantin Tokarev 2017-07-14 08:44:23 PDT
>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?
Comment 6 Michael Catanzaro 2017-07-14 11:18:57 PDT
(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.
Comment 7 Konstantin Tokarev 2017-07-14 11:25:13 PDT
>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
Comment 8 Adrian Perez 2017-07-14 11:58:05 PDT
(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.
Comment 9 Michael Catanzaro 2017-07-14 12:02:06 PDT
(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.
Comment 10 Michael Catanzaro 2017-07-15 19:20:03 PDT
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.
Comment 11 Michael Catanzaro 2017-07-15 20:36:09 PDT
Next step is to remove WEBKIT_SET_EXTRA_COMPILER_FLAGS.
Comment 12 Michael Catanzaro 2017-07-16 23:25:15 PDT
Created attachment 315641 [details]
Patch
Comment 13 Michael Catanzaro 2017-07-16 23:26:59 PDT
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.
Comment 14 Michael Catanzaro 2017-07-16 23:33:13 PDT
(This will probably add a ton of warnings for all ports except GTK. They will just need to be suppressed.)
Comment 15 Build Bot 2017-07-17 01:13:53 PDT
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
Comment 16 Build Bot 2017-07-17 01:13:55 PDT
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
Comment 17 Michael Catanzaro 2017-07-19 11:35:33 PDT
Konstantin, Alex, are you OK with this approach?
Comment 18 Konstantin Tokarev 2017-07-19 11:45:11 PDT
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 19 Konstantin Tokarev 2017-07-19 11:53:12 PDT
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?
Comment 20 Michael Catanzaro 2017-07-19 11:58:39 PDT
(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 21 Konstantin Tokarev 2017-07-19 12:02:23 PDT
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 22 Konstantin Tokarev 2017-07-19 12:16:01 PDT
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
Comment 23 Michael Catanzaro 2017-07-19 12:29:47 PDT
(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).
Comment 24 Konstantin Tokarev 2017-07-19 12:38:36 PDT
>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 25 Konstantin Tokarev 2017-07-19 12:39:50 PDT
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
Comment 26 Michael Catanzaro 2017-07-19 12:40:07 PDT
(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.
Comment 27 Michael Catanzaro 2017-08-01 03:33:35 PDT
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.
Comment 28 Michael Catanzaro 2017-08-01 03:38:06 PDT
Sorry, I'm too trigger-happy. Let me finish a test build before I upload.
Comment 29 Michael Catanzaro 2017-08-01 06:55:16 PDT
(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.
Comment 30 Michael Catanzaro 2017-08-01 07:01:10 PDT
Created attachment 316851 [details]
Patch
Comment 31 Michael Catanzaro 2017-08-01 07:01:59 PDT
No cq? because I am thinking I should add an argument list version of the target compiler flags commands as well. Perhaps.
Comment 32 Michael Catanzaro 2017-08-01 07:08:37 PDT
Comment on attachment 316851 [details]
Patch

Seems I accidentally deleted the new file.
Comment 33 Darin Adler 2017-08-01 08:59:12 PDT
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?
Comment 34 Michael Catanzaro 2017-08-01 15:38:12 PDT
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 35 Konstantin Tokarev 2017-08-01 16:31:48 PDT
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)
Comment 36 Michael Catanzaro 2017-08-02 01:08:12 PDT
I could not find any way to make it work when I added VERBATIM. Perhaps I was missing something obvious.
Comment 37 Konstantin Tokarev 2017-08-02 02:08:34 PDT
(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
Comment 38 Konstantin Tokarev 2017-08-02 02:58:42 PDT
(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 39 Darin Adler 2017-08-02 08:23:19 PDT
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.
Comment 40 Michael Catanzaro 2017-08-07 10:08:06 PDT
(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.
Comment 41 Michael Catanzaro 2017-08-07 10:23:21 PDT
Created attachment 317430 [details]
Patch
Comment 42 Michael Catanzaro 2017-08-07 10:24:01 PDT
Created attachment 317431 [details]
Patch
Comment 43 Michael Catanzaro 2017-08-07 10:27:49 PDT
Created attachment 317433 [details]
Patch
Comment 44 Michael Catanzaro 2017-08-07 10:38:50 PDT
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")
Comment 45 Michael Catanzaro 2017-08-07 11:47:16 PDT
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]
 };
 ^
Comment 46 Michael Catanzaro 2017-08-07 12:05:50 PDT
*** Bug 175020 has been marked as a duplicate of this bug. ***
Comment 47 Michael Catanzaro 2017-08-07 12:16:58 PDT
Created attachment 317448 [details]
Patch
Comment 48 Michael Catanzaro 2017-08-07 12:22:34 PDT
Created attachment 317449 [details]
Patch
Comment 49 Michael Catanzaro 2017-08-07 13:04:34 PDT
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 50 Michael Catanzaro 2017-08-07 13:04:54 PDT
Comment on attachment 317449 [details]
Patch

And the iOS EWS failure is unrelated.
Comment 51 Konstantin Tokarev 2017-08-08 03:02:42 PDT
I wish it was possible to see diff between patch versions, like in Gerrit
Comment 52 Michael Catanzaro 2017-08-08 05:36:13 PDT
(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 53 Konstantin Tokarev 2017-08-08 07:02:08 PDT
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 54 Konstantin Tokarev 2017-08-08 07:13:07 PDT
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
Comment 55 Michael Catanzaro 2017-08-08 07:19:09 PDT
(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?
Comment 56 Michael Catanzaro 2017-08-08 07:20:26 PDT
(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.
Comment 57 Konstantin Tokarev 2017-08-08 07:26:30 PDT
(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
Comment 58 Michael Catanzaro 2017-08-08 08:03:55 PDT
Committed r220403: <http://trac.webkit.org/changeset/220403>
Comment 59 Carlos Alberto Lopez Perez 2017-08-08 13:34:19 PDT
(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
Comment 60 Michael Catanzaro 2017-08-08 14:43:59 PDT
What a useful bot! I will try to fix it.
Comment 61 Michael Catanzaro 2017-08-08 16:03:25 PDT
Committed r220425: <http://trac.webkit.org/changeset/220425>