Summary: | [GTK] OSX linker doesn't understand --whole-archive | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philip Chimento <philip.chimento> | ||||||||
Component: | WebKitGTK | Assignee: | 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
Philip Chimento
2015-05-03 16:50:49 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. 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 |