Bug 204708 - Build ANGLE as a dynamic library
Summary: Build ANGLE as a dynamic library
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-29 13:47 PST by Dean Jackson
Modified: 2019-12-02 10:36 PST (History)
10 users (show)

See Also:


Attachments
Patch (44.84 KB, patch)
2019-11-29 14:02 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (47.64 KB, patch)
2019-11-29 14:25 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (49.18 KB, patch)
2019-11-29 16:05 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (41.15 KB, patch)
2019-11-29 16:15 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (54.09 KB, patch)
2019-11-30 12:14 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (54.09 KB, patch)
2019-11-30 12:40 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (54.09 KB, patch)
2019-11-30 14:14 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (54.26 KB, patch)
2019-11-30 15:03 PST, Dean Jackson
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2019-11-29 13:47:35 PST
Build ANGLE as a dynamic library
Comment 1 Dean Jackson 2019-11-29 13:48:46 PST
rdar://57349384
Comment 2 Dean Jackson 2019-11-29 14:02:50 PST
Created attachment 384524 [details]
Patch
Comment 3 Build Bot 2019-11-29 14:03:34 PST
Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE
Comment 4 Dean Jackson 2019-11-29 14:25:33 PST
Created attachment 384527 [details]
Patch
Comment 5 mitz 2019-11-29 15:06:52 PST
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?
Comment 6 Dean Jackson 2019-11-29 16:05:56 PST
Created attachment 384530 [details]
Patch
Comment 7 Dean Jackson 2019-11-29 16:09:37 PST
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?
Comment 8 Dean Jackson 2019-11-29 16:15:10 PST
Created attachment 384531 [details]
Patch
Comment 9 mitz 2019-11-29 16:15:52 PST
(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.
Comment 10 Dean Jackson 2019-11-29 16:44:21 PST
(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.
Comment 11 Dean Jackson 2019-11-30 12:14:23 PST
Created attachment 384546 [details]
Patch
Comment 12 Dean Jackson 2019-11-30 12:40:01 PST
Created attachment 384547 [details]
Patch
Comment 13 Dean Jackson 2019-11-30 14:14:10 PST
Created attachment 384548 [details]
Patch
Comment 14 Dean Jackson 2019-11-30 15:03:32 PST
Created attachment 384551 [details]
Patch
Comment 15 Dean Jackson 2019-11-30 15:37:07 PST
Finally got all the ports building. Also tested with the Apple-internal build system.
Comment 16 Tim Horton 2019-12-02 10:00:15 PST
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?
Comment 17 Dean Jackson 2019-12-02 10:24:18 PST
(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.
Comment 18 Dean Jackson 2019-12-02 10:36:21 PST
Committed r252992: <https://trac.webkit.org/changeset/252992>