Bug 141816 – Add SOFT_LINK_CONSTANT_SOURCE() cross-platform macro and start using it

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Add SOFT_LINK_CONSTANT_SOURCE() cross-platform macro and start using it.

Also, make ap happy again by removing CoreMediaSoftLinking.h (Bug 141655 Comment #37).

[David Kilzer (:ddkilzer)]

Created attachment 246927 [details]
Patch v1 (for EWS test; need to add the other constants)

[David Kilzer (:ddkilzer)]

Created attachment 246933 [details]
Patch v2 (for EWS builds)

[David Kilzer (:ddkilzer)]

Created attachment 246968 [details]
Patch v3 (for EWS builds)

[Brent Fulgham]

Comment on attachment 246968 [details]
Patch v3 (for EWS builds)

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

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:-77
> -#include "CoreMediaSoftLinking.h"

CoreMediaSoftLinking defines a method "CoreMediaLibrary()" that was defined by SOFT_LINK_LIBRARY(CoreMedia) and indicated if the CoreMedia.dll was present and linked to. We might need to expose some kind of predicate that indicates if the CoreMedia (and later, AVFoundationCF) stuff is available so we can exit out of processing early on systems that do not support certain features.

Removing this line causes a build failure because MediaPlayerPrivateAVFoundationCF is trying to access the CoreMediaLibrary() function to see if it can do CoreMedia things.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> Comment on attachment 246968 [details]
> Patch v3 (for EWS builds)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246968&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:-77
> > -#include "CoreMediaSoftLinking.h"
> 
> CoreMediaSoftLinking defines a method "CoreMediaLibrary()" that was defined
> by SOFT_LINK_LIBRARY(CoreMedia) and indicated if the CoreMedia.dll was
> present and linked to. We might need to expose some kind of predicate that
> indicates if the CoreMedia (and later, AVFoundationCF) stuff is available so
> we can exit out of processing early on systems that do not support certain
> features.
> 
> Removing this line causes a build failure because
> MediaPlayerPrivateAVFoundationCF is trying to access the CoreMediaLibrary()
> function to see if it can do CoreMedia things.

Thanks!  I have an idea of how to fix this, but forgot I needed to add it when removing CoreMediaSoftLinking.h.

[David Kilzer (:ddkilzer)]

Created attachment 247037 [details]
Patch v4 (test EWS builds)

[David Kilzer (:ddkilzer)]

Created attachment 247039 [details]
Patch v5 (test EWS builds)

[David Kilzer (:ddkilzer)]

Created attachment 247041 [details]
Patch v6 (test EWS builds)

[David Kilzer (:ddkilzer)]

Created attachment 247042 [details]
Patch v7 (test EWS builds)

[David Kilzer (:ddkilzer)]

Created attachment 247044 [details]
Patch v8 (test EWS builds)

[David Kilzer (:ddkilzer)]

Created attachment 247046 [details]
Patch v9 (test EWS builds)

[David Kilzer (:ddkilzer)]

(In reply to comment #11)
> Created attachment 247046 [details]
> Patch v9 (test EWS builds)

         CoreMediaSoftLink.cpp
     1>..\platform\cf\CoreMediaSoftLink.cpp(36): error C2660: 'WebCore::CoreMediaLibrary' : function does not take 0 arguments
     1>..\platform\cf\CoreMediaSoftLink.cpp(37): error C2660: 'WebCore::CoreMediaLibrary' : function does not take 0 arguments
     1>..\platform\cf\CoreMediaSoftLink.cpp(38): error C2660: 'WebCore::CoreMediaLibrary' : function does not take 0 arguments
     1>..\platform\cf\CoreMediaSoftLink.cpp(39): error C2660: 'WebCore::CoreMediaLibrary' : function does not take 0 arguments
     1>..\platform\cf\CoreMediaSoftLink.cpp(40): error C2660: 'WebCore::CoreMediaLibrary' : function does not take 0 arguments
     1>..\platform\cf\CoreMediaSoftLink.cpp(42): error C2660: 'WebCore::CoreMediaLibrary' : function does not take 0 arguments
     1>..\platform\cf\CoreMediaSoftLink.cpp(53): error C2660: 'WebCore::CoreMediaLibrary' : function does not take 0 arguments
     1>..\platform\cf\CoreMediaSoftLink.cpp(54): error C2660: 'WebCore::CoreMediaLibrary' : function does not take 0 arguments

[David Kilzer (:ddkilzer)]

Created attachment 247047 [details]
Patch v10 (test EWS builds)

[David Kilzer (:ddkilzer)]

(In reply to comment #13)
> Created attachment 247047 [details]
> Patch v10 (test EWS builds)

     1>WebCore.lib(MediaTimeAVFoundation.obj) : error LNK2005: "bool __cdecl WebCore::isCoreMediaFrameworkAvailable(void)" (?isCoreMediaFrameworkAvailable@WebCore@@YA_NXZ) already defined in WebCore.lib(MediaPlayerPrivateAVFoundationCF.obj)
         translator_common.lib(ShaderLang.obj) : MSIL .netmodule or module compiled with /GL found; restarting link with /LTCG; add /LTCG to the link command line to improve linker performance
     1>WebCore.lib(MediaTimeAVFoundation.obj) : error LNK2005: "bool __cdecl WebCore::isCoreMediaFrameworkAvailable(void)" (?isCoreMediaFrameworkAvailable@WebCore@@YA_NXZ) already defined in WebCore.lib(MediaPlayerPrivateAVFoundationCF.obj)
            Creating library C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\WebKit.lib and object C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\WebKit.exp
     1>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\bin32\WebKit.dll : fatal error LNK1169: one or more multiply defined symbols found
     1>Done Building Project "C:\cygwin\home\buildbot\WebKit\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj" (Build target(s)) -- FAILED.

I seem to be declaring WebCore::isCoreMediaFrameworkAvailable() incorrectly.  Maybe I don't need to pre-declare it on Windows?  

For Patch v9, I failed to pre-declare it for iOS/Mac, and got this error:

In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:35:
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/cf/CoreMediaSoftLink.h:35:1: error: no previous prototype for function 'isCoreMediaFrameworkAvailable' [-Werror,-Wmissing-prototypes]
SOFT_LINK_FRAMEWORK_HEADER(WebCore, CoreMedia)
^
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:35:
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/cf/CoreMediaSoftLink.h:32:
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/mac/SoftLinking.h:299:10: note: expanded from macro 'SOFT_LINK_FRAMEWORK_HEADER'
    bool is##framework##FrameworkAvailable() { \
         ^
<scratch space>:35:1: note: expanded from here
isCoreMediaFrameworkAvailable
^
1 error generated.

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> (In reply to comment #13)
> > Created attachment 247047 [details]
> > Patch v10 (test EWS builds)
> 
>      1>WebCore.lib(MediaTimeAVFoundation.obj) : error LNK2005: "bool __cdecl
> WebCore::isCoreMediaFrameworkAvailable(void)"
> (?isCoreMediaFrameworkAvailable@WebCore@@YA_NXZ) already defined in
> WebCore.lib(MediaPlayerPrivateAVFoundationCF.obj)
>          translator_common.lib(ShaderLang.obj) : MSIL .netmodule or module
> compiled with /GL found; restarting link with /LTCG; add /LTCG to the link
> command line to improve linker performance
>      1>WebCore.lib(MediaTimeAVFoundation.obj) : error LNK2005: "bool __cdecl
> WebCore::isCoreMediaFrameworkAvailable(void)"
> (?isCoreMediaFrameworkAvailable@WebCore@@YA_NXZ) already defined in
> WebCore.lib(MediaPlayerPrivateAVFoundationCF.obj)
>             Creating library
> C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\WebKit.lib and
> object C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\WebKit.exp
>      1>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\bin32\WebKit.dll :
> fatal error LNK1169: one or more multiply defined symbols found
>      1>Done Building Project
> "C:\cygwin\home\buildbot\WebKit\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.
> vcxproj" (Build target(s)) -- FAILED.
> 
> I seem to be declaring WebCore::isCoreMediaFrameworkAvailable() incorrectly.
> Maybe I don't need to pre-declare it on Windows?  
> 
> For Patch v9, I failed to pre-declare it for iOS/Mac, and got this error:
> 
> In file included from
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/avfoundation/
> AudioSourceProviderAVFObjC.mm:35:
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/cf/CoreMediaSoftLink.h:35:1:
> error: no previous prototype for function 'isCoreMediaFrameworkAvailable'
> [-Werror,-Wmissing-prototypes]
> SOFT_LINK_FRAMEWORK_HEADER(WebCore, CoreMedia)
> ^
> In file included from
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/avfoundation/
> AudioSourceProviderAVFObjC.mm:35:
> In file included from
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/cf/CoreMediaSoftLink.h:32:
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/mac/SoftLinking.h:299:10:
> note: expanded from macro 'SOFT_LINK_FRAMEWORK_HEADER'
>     bool is##framework##FrameworkAvailable() { \
>          ^
> <scratch space>:35:1: note: expanded from here
> isCoreMediaFrameworkAvailable
> ^
> 1 error generated.

Oh, maybe an 'inline' keyword would help.

[David Kilzer (:ddkilzer)]

Created attachment 247048 [details]
Patch v11 (for review)

[Alexey Proskuryakov]

Comment on attachment 247048 [details]
Patch v11 (for review)

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

rs=me; I could not follow the the macros in enough detail to fully review, but it must be right if it compiles.

> Source/WebCore/platform/cf/CoreMediaSoftLink.cpp:43
>  SOFT_LINK_FUNCTION_SOURCE(WebCore, CoreMedia, CMTimeRangeGetEnd, CMTime, (CMTimeRange range), (range))
>  
> +SOFT_LINK_CONSTANT_SOURCE(WebCore, CoreMedia, kCMTimeZero, CMTime);
> +

This is not new naming, but I don't understand or like it. Does it mean "implementation", as opposed to "declaration"?

> Source/WebCore/platform/cf/CoreMediaSoftLink.h:35
> +SOFT_LINK_FRAMEWORK_HEADER(WebCore, CoreMedia)

As above, this is not a "header", this is something that is used in a header. So "HEADER" is not right as the main noun in it name.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:149
> +#pragma mark - Soft Linking
> +
> +#import "CoreMediaSoftLink.h"

You are adding a very long comment about include order to MediaPlayerPrivateAVFoundationCF.cpp, but not here. Why?

> Source/WebCore/platform/mac/SoftLinking.h:340
> +            ASSERT_WITH_MESSAGE(constant, "%s", dlerror()); \

Why not a RELEASE_ASSERT? In a dispatch_once, it's not a problem for performance.

[David Kilzer (:ddkilzer)]

Comment on attachment 247048 [details]
Patch v11 (for review)

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

>> Source/WebCore/platform/cf/CoreMediaSoftLink.cpp:43
>> +
> 
> This is not new naming, but I don't understand or like it. Does it mean "implementation", as opposed to "declaration"?

In r180367, I renamed "IMPL" to "SOURCE" and "DECL" to "HEADER".  The idea is that the *_HEADER macros go in the header file, and the *_SOURCE macros go in the source file.

I thought "IMPLEMENTATION" and "DECLARATION" were too long to type, and "IMPL" and "DECL" were abbreviations which are eschewed in WebKit.

When using this pattern and you want to add a function, constant, etc. to be soft-linked, there will be a header macro that goes in the header file, and a source macro that goes in the source file.

What alternative do you propose?

>> Source/WebCore/platform/cf/CoreMediaSoftLink.h:35
>> +SOFT_LINK_FRAMEWORK_HEADER(WebCore, CoreMedia)
> 
> As above, this is not a "header", this is something that is used in a header. So "HEADER" is not right as the main noun in it name.

See my comment above.  I think this is a much easier pattern to follow than "IMPL" and "DECL" or "IMPLEMENTATION" and "DECLARATION".

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:76
> -// The softlink header files must be included after the AVCF and CoreMedia header files.
> +// The soft-link header files must be included after the AVCF header files
> +// because they use #define macros to rename C functions, constants, etc. to
> +// their soft-link equivalents.

I'll shorten this comment.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:149
>> +#import "CoreMediaSoftLink.h"
> 
> You are adding a very long comment about include order to MediaPlayerPrivateAVFoundationCF.cpp, but not here. Why?

I'll add a shortened comment to match the other comment.

>> Source/WebCore/platform/mac/SoftLinking.h:340
>> +            ASSERT_WITH_MESSAGE(constant, "%s", dlerror()); \
> 
> Why not a RELEASE_ASSERT? In a dispatch_once, it's not a problem for performance.

Is there any advantage crashing here versus the next line with a NULL deref?  I guess you might get a log message if ReportCrash includes it.

I can change them if you have a specific opinion (rather than just a question), but we might as well change all of them at once, so I'm going to leave this one as-is when landing this patch.

[David Kilzer (:ddkilzer)]

Committed r180481: <http://trac.webkit.org/changeset/180481>

[Alexey Proskuryakov]

To correctly name the macros in accordance to this idea, that should be _FOR_HEADER and _FOR_CPP, I think.

> Is there any advantage crashing here versus the next line with a NULL deref?

Probably no practical advantage in this case, however in general, we can never rely on NULL deref being a crash. That's undefined behavior, and as you are well aware, the compiler does exceptionally crazy things when it optimizes code with potentially undefined behavior.

[David Kilzer (:ddkilzer)]

(In reply to comment #20)
> To correctly name the macros in accordance to this idea, that should be
> _FOR_HEADER and _FOR_CPP, I think.

_FOR_HEADER seems fine.

I think _FOR_CPP is ambiguous at best.  Does "CPP" mean "C++ Source file [extension]" or "C pre-processor"?  I would suggest _FOR_SOURCE here.

> > Is there any advantage crashing here versus the next line with a NULL deref?
> 
> Probably no practical advantage in this case, however in general, we can
> never rely on NULL deref being a crash. That's undefined behavior, and as
> you are well aware, the compiler does exceptionally crazy things when it
> optimizes code with potentially undefined behavior.

I'll fix in a follow-up, although I don't think this a case where it would fail that way because it's dereferencing NULL, not comparing something to NULL.

[David Kilzer (:ddkilzer)]

iOS build fix:  Commit r180482:  <http://trac.webkit.org/r180482>

[David Kilzer (:ddkilzer)]

(In reply to comment #21)
> (In reply to comment #20)
> > To correctly name the macros in accordance to this idea, that should be
> > _FOR_HEADER and _FOR_CPP, I think.
> 
> _FOR_HEADER seems fine.
> 
> I think _FOR_CPP is ambiguous at best.  Does "CPP" mean "C++ Source file
> [extension]" or "C pre-processor"?  I would suggest _FOR_SOURCE here.

Bug 142177: Rename SOFT_LINK_{CONSTANT,FUNCTION}_{HEADER,SOURCE} to SOFT_LINK_{CONSTANT,FUNCTION}_FOR_{HEADER,SOURCE}

> > > Is there any advantage crashing here versus the next line with a NULL deref?
> > 
> > Probably no practical advantage in this case, however in general, we can
> > never rely on NULL deref being a crash. That's undefined behavior, and as
> > you are well aware, the compiler does exceptionally crazy things when it
> > optimizes code with potentially undefined behavior.
> 
> I'll fix in a follow-up, although I don't think this a case where it would
> fail that way because it's dereferencing NULL, not comparing something to
> NULL.

Bug 142176: Switch new soft-linking debug asserts to release asserts