Bug 144837 - [Cocoa] Don't soft link DataDetectorsCore
Summary: [Cocoa] Don't soft link DataDetectorsCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-05-09 18:08 PDT by Darin Adler
Modified: 2015-05-15 14:15 PDT (History)
7 users (show)

See Also:


Attachments
Patch (317.61 KB, patch)
2015-05-09 18:18 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (18.06 KB, patch)
2015-05-09 20:22 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (27.47 KB, patch)
2015-05-09 20:26 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (21.83 KB, patch)
2015-05-09 20:28 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (21.93 KB, patch)
2015-05-10 09:31 PDT, Darin Adler
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2015-05-09 18:08:17 PDT
[Cocoa] Don't soft link DataDetectorsCore
Comment 1 Darin Adler 2015-05-09 18:18:44 PDT
Created attachment 252792 [details]
Patch
Comment 2 Darin Adler 2015-05-09 18:21:38 PDT
I noticed the dlopen for this framework in a profile of the web process when launching Safari. The only thing I am unsure about is whether this will cause some kind of dependency cycle in Apple's build process.
Comment 3 Darin Adler 2015-05-09 18:22:26 PDT
And yes, I am happy to land the project file fixes separate from the DataDetectorsCore change. Won’t be hard to disentangle them.
Comment 4 mitz 2015-05-09 19:48:37 PDT
For this to work, $(SYSTEM_LIBRARY_DIR)/PrivateFrameworks needs to be added to FRAMEWORK_SEARCH_PATHS[sdk=macosx*]. Everything compiles because of the system frameworks include path in OTHER_CFLAGS, but linking is failing.
Comment 5 Darin Adler 2015-05-09 19:50:24 PDT
OK.

Xcode had actually added that to the project, and I deleted it by hand, but clearly we would need to add it to the .xcconfig file instead as your syntax implies.

I currently have this linking for iOS as well as Mac. I guess we don’t want that. I don’t know how to have a conditional framework in the project file.
Comment 6 mitz 2015-05-09 19:52:48 PDT
(In reply to comment #5)
> I currently have this linking for iOS as well as Mac. I guess we don’t want
> that. I don’t know how to have a conditional framework in the project file.

Good point! It should go in OTHER_LDFLAGS_PLATFORM[sdk=macosx*].
Comment 7 Darin Adler 2015-05-09 20:22:15 PDT
Created attachment 252797 [details]
Patch
Comment 8 Darin Adler 2015-05-09 20:26:17 PDT
Created attachment 252798 [details]
Patch
Comment 9 Darin Adler 2015-05-09 20:28:22 PDT
Created attachment 252799 [details]
Patch
Comment 10 Darin Adler 2015-05-10 09:31:37 PDT
Created attachment 252816 [details]
Patch
Comment 11 mitz 2015-05-10 10:21:45 PDT
Comment on attachment 252816 [details]
Patch

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

> Source/WebCore/platform/spi/mac/DataDetectorsSPI.h:27
> +#import "DataDetectorsCoreSPI.h"
>  #import "SoftLinking.h"

This being a framework (private) header I would have expected these to be #import <WebCore/DataDetectorsCoreSPI.h> and #import <WebCore/SoftLinking.h>.
Comment 12 Darin Adler 2015-05-10 11:31:22 PDT
Committed r184047: <http://trac.webkit.org/changeset/184047>
Comment 13 Darin Adler 2015-05-10 11:31:50 PDT
<rdar://problem/20890647>
Comment 14 Darin Adler 2015-05-10 13:30:39 PDT
Committed follow-up fix r184052: <http://trac.webkit.org/changeset/184052>
Comment 15 Myles C. Maxfield 2015-05-11 14:45:07 PDT
Upon compiling with ASAN, the "Check For Weak VTables and Externals" step fails with:

ERROR: WebCore has a weak external symbol in it (/Volumes/Data/home/mmaxfield/Build/Debug/WebCore.framework/Versions/A/WebCore)
ERROR: A weak external symbol is generated when a symbol is defined in multiple compilation units and is also marked as being exported from the library.
ERROR: A common cause of weak external symbols is when an inline function is listed in the linker export file.
ERROR: symbol _DDHighlightCreateWithRectsInVisibleRectWithStyleAndDirection
ERROR: symbol _DDHighlightPointIsOnHighlight

It looks like this patch is relevant.
Comment 16 Darin Adler 2015-05-11 17:22:51 PDT
It seems that we need to export the soft-linking symbols to fix that.
Comment 17 mitz 2015-05-11 17:51:02 PDT
(In reply to comment #16)
> It seems that we need to export the soft-linking symbols to fix that.

I don’t think we can simply export those symbols, because they are named the same as their counterparts in DataDetectors. If we did that, when building a target that links against WebKit and DataDetectors, the static might end up record those symbols as coming from WebKit.
Comment 18 mitz 2015-05-11 17:51:53 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > It seems that we need to export the soft-linking symbols to fix that.
> 
> I don’t think we can simply export those symbols, because they are named the
> same as their counterparts in DataDetectors. If we did that, when building a
> target that links against WebKit and DataDetectors, the static might end up
> record those symbols as coming from WebKit.

Unless we can exclude them from the blanket reexport of WebCore by WebKit.
Comment 19 mitz 2015-05-11 22:09:46 PDT
Is there a downside to not checking for weak vTables and externals when building with ASAN?
Comment 20 Alexey Proskuryakov 2015-05-11 22:19:40 PDT
I do not know if there is anything about ASan that would make a difference, and in fact this doesn't happen on ASan bots, as far as I can tell.
Comment 21 Alexey Proskuryakov 2015-05-15 14:15:19 PDT
Zalan fixed the failure in bug 145070.