Bug 144557

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.

Created attachment 252688 [details] Patch

I'm not sure about this patch. What do you think Martin, Carlos?

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.

(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 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?

(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.

Created attachment 262929 [details] Patch

Here's a new patch with the check inside the macro, as suggested, and everything inside CMAKE_SYSTEM_NAME checks.

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?

Created attachment 262930 [details] Patch

Sure thing, here it is. Thanks for the review.

Comment on attachment 262930 [details] Patch Clearing flags on attachment: 262930 Committed r190909: <http://trac.webkit.org/changeset/190909>

All reviewed patches have been landed. Closing bug.

This was fixed incorrectly. Following up in bug #153117