Bug 204708 – Build ANGLE as a dynamic library

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

Build ANGLE as a dynamic library

[Dean Jackson]

rdar://57349384

[Dean Jackson]

Created attachment 384524 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE

[Dean Jackson]

Created attachment 384527 [details]
Patch

Comment on attachment 384527 [details]
Patch

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

> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:79
> +// DEBUG_DEFINES, GCC_OPTIMIZATION_LEVEL, STRIP_INSTALLED_PRODUCT and DEAD_CODE_STRIPPING vary between the debug and normal variants.
> +// We set up the values for each variant here, and have the Debug configuration in the Xcode project use the _debug variant.

Is this comment true? I don’t see anything in the Xcode project that defines DEBUG_DEFINES, STRIP_INSTALLED_PRODUCT, or DEAD_CODE_STRIPPING, and I don’t think the Debug configuration builds the “debug” variant (that is, I believe it builds the “normal” variant).

> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:96
> +OTHER_CFLAGS = $(ASAN_OTHER_CFLAGS) -fvisibility=hidden;

I think this is better specified with the dedicated build setting GCC_SYMBOLS_PRIVATE_EXTERN.

> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:97
> +OTHER_CPLUSPLUSFLAGS = $(ASAN_OTHER_CPLUSPLUSFLAGS) -fvisibility=hidden;

You seem to have left this setting in the Xcode project file, where I think it takes precedence over this. Either way it’s probably best to just use GCC_SYMBOLS_PRIVATE_EXTERN.

> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:98
> +OTHER_LDFLAGS = $(ASAN_OTHER_LDFLAGS) $(ANGLE_OTHER_LDFLAGS) -fvisibility=hidden;

Similar comment here.

> Source/WebCore/Configurations/WebCore.xcconfig:97
> +WK_ANGLE_LDFLAGS = -weak-lANGLE;

What guarantees that symbols from ANGLE aren’t dereferenced at runtime when it’s not present?

[Dean Jackson]

Created attachment 384530 [details]
Patch

[Dean Jackson]

Comment on attachment 384527 [details]
Patch

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

>> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:79
>> +// We set up the values for each variant here, and have the Debug configuration in the Xcode project use the _debug variant.
> 
> Is this comment true? I don’t see anything in the Xcode project that defines DEBUG_DEFINES, STRIP_INSTALLED_PRODUCT, or DEAD_CODE_STRIPPING, and I don’t think the Debug configuration builds the “debug” variant (that is, I believe it builds the “normal” variant).

You're right. This was copied from another .xcconfig file - I removed it in the subsequent patch.

>> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:96
>> +OTHER_CFLAGS = $(ASAN_OTHER_CFLAGS) -fvisibility=hidden;
> 
> I think this is better specified with the dedicated build setting GCC_SYMBOLS_PRIVATE_EXTERN.

Done.

>> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:97
>> +OTHER_CPLUSPLUSFLAGS = $(ASAN_OTHER_CPLUSPLUSFLAGS) -fvisibility=hidden;
> 
> You seem to have left this setting in the Xcode project file, where I think it takes precedence over this. Either way it’s probably best to just use GCC_SYMBOLS_PRIVATE_EXTERN.

Removed from the Xcode project file.

>> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:98
>> +OTHER_LDFLAGS = $(ASAN_OTHER_LDFLAGS) $(ANGLE_OTHER_LDFLAGS) -fvisibility=hidden;
> 
> Similar comment here.

Also done.

>> Source/WebCore/Configurations/WebCore.xcconfig:97
>> +WK_ANGLE_LDFLAGS = -weak-lANGLE;
> 
> What guarantees that symbols from ANGLE aren’t dereferenced at runtime when it’s not present?

Nothing. libwebrtc does the same thing. What should I do? Would you prefer I explicitly dlopen?

[Dean Jackson]

Created attachment 384531 [details]
Patch

