Bug 141607 – REGRESSION (r180082): WebCore Debug builds fail on Mavericks due to weak export symbols

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

After r180082 landed, WebCore Debug builds fail on Mountain Lion due to weak export symbols:

    /bin/sh -c OpenSource/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5D0D540D0E9862F60029E223.sh
ERROR: WebCore has a weak external symbol in it (OpenSource/WebKitBuild/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 _AudioConverterConvertComplexBuffer
ERROR: symbol _AudioConverterNew
ERROR: symbol _CMAudioDeviceClockCreate
ERROR: symbol _CMBlockBufferCopyDataBytes
ERROR: symbol _CMBlockBufferGetDataLength
ERROR: symbol _CMFormatDescriptionGetExtensions
ERROR: symbol _CMSampleBufferGetDataBuffer
ERROR: symbol _CMSampleBufferGetFormatDescription
ERROR: symbol _CMSampleBufferGetSampleTimingInfo
ERROR: symbol _CMTimeCompare
ERROR: symbol _CMTimeGetSeconds
ERROR: symbol _CMTimeMake
ERROR: symbol _CMTimeMakeWithSeconds
ERROR: symbol _CMTimeRangeGetEnd
ERROR: symbol _CMTimebaseCreateWithMasterClock
ERROR: symbol _CMTimebaseGetTime
ERROR: symbol _CMTimebaseSetRate
ERROR: symbol _CMTimebaseSetTime
ERROR: symbol _CTFontDescriptorCopyAttribute
ERROR: symbol _CVPixelBufferGetBaseAddress
ERROR: symbol _CVPixelBufferGetBytesPerRow
ERROR: symbol _CVPixelBufferGetDataSize
ERROR: symbol _CVPixelBufferGetHeight
ERROR: symbol _CVPixelBufferGetPixelFormatType
ERROR: symbol _CVPixelBufferGetWidth
ERROR: symbol _CVPixelBufferLockBaseAddress
ERROR: symbol _CVPixelBufferUnlockBaseAddress
ERROR: symbol _MACaptionAppearanceAddSelectedLanguage
ERROR: symbol _MACaptionAppearanceCopyBackgroundColor
ERROR: symbol _MACaptionAppearanceCopyFontDescriptorForStyle
ERROR: symbol _MACaptionAppearanceCopyForegroundColor
ERROR: symbol _MACaptionAppearanceCopyPreferredCaptioningMediaCharacteristics
ERROR: symbol _MACaptionAppearanceCopySelectedLanguages
ERROR: symbol _MACaptionAppearanceCopyWindowColor
ERROR: symbol _MACaptionAppearanceGetBackgroundOpacity
ERROR: symbol _MACaptionAppearanceGetDisplayType
ERROR: symbol _MACaptionAppearanceGetForegroundOpacity
ERROR: symbol _MACaptionAppearanceGetRelativeCharacterSize
ERROR: symbol _MACaptionAppearanceGetTextEdgeStyle
ERROR: symbol _MACaptionAppearanceGetWindowOpacity
ERROR: symbol _MACaptionAppearanceGetWindowRoundedCornerRadius
ERROR: symbol _MACaptionAppearanceSetDisplayType
ERROR: symbol _MTAudioProcessingTapGetSourceAudio
ERROR: symbol _MTAudioProcessingTapGetStorage
ERROR: symbol _VTPixelTransferSessionCreate
ERROR: symbol _VTPixelTransferSessionTransferImage
ERROR: symbol __ZN3JSC14JSGlobalObject16setConsoleClientEPNS_13ConsoleClientE
ERROR: symbol __ZN7WebCore19DragCaretController5clearEv
ERROR: symbol __ZN7WebCore4toJSEPN3JSC9ExecStateEPNS_17JSDOMGlobalObjectEPNS_8NodeListE
ERROR: symbol __ZN7WebCore5Event6createERKN3WTF12AtomicStringEbb
ERROR: symbol __ZN7WebCore8Document24setAnnotatedRegionsDirtyEb
ERROR: symbol __ZN7WebCore8PositionC1Ev
ERROR: symbol __ZN7WebCore8PositionC2Ev
ERROR: symbol __ZNK3JSC7JSValue7toFloatEPNS_9ExecStateE
ERROR: symbol __ZNK7WebCore14ResourceLoader15originalRequestEv
ERROR: symbol __ZNK7WebCore4Node16hasEditableStyleENS_12EditableTypeENS0_22UserSelectAllTreatmentE
ERROR: symbol __ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE0_NS_13basic_istreamIcS2_EE
ERROR: symbol __ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE0_NS_14basic_iostreamIcS2_EE
ERROR: symbol __ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE16_NS_13basic_ostreamIcS2_EE
ERROR: symbol __ZTCNSt3__119basic_istringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE0_NS_13basic_istreamIcS2_EE
ERROR: symbol __ZTCNSt3__119basic_ostringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE0_NS_13basic_ostreamIcS2_EE
ERROR: symbol __ZTTNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE
ERROR: symbol __ZTTNSt3__119basic_istringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE
ERROR: symbol __ZTTNSt3__119basic_ostringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE
ERROR: symbol __ZTVN9Inspector17ScriptDebugServer4TaskE
ERROR: symbol __ZTVN9Inspector22InspectorDebuggerAgent8ListenerE
ERROR: symbol __ZTVNSt3__115basic_stringbufIcNS_11char_traitsIcEENS_9allocatorIcEEEE
ERROR: symbol __ZTVNSt3__117bad_function_callE
ERROR: symbol __ZTVNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE
ERROR: symbol __ZTVNSt3__119basic_istringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE
ERROR: symbol __ZTVNSt3__119basic_ostringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE
Command /bin/sh failed with exit code 1

** BUILD FAILED **


The following build commands failed:
	PhaseScriptExecution Check\ For\ Weak\ VTables\ and\ Externals OpenSource/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5D0D540D0E9862F60029E223.sh
(1 failure)

[David Kilzer (:ddkilzer)]

Created attachment 246601 [details]
Patch v1

[David Kilzer (:ddkilzer)]

I'm not sure if listing "extra" symbols in WebCore.unexp is an issue or not.

[David Kilzer (:ddkilzer)]

Oops, this was Mavericks, not Mountain Lion.  I've already forgotten what the Mavericks background looks like.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> I'm not sure if listing "extra" symbols in WebCore.unexp is an issue or not.

Meaning that I'm not sure whether adding these symbols will break Release builds if the weak symbols don't appear in the binary.

[David Kilzer (:ddkilzer)]

Also, the JavaScriptCore and WebCore symbols should probably be marked as exported instead of listed in this file, correct?

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> Also, the JavaScriptCore and WebCore symbols should probably be marked as
> exported instead of listed in this file, correct?

Most of them look like inlined functions defined in a header that are already exported.  I guess that's what the bug is that WebCore.unexp is working around?

[David Kilzer (:ddkilzer)]

Comment on attachment 246601 [details]
Patch v1

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

> Source/WebCore/Configurations/WebCore.unexp:67
> +__ZN3JSC14JSGlobalObject16setConsoleClientEPNS_13ConsoleClientE

JS_EXPORT_PRIVATE void setConsoleClient(ConsoleClient* consoleClient) { m_consoleClient = consoleClient; }

> Source/WebCore/Configurations/WebCore.unexp:68
> +__ZN7WebCore19DragCaretController5clearEv

WEBCORE_EXPORT void clear() { setCaretPosition(VisiblePosition()); }

> Source/WebCore/Configurations/WebCore.unexp:69
> +__ZN7WebCore4toJSEPN3JSC9ExecStateEPNS_17JSDOMGlobalObjectEPNS_8NodeListE

ALWAYS_INLINE JSC::JSValue toJS(JSC::ExecState*, JSDOMGlobalObject* globalObject, NodeList* nodeList)
{
    if (!nodeList)
        return JSC::jsNull();
    if (JSC::JSValue existingWrapper = getExistingWrapper<JSNodeList>(globalObject, nodeList))
        return existingWrapper;
    return createWrapper(*globalObject, *nodeList);
}

This should never appear as a symbol if it's always inlined, although maybe Debug builds turn off the inlining such that the symbol is still present?

> Source/WebCore/Configurations/WebCore.unexp:70
> +__ZN7WebCore5Event6createERKN3WTF12AtomicStringEbb

WEBCORE_EXPORT static Ref<Event> create(const AtomicString& type, bool canBubble, bool cancelable)
    {
        return adoptRef(*new Event(type, canBubble, cancelable));
    }

> Source/WebCore/Configurations/WebCore.unexp:71
> +__ZN7WebCore8Document24setAnnotatedRegionsDirtyEb

WEBCORE_EXPORT void setAnnotatedRegionsDirty(bool f) { m_annotatedRegionsDirty = f; }

> Source/WebCore/Configurations/WebCore.unexp:73
> +__ZN7WebCore8PositionC1Ev
> +__ZN7WebCore8PositionC2Ev

WEBCORE_EXPORT Position()
        : m_offset(0)
        , m_anchorType(PositionIsOffsetInAnchor)
        , m_isLegacyEditingPosition(false)
    {
    }

> Source/WebCore/Configurations/WebCore.unexp:75
> +__ZNK7WebCore14ResourceLoader15originalRequestEv

WEBCORE_EXPORT const ResourceRequest& originalRequest() const { return m_originalRequest; }

> Source/WebCore/Configurations/WebCore.unexp:76
> +__ZNK7WebCore4Node16hasEditableStyleENS_12EditableTypeENS0_22UserSelectAllTreatmentE

WEBCORE_EXPORT bool hasEditableStyle(EditableType editableType = ContentIsEditable, UserSelectAllTreatment treatment = UserSelectAllIsAlwaysNonEditable) const
    {
        switch (editableType) {
        case ContentIsEditable:
            return hasEditableStyle(Editable, treatment);
        case HasEditableAXRole:
            return isEditableToAccessibility(Editable);
        }
        ASSERT_NOT_REACHED();
        return false;
    }

> Source/WebCore/Configurations/WebCore.unexp:77
> +__ZTVN9Inspector17ScriptDebugServer4TaskE

class JS_EXPORT_PRIVATE ScriptDebugServer : public JSC::Debugger {
[...]
public:
[...]
    class Task {
        WTF_MAKE_FAST_ALLOCATED;
    public:
        virtual ~Task() { }
        virtual void run() = 0;
    };

Maybe Task needs to be exported here?

> Source/WebCore/Configurations/WebCore.unexp:78
> +__ZTVN9Inspector22InspectorDebuggerAgent8ListenerE

class JS_EXPORT_PRIVATE InspectorDebuggerAgent : public InspectorAgentBase, public ScriptDebugListener, public InspectorDebuggerBackendDispatcherHandler {
[...]
public:
[...]
    class Listener {
    public:
        virtual ~Listener() { }
        virtual void debuggerWasEnabled() = 0;
        virtual void debuggerWasDisabled() = 0;
        virtual void stepInto() = 0;
        virtual void didPause() = 0;
    };

Maybe Listener needs to be exported here?

[Mark Rowe (bdash)]

Is it really sensible for inline functions declared in headers to be marked as exported? Why are symbols from CoreMedia and friends being marked as exported from WebCore at all? These seem like issues that should be fixed at the source rather than worked around in the .unexp file.

[David Kilzer (:ddkilzer)]

Comment on attachment 246601 [details]
Patch v1

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

> Source/WebCore/Configurations/WebCore.unexp:47
> +_CVPixelBufferGetBaseAddress
> +_CVPixelBufferGetBytesPerRow
> +_CVPixelBufferGetDataSize
> +_CVPixelBufferGetHeight
> +_CVPixelBufferGetPixelFormatType
> +_CVPixelBufferGetWidth
> +_CVPixelBufferLockBaseAddress
> +_CVPixelBufferUnlockBaseAddress

These appear to be soft-linked symbols:

SOFT_LINK(CoreVideo, CVPixelBufferGetWidth, size_t, (CVPixelBufferRef pixelBuffer), (pixelBuffer))
SOFT_LINK(CoreVideo, CVPixelBufferGetHeight, size_t, (CVPixelBufferRef pixelBuffer), (pixelBuffer))
SOFT_LINK(CoreVideo, CVPixelBufferGetBaseAddress, void*, (CVPixelBufferRef pixelBuffer), (pixelBuffer))
SOFT_LINK(CoreVideo, CVPixelBufferGetBytesPerRow, size_t, (CVPixelBufferRef pixelBuffer), (pixelBuffer))
SOFT_LINK(CoreVideo, CVPixelBufferGetDataSize, size_t, (CVPixelBufferRef pixelBuffer), (pixelBuffer))
SOFT_LINK(CoreVideo, CVPixelBufferGetPixelFormatType, OSType, (CVPixelBufferRef pixelBuffer), (pixelBuffer))
SOFT_LINK(CoreVideo, CVPixelBufferLockBaseAddress, CVReturn, (CVPixelBufferRef pixelBuffer, CVOptionFlags lockFlags), (pixelBuffer, lockFlags))
SOFT_LINK(CoreVideo, CVPixelBufferUnlockBaseAddress, CVReturn, (CVPixelBufferRef pixelBuffer, CVOptionFlags lockFlags), (pixelBuffer, lockFlags))

I suspect the rest of them are as well.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> Is it really sensible for inline functions declared in headers to be marked
> as exported? Why are symbols from CoreMedia and friends being marked as
> exported from WebCore at all? These seem like issues that should be fixed at
> the source rather than worked around in the .unexp file.

True, but I suspect it would be better to unbreak the build first, then fix the issues so that the symbols could be removed from the .unexp file.

Also, I think these symbols need to be ignored in Tools/Scripts/check-for-weak-vtables-and-externals:

__ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE0_NS_13basic_istreamIcS2_EE
__ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE8_NS_13basic_ostreamIcS2_EE
__ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE0_NS_14basic_iostreamIcS2_EE
__ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE16_NS_13basic_ostreamIcS2_EE
__ZTCNSt3__119basic_istringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE0_NS_13basic_istreamIcS2_EE
__ZTCNSt3__119basic_ostringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE0_NS_13basic_ostreamIcS2_EE
__ZTTNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE
__ZTTNSt3__119basic_istringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE
__ZTTNSt3__119basic_ostringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE
__ZTVNSt3__115basic_stringbufIcNS_11char_traitsIcEENS_9allocatorIcEEEE
__ZTVNSt3__117bad_function_callE
__ZTVNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE
__ZTVNSt3__119basic_istringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE
__ZTVNSt3__119basic_ostringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE

[David Kilzer (:ddkilzer)]

Work towards fixing the Mavericks Debug build:

Committed r180114: <http://trac.webkit.org/changeset/180114>

[David Kilzer (:ddkilzer)]

Created attachment 246613 [details]
Patch #2 v1

[David Kilzer (:ddkilzer)]

Created attachment 246615 [details]
Try declaring soft-linked functions with extern "C" linkage

[David Kilzer (:ddkilzer)]

(In reply to comment #13)
> Created attachment 246615 [details]
> Try declaring soft-linked functions with extern "C" linkage

Committed r180122: <http://trac.webkit.org/changeset/180122>

This should hopefully clean up most of the soft-linked functions being complained about on the Mavericks Debug build.  If it does, that means that these functions were not declared by another header (such as an *SPI.h header or a framework header).  That should be fixed separately.

On a positive note, for functions that were declared by another header, if the declarations don't match, the build should fail (which is what we want to happen so we can fix the function parameters when soft-linking).

[Alexey Proskuryakov]

This broke the build on Yosemite open source builders.

[Alexey Proskuryakov]

Attempted a fix in r180124.

It doesn't seem like we are on the right track here. How can a soft link declaration possibly affect linker warnings? The names we use for soft linked entities do not clash with original names, they are entirely separate (e.g. softLinkAudioConverterNew). And r180122 only changed a declaration, not any definition.

[Alexey Proskuryakov]

Landing the build fix was quicker for me at the time, but really, both 180122 and 180124 should be rolled out.

[Alexey Proskuryakov]

Actually, let me take that back. SOFT_LINK does define the function with that name.

[Alexey Proskuryakov]

More build fixing in <http://trac.webkit.org/r180125>. Sorry if this will break more than it fixes, I don't have a trunk build to confirm with.

[Alexey Proskuryakov]

More in r180126.

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> (In reply to comment #13)
> > Created attachment 246615 [details]
> > Try declaring soft-linked functions with extern "C" linkage
> 
> Committed r180122: <http://trac.webkit.org/changeset/180122>
> 
> This should hopefully clean up most of the soft-linked functions being
> complained about on the Mavericks Debug build.  If it does, that means that
> these functions were not declared by another header (such as an *SPI.h
> header or a framework header).  That should be fixed separately.

These declarations had no effect on the Mavericks Debug build.

> On a positive note, for functions that were declared by another header, if
> the declarations don't match, the build should fail (which is what we want
> to happen so we can fix the function parameters when soft-linking).

However, they did find some other incorrect declarations (r180124, r180125).  Sorry for breaking the build, Alexey.  I was watching EWS bubbles too intently instead of checking the dashboard as well.

[David Kilzer (:ddkilzer)]

Created attachment 246618 [details]
SOFT_LINK() shoulde hide redeclared function like SOFT_LINK_MAY_FAIL()

[David Kilzer (:ddkilzer)]

Comment on attachment 246618 [details]
SOFT_LINK() shoulde hide redeclared function like SOFT_LINK_MAY_FAIL()

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

> Source/WebCore/platform/mac/SoftLinking.h:111
> -    inline resultType functionName parameterDeclarations \
> +    __attribute__((visibility("hidden"))) inline resultType functionName parameterDeclarations \

Why not just use 'static' instead of '__attribute__((visibility("hidden")))' here and in SOFT_LINKING_MAY_FAIL()?

[David Kilzer (:ddkilzer)]

Comment on attachment 246618 [details]
SOFT_LINK() shoulde hide redeclared function like SOFT_LINK_MAY_FAIL()

This seems like the correct approach, but the redefinition of the method conflicts with at least some methods that set an explicit visibility themselves, like MediaAccessibility.framework:

/Volumes/Data/EWS/WebKit/Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:79:1: error: visibility does not match previous declaration
SOFT_LINK_AVF_FRAMEWORK_IMPORT(MediaAccessibility, MACaptionAppearanceGetDisplayType, MACaptionAppearanceDisplayType, (MACaptionAppearanceDomain domain), (domain))
^
/Volumes/Data/EWS/WebKit/Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:62:84: note: expanded from macro 'SOFT_LINK_AVF_FRAMEWORK_IMPORT'
#define SOFT_LINK_AVF_FRAMEWORK_IMPORT(Lib, Fun, ReturnType, Arguments, Signature) SOFT_LINK(Lib, Fun, ReturnType, Arguments, Signature)
                                                                                   ^
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:39:
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/mac/SoftLinking.h:111:20: note: expanded from macro 'SOFT_LINK'
    __attribute__((visibility("hidden"))) inline resultType functionName parameterDeclarations \
                   ^
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:53:
In file included from /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS8.1.sdk/System/Library/Frameworks/MediaAccessibility.framework/Headers/MediaAccessibility.h:8:
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS8.1.sdk/System/Library/Frameworks/MediaAccessibility.framework/Headers/MACaptionAppearance.h:212:1: note: previous attribute is here
MA_EXPORT
^
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:53:
In file included from /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS8.1.sdk/System/Library/Frameworks/MediaAccessibility.framework/Headers/MediaAccessibility.h:7:
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS8.1.sdk/System/Library/Frameworks/MediaAccessibility.framework/Headers/MADefinitions.h:36:29: note: expanded from macro 'MA_EXPORT'
#define MA_EXPORT MA_EXTERN MA_VISIBLE
                            ^
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS8.1.sdk/System/Library/Frameworks/MediaAccessibility.framework/Headers/MADefinitions.h:27:38: note: expanded from macro 'MA_VISIBLE'
#   define MA_VISIBLE __attribute__((visibility ("default")))
                                     ^

[David Kilzer (:ddkilzer)]

Created attachment 246619 [details]
CoreText only needs to be soft-linked on Windows

[David Kilzer (:ddkilzer)]

Created attachment 246620 [details]
CoreText only needs to be soft-linked on Windows v2

[Mark Rowe (bdash)]

(In reply to comment #24)
> Comment on attachment 246618 [details]
> SOFT_LINK() shoulde hide redeclared function like SOFT_LINK_MAY_FAIL()
> 
> This seems like the correct approach, but the redefinition of the method
> conflicts with at least some methods that set an explicit visibility
> themselves, like MediaAccessibility.framework:

In WebKitSystemInterface we do shenanigans with __AVAILABILITY_INTERNAL_REGULAR in order to prevent symbols declared in other frameworks from having the wrong visibility.

[David Kilzer (:ddkilzer)]

CoreText only needs to be soft-linked on Windows
Committed r180127: <http://trac.webkit.org/changeset/180127>

[David Kilzer (:ddkilzer)]

(In reply to comment #27)
> (In reply to comment #24)
> > Comment on attachment 246618 [details]
> > SOFT_LINK() shoulde hide redeclared function like SOFT_LINK_MAY_FAIL()
> > 
> > This seems like the correct approach, but the redefinition of the method
> > conflicts with at least some methods that set an explicit visibility
> > themselves, like MediaAccessibility.framework:
> 
> In WebKitSystemInterface we do shenanigans with
> __AVAILABILITY_INTERNAL_REGULAR in order to prevent symbols declared in
> other frameworks from having the wrong visibility.

I think for that to work reasonably, we'd want to pull out all soft-linked libraries into a stand-alone static library target within the WebCore project.  We're not going to want to undefine __AVAILABILITY_INTERNAL_REGULAR in WebCorePrefix.h, as that would undermine our own availability macros.  And even if we limited the scope where it is undefined, I don't think there's any way to ensure that the headers we want to affect weren't included by other headers first.  In the case of MediaAccessibility, they have their own MA_EXPORT macro, so we'd be mucking with implementation details that could change at any time.

With so few new or changed soft-linked methods, and this being something we have to do to support older toolchains for a short period of time, it seems like listing the methods in the .unexp file isn't that burdensome.

A couple other related thoughts:

1. Could a C++ method in the WebCore namespace (say WebCore::AudioConverterNew) take precedence over an extern "C" method so that our redeclared inline method in SOFT_LINK() could be made hidden using __attribute__((visibility("hidden"))) without the compiler warning us about the ambiguity?  I think this would require an additional #define like this, though, for the remainder of the code:

#define AudioConverterNew(...) WebCore::AudioConverterNew(...)

This does cause a -Wdisabled-macro-expansion warning with clang, but I think that's only enabled with -Weverything.

Would this technique work with Windows?  I know the current technique doesn't, and requires macros like this to be defined instead:

#define AudioConverterNew softLink_AudioConverterNew

2. Soft-linking requires static variables and a few small methods be declared for every function.  When soft-linked functions are duplicated between compilation units, it results in duplicate code and a small amount of wasted space (as well as extra static variables that can cause dirty pages, I think).  It would be better if we could pull soft-linked functions into a single source file, and then include a header that would do the magic to use them where they're needed.

For example, CMTimeCompare is used in these source files:
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm
Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm

This is kind of similar to the *SPI.h header approach, and I was even wondering whether we could "magically" soft-link functions through *SPI.h headers (as needed/wanted).

[David Kilzer (:ddkilzer)]

I filed this bug for #2:  Bug 141625: Consolidate duplicate soft-linked functions

[David Kilzer (:ddkilzer)]

Back out subclass exports and fix one more exported inline function
Committed r180130: <http://trac.webkit.org/changeset/180130>

[David Kilzer (:ddkilzer)]

Work around the remaining issues to try to get the Mavericks Debug bot green:
Committed r180135:  <http://trac.webkit.org/changeset/180135>

I left a FIXME comment in WebCore.unexp to work on the remaining symbols.

[David Kilzer (:ddkilzer)]

(In reply to comment #32)
> Work around the remaining issues to try to get the Mavericks Debug bot green:
> Committed r180135:  <http://trac.webkit.org/changeset/180135>
> 
> I left a FIXME comment in WebCore.unexp to work on the remaining symbols.

Okay, this fixed the Mavericks Debug build, so moving to RESOLVED/FIXED.

The GTK Debug build may have broken with this change (or r180134), although there's not enough information in the stdio log for me to attempt to fix it.  I suspect it's unrelated, though, as r180135 had changes to files only used on Mac (or iOS):

<https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Build%29/builds/48444>

[David Kilzer (:ddkilzer)]

(In reply to comment #33)
> The GTK Debug build may have broken with this change (or r180134), although
> there's not enough information in the stdio log for me to attempt to fix it.
> I suspect it's unrelated, though, as r180135 had changes to files only used
> on Mac (or iOS):
> 
> <https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Debug%20%28Build%29/builds/48444>

It fixed itself with r180136, so this seems unrelated.