Bug 144557

Summary: [GTK] OSX linker doesn't understand --whole-archive
Product: WebKit Reporter: Philip Chimento <philip.chimento>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: cgarcia, commit-queue, jeremyhu, mrobinson, pnormand
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: OS X 10.10   
Bug Depends on:    
Bug Blocks: 126492    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Philip Chimento 2015-05-03 16:50:49 PDT
On OSX, the linker doesn't understand --whole-archive. This causes the build to fail due to ADD_WHOLE_ARCHIVE_TO_LIBRARIES(WebKit2_LIBRARIES) in PlatformGTK.cmake. The corresponding option for the OSX linker is -all_load and -noall_load.
Comment 1 Philip Chimento 2015-05-07 23:15:23 PDT
However, using -all_load or -force_load makes MiniBrowser hit an assertion failure on startup:

ASSERTION FAILED: m_key != PTHREAD_KEYS_MAX
/Users/fliep/gtk/source/webkitgtk-2.8.0/Source/WTF/wtf/ThreadIdentifierDataPthreads.cpp(65) : static ThreadIdentifier WTF::ThreadIdentifierData::identifier()
1   0x11509d1d0
2   0x1150fb11d
3   0x1150fc8cd
4   0x1150a8a06
5   0x112c8ab9e
6   0x112c3fc6e
7   0x112e53ecf
8   0x112e4c558
9   0x112e4c1fd
10  0x112e4c190
11  0x1131e38db
12  0x1131e3865
13  0x1131d97d5
14  0x11a29b452
15  0x11a27fa55
16  0x11a27edce
17  0x11a27ea08
18  0x1131d981b
19  0x10dcc5a03
20  0x10dcbc694
Segmentation fault: 11

Simply leaving off the option seems to work fine. I'll upload a patch to that effect.
Comment 2 Philip Chimento 2015-05-07 23:20:04 PDT
Created attachment 252688 [details]
Patch
Comment 3 Philippe Normand 2015-10-12 09:12:08 PDT
I'm not sure about this patch. What do you think Martin, Carlos?
Comment 4 Martin Robinson 2015-10-12 09:21:06 PDT
Comment on attachment 252688 [details]
Patch

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

Seems sensible to me, but this patch needs a bit of work. Thanks!

> Source/WebKit2/PlatformGTK.cmake:549
> -ADD_WHOLE_ARCHIVE_TO_LIBRARIES(WebKit2_LIBRARIES)
> +if (CMAKE_SYSTEM_NAME MATCHES "Linux")
> +    ADD_WHOLE_ARCHIVE_TO_LIBRARIES(WebKit2_LIBRARIES)
> +endif ()

It would be better to add this check to the ADD_WHOLE_ARCHIVE_TO_LIBRARIES macro instead of requiring it for every invocation.

> Source/WebKit2/PlatformGTK.cmake:875
> -    COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=-Wno-deprecated-declarations LDFLAGS=
> +    COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=-Wno-deprecated-declarations LDFLAGS=-lGObjectDOMBindings

This looks unrelated.

> Source/WebKit2/PlatformGTK.cmake:890
> +        --library=c++