(In reply to Dean Jackson from comment #7)
> Comment on attachment 384527 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384527&action=review
> 
> >> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:79
> >> +// We set up the values for each variant here, and have the Debug configuration in the Xcode project use the _debug variant.
> > 
> > Is this comment true? I don’t see anything in the Xcode project that defines DEBUG_DEFINES, STRIP_INSTALLED_PRODUCT, or DEAD_CODE_STRIPPING, and I don’t think the Debug configuration builds the “debug” variant (that is, I believe it builds the “normal” variant).
> 
> You're right. This was copied from another .xcconfig file - I removed it in
> the subsequent patch.

I can’t tell if anything else guarantees that it build with the right values of these build settings for Debug, Release, and Production, but I guess it’s no different than before.

> 
> >> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:96
> >> +OTHER_CFLAGS = $(ASAN_OTHER_CFLAGS) -fvisibility=hidden;
> > 
> > I think this is better specified with the dedicated build setting GCC_SYMBOLS_PRIVATE_EXTERN.
> 
> Done.

Seems like this was already present (just above where you added it).

> 
> >> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:97
> >> +OTHER_CPLUSPLUSFLAGS = $(ASAN_OTHER_CPLUSPLUSFLAGS) -fvisibility=hidden;
> > 
> > You seem to have left this setting in the Xcode project file, where I think it takes precedence over this. Either way it’s probably best to just use GCC_SYMBOLS_PRIVATE_EXTERN.
> 
> Removed from the Xcode project file.
> 
> >> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:98
> >> +OTHER_LDFLAGS = $(ASAN_OTHER_LDFLAGS) $(ANGLE_OTHER_LDFLAGS) -fvisibility=hidden;
> > 
> > Similar comment here.
> 
> Also done.
> 
> >> Source/WebCore/Configurations/WebCore.xcconfig:97
> >> +WK_ANGLE_LDFLAGS = -weak-lANGLE;
> > 
> > What guarantees that symbols from ANGLE aren’t dereferenced at runtime when it’s not present?
> 
> Nothing. libwebrtc does the same thing.

Not quite. In platform/mediastream/libwebrtc/LibWebRTCProviderCocoa.cpp I see LibWebRTCProvider::webRTCAvailable checking whether the library is loaded; presumably all other uses of the library are gated on that function. The question is whether something similar needs to be done with ANGLE.

> Would you prefer I explicitly dlopen?

Absolutely not.

[Dean Jackson]

(In reply to mitz from comment #9)

> > > I think this is better specified with the dedicated build setting GCC_SYMBOLS_PRIVATE_EXTERN.
> > 
> > Done.
> 
> Seems like this was already present (just above where you added it).

Yes.

> > >> +WK_ANGLE_LDFLAGS = -weak-lANGLE;
> > > 
> > > What guarantees that symbols from ANGLE aren’t dereferenced at runtime when it’s not present?
> > 
> > Nothing. libwebrtc does the same thing.
> 
> Not quite. In platform/mediastream/libwebrtc/LibWebRTCProviderCocoa.cpp I
> see LibWebRTCProvider::webRTCAvailable checking whether the library is
> loaded; presumably all other uses of the library are gated on that function.
> The question is whether something similar needs to be done with ANGLE.

I should. Thank you for noticing!

> > Would you prefer I explicitly dlopen?
> 
> Absolutely not.

OK.

[Dean Jackson]

Created attachment 384546 [details]
Patch

[Dean Jackson]

Created attachment 384547 [details]
Patch

[Dean Jackson]

Created attachment 384548 [details]
Patch

[Dean Jackson]

Created attachment 384551 [details]
Patch

[Dean Jackson]

Finally got all the ports building. Also tested with the Apple-internal build system.

[Tim Horton]

Comment on attachment 384551 [details]
Patch

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

> Source/ThirdParty/ANGLE/Configurations/ANGLE.xcconfig:10
> +ANGLE_OTHER_LDFLAGS_iphonesimulator = -framework OpenGLES;

Why no IOSurface here? We use it in the sim now everywhere else.

> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:22
> +CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;

This one seems odd. is this strict selector matching?

> Source/ThirdParty/ANGLE/Configurations/Version.xcconfig:23
> +

Do you need to tell someone this is here so they update it?

[Dean Jackson]

(In reply to Tim Horton from comment #16)
> Comment on attachment 384551 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384551&action=review
> 
> > Source/ThirdParty/ANGLE/Configurations/ANGLE.xcconfig:10
> > +ANGLE_OTHER_LDFLAGS_iphonesimulator = -framework OpenGLES;
> 
> Why no IOSurface here? We use it in the sim now everywhere else.

Unfortunately the sim is missing the one crucial method we need:

[EAGLContext texImageIOSurface:]

> 
> > Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:22
> > +CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> 
> This one seems odd. is this strict selector matching?

I will remove it.

> 
> > Source/ThirdParty/ANGLE/Configurations/Version.xcconfig:23
> > +
> 
> Do you need to tell someone this is here so they update it?

Yeah, I wondered about that - or decided to hope they do it by find&replace.

[Dean Jackson]

Committed r252992: <https://trac.webkit.org/changeset/252992>

[Dean Jackson]

Committed r253824: <https://trac.webkit.org/changeset/253824>