Bug 144837 – [Cocoa] Don't soft link DataDetectorsCore

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Darin Adler]

[Cocoa] Don't soft link DataDetectorsCore

[Darin Adler]

Created attachment 252792 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

And yes, I am happy to land the project file fixes separate from the DataDetectorsCore change. Won’t be hard to disentangle them.

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.

[Darin Adler]

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.

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

[Darin Adler]

Created attachment 252797 [details]
Patch

[Darin Adler]

Created attachment 252798 [details]
Patch

[Darin Adler]

Created attachment 252799 [details]
Patch

[Darin Adler]

Created attachment 252816 [details]
Patch

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

[Darin Adler]

Committed r184047: <http://trac.webkit.org/changeset/184047>

[Darin Adler]

<rdar://problem/20890647>

[Darin Adler]

Committed follow-up fix r184052: <http://trac.webkit.org/changeset/184052>

[Myles C. Maxfield]

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.

[Darin Adler]

It seems that we need to export the soft-linking symbols to fix that.

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

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

Is there a downside to not checking for weak vTables and externals when building with ASAN?

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Zalan fixed the failure in bug 145070.