Ditto.
Comment 5 Philip Chimento 2015-10-12 12:48:22 PDT
(In reply to comment #4)
> Comment on attachment 252688 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252688&action=review
> 
> Seems sensible to me, but this patch needs a bit of work. Thanks!
> 
> > Source/WebKit2/PlatformGTK.cmake:549
> > -ADD_WHOLE_ARCHIVE_TO_LIBRARIES(WebKit2_LIBRARIES)
> > +if (CMAKE_SYSTEM_NAME MATCHES "Linux")
> > +    ADD_WHOLE_ARCHIVE_TO_LIBRARIES(WebKit2_LIBRARIES)
> > +endif ()
> 
> It would be better to add this check to the ADD_WHOLE_ARCHIVE_TO_LIBRARIES
> macro instead of requiring it for every invocation.

Sure. I'll also change it to NOT CMAKE_SYSTEM_MATCHES "Darwin" in order to be more specific like I did in #144555.

> > Source/WebKit2/PlatformGTK.cmake:875
> > -    COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=-Wno-deprecated-declarations LDFLAGS=
> > +    COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=-Wno-deprecated-declarations LDFLAGS=-lGObjectDOMBindings
> 
> This looks unrelated.

These are related though; it's what I was referring to with "link with extra libraries instead" in the patch's changelog. Without --whole-archive, these libraries don't get picked up for linking, so I have to add them manually.

I wonder if it might make sense to get rid of the ADD_WHOLE_ARCHIVE_TO_LIBRARIES macro entirely, since all the necessary libraries are now added manually. Alternatively, I could bracket these manual additions inside CMAKE_SYSTEM_NAME checks.
Comment 6 Martin Robinson 2015-10-12 13:16:04 PDT
Comment on attachment 252688 [details]
Patch

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

>>> Source/WebKit2/PlatformGTK.cmake:875
>>> +    COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=-Wno-deprecated-declarations LDFLAGS=-lGObjectDOMBindings
>> 
>> This looks unrelated.
> 
> These are related though; it's what I was referring to with "link with extra libraries instead" in the patch's changelog. Without --whole-archive, these libraries don't get picked up for linking, so I have to add them manually.
> 
> I wonder if it might make sense to get rid of the ADD_WHOLE_ARCHIVE_TO_LIBRARIES macro entirely, since all the necessary libraries are now added manually. Alternatively, I could bracket these manual additions inside CMAKE_SYSTEM_NAME checks.

If you can get WebKitGTK+ to compile properly on Linux and on the build bots without ADD_WHOLE_ARCHIVE_TO_LIBRARIES, feel free to remove it.

>> Source/WebKit2/PlatformGTK.cmake:890
>> +        --library=c++
> 
> Ditto.

Hrm. Is c++ a system library or a special value understood by g-ir-scanner? If it's the latter, is it documented somewhere?
Comment 7 Philip Chimento 2015-10-12 15:14:34 PDT
(In reply to comment #6)
> >> Source/WebKit2/PlatformGTK.cmake:890
> >> +        --library=c++
> > 
> > Ditto.
> 
> Hrm. Is c++ a system library or a special value understood by g-ir-scanner?
> If it's the latter, is it documented somewhere?

libc++ is LLVM's version of libstdc++, so it's a system library.

That makes me realize though, that I'd need to add libstdc++ on normal systems and libc++ on systems using XCode's linker. So I think removing ADD_WHOLE_ARCHIVE_TO_LIBRARIES is a non-starter; better to use --whole-archive when it's available.
Comment 8 Philip Chimento 2015-10-12 15:26:44 PDT
Created attachment 262929 [details]
Patch
Comment 9 Philip Chimento 2015-10-12 15:27:34 PDT
Here's a new patch with the check inside the macro, as suggested, and everything inside CMAKE_SYSTEM_NAME checks.
Comment 10 Martin Robinson 2015-10-12 15:31:56 PDT
Comment on attachment 262929 [details]
Patch

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

Looks good, but I want to suggest one small thing before committing the patch.

> Source/WebKit2/PlatformGTK.cmake:929
> +    set(DARWIN_ADDITIONAL_LIBRARIES --library=c++)
> +    set(DARWIN_ADDITIONAL_LDFLAGS -lGObjectDOMBindings)

Do you mind renaming these to INTROSPECTION_ADDITIONAL_LIBRARIES and INTROSPECTION_ADDITIONAL_LDFLAGS to make it consistent with INTROSPECTION_ADDITIONAL_LIBRARY_PATH?
Comment 11 Philip Chimento 2015-10-12 15:37:16 PDT
Created attachment 262930 [details]
Patch
Comment 12 Philip Chimento 2015-10-12 15:38:02 PDT
Sure thing, here it is. Thanks for the review.
Comment 13 WebKit Commit Bot 2015-10-12 17:38:23 PDT
Comment on attachment 262930 [details]
Patch

Clearing flags on attachment: 262930

Committed r190909: <http://trac.webkit.org/changeset/190909>
Comment 14 WebKit Commit Bot 2015-10-12 17:38:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Jeremy Huddleston Sequoia 2016-01-14 23:21:55 PST
This was fixed incorrectly.  Following up in bug #153117