Bug 186570 - Eliminate static initializers in libwebrtc.dylib
Summary: Eliminate static initializers in libwebrtc.dylib
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-12 11:33 PDT by youenn fablet
Modified: 2018-06-14 11:04 PDT (History)
7 users (show)

See Also:


Attachments
Patch (8.13 KB, patch)
2018-06-12 17:46 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (8.29 KB, patch)
2018-06-12 19:11 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (8.46 KB, patch)
2018-06-12 19:16 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (8.72 KB, patch)
2018-06-13 11:20 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fix memory error issue (1.69 KB, patch)
2018-06-14 10:22 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-06-12 11:33:37 PDT
Eliminate static initializers in libwebrtc.dylib
Comment 1 Radar WebKit Bug Importer 2018-06-12 11:40:12 PDT
<rdar://problem/41054874>
Comment 2 youenn fablet 2018-06-12 17:46:31 PDT
Created attachment 342615 [details]
Patch
Comment 3 youenn fablet 2018-06-12 19:11:42 PDT
Created attachment 342619 [details]
Patch
Comment 4 youenn fablet 2018-06-12 19:16:14 PDT
Created attachment 342620 [details]
Patch
Comment 5 Darin Adler 2018-06-12 23:34:19 PDT
Comment on attachment 342620 [details]
Patch

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

> Source/ThirdParty/libwebrtc/Source/webrtc/rtc_base/logging.cc:110
> +  static CriticalSection g_log_crit;

It would be nice to eliminate not just static constructors, but also static destructors. Can we do that? Would need something like NeverDestroyed here.
Comment 6 Darin Adler 2018-06-12 23:35:03 PDT
Comment on attachment 342620 [details]
Patch

Should also turn on the compiler option to make it an error if we have any, so we notice if someone accidentally adds one in the future.
Comment 7 youenn fablet 2018-06-13 11:20:33 PDT
Created attachment 342678 [details]
Patch
Comment 8 youenn fablet 2018-06-13 15:33:51 PDT
(In reply to Darin Adler from comment #5)
> Comment on attachment 342620 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342620&action=review
> 
> > Source/ThirdParty/libwebrtc/Source/webrtc/rtc_base/logging.cc:110
> > +  static CriticalSection g_log_crit;
> 
> It would be nice to eliminate not just static constructors, but also static
> destructors. Can we do that? Would need something like NeverDestroyed here.

Done

> Should also turn on the compiler option to make it an error if we have any,
> so we notice if someone accidentally adds one in the future.

Let's do that as a follow-up
Comment 9 Darin Adler 2018-06-13 20:05:41 PDT
(In reply to Darin Adler from comment #6)
> Should also turn on the compiler option to make it an error if we have any,
> so we notice if someone accidentally adds one in the future.

The options are -Wexit-time-destructors -and Wglobal-constructors.

These are both already specified in ThirdParty/libwebrtc/Configurations/Base.xcconfig, but disabled by libwebrtc.xcconfig, which overrides WARNING_CFLAGS with another warning setting without using $(inherited). It seems like that mistake led to us having these without noticing.

It seems that other .xcconfig files in the libwebrtc directories are making the same mistake. Also, there are lots more warnings enabled in WebCore's Base.xconfig and it would be nice to enable more here.
Comment 10 youenn fablet 2018-06-13 20:46:30 PDT
> The options are -Wexit-time-destructors -and Wglobal-constructors.

OK

> Also, there are lots more warnings enabled in WebCore's
> Base.xconfig and it would be nice to enable more here.

Sure.
We want to stay as close as possible to upstream libwebrtc to limit the cost of bumping revision.
If we do that cleaning work once, we need to ensure we will not do that again and again.
Comment 11 WebKit Commit Bot 2018-06-13 21:14:57 PDT
Comment on attachment 342678 [details]
Patch

Clearing flags on attachment: 342678

Committed r232827: <https://trac.webkit.org/changeset/232827>
Comment 12 WebKit Commit Bot 2018-06-13 21:14:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Darin Adler 2018-06-13 22:46:40 PDT
There are still static initializers for:

    LogMessage::streams_ in logging.cc
    NonlinearBeamformer::kHalfBeamWidthRadians in nonlinear_beamformer.cc
    PI in virtualsocketserver.cc
Comment 14 youenn fablet 2018-06-14 08:28:04 PDT
I did some follow-up work yesterday at https://bugs.webkit.org/show_bug.cgi?id=186615 to enable these flags.

It seems this current set of changes is breaking GTK, https://bugs.webkit.org/show_bug.cgi?id=186619
Comment 15 Darin Adler 2018-06-14 09:19:21 PDT
Comment on attachment 342678 [details]
Patch

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

> Source/ThirdParty/libwebrtc/Source/webrtc/rtc_base/flags.h:158
> +    c_type FLAG_##name = (default);                          \

I think that "static" is missing here.
Comment 16 Darin Adler 2018-06-14 09:19:41 PDT
(In reply to youenn fablet from comment #14)
> It seems this current set of changes is breaking GTK,
> https://bugs.webkit.org/show_bug.cgi?id=186619

Maybe because of the "static" thing I commented on above?
Comment 17 youenn fablet 2018-06-14 10:21:01 PDT
(In reply to Darin Adler from comment #16)
> (In reply to youenn fablet from comment #14)
> > It seems this current set of changes is breaking GTK,
> > https://bugs.webkit.org/show_bug.cgi?id=186619
> 
> Maybe because of the "static" thing I commented on above?

Makes sense, I'll fix it asap.
Comment 18 youenn fablet 2018-06-14 10:22:45 PDT
Reopening to attach new patch.
Comment 19 youenn fablet 2018-06-14 10:22:46 PDT
Created attachment 342738 [details]
Fix memory error issue
Comment 20 WebKit Commit Bot 2018-06-14 11:04:40 PDT
Comment on attachment 342738 [details]
Fix memory error issue

Clearing flags on attachment: 342738

Committed r232846: <https://trac.webkit.org/changeset/232846>
Comment 21 WebKit Commit Bot 2018-06-14 11:04:42 PDT
All reviewed patches have been landed.  Closing bug.