Bug 186570 – Eliminate static initializers in libwebrtc.dylib

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Eliminate static initializers in libwebrtc.dylib

[Radar WebKit Bug Importer]

<rdar://problem/41054874>

[youenn fablet]

Created attachment 342615 [details]
Patch

[youenn fablet]

Created attachment 342619 [details]
Patch

[youenn fablet]

Created attachment 342620 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[youenn fablet]

Created attachment 342678 [details]
Patch

[youenn fablet]

(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

[Darin Adler]

(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.

[youenn fablet]

> 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.

[WebKit Commit Bot]

Comment on attachment 342678 [details]
Patch

Clearing flags on attachment: 342678

Committed r232827: <https://trac.webkit.org/changeset/232827>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

There are still static initializers for:

    LogMessage::streams_ in logging.cc
    NonlinearBeamformer::kHalfBeamWidthRadians in nonlinear_beamformer.cc
    PI in virtualsocketserver.cc

[youenn fablet]

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

[Darin Adler]

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.

[Darin Adler]

(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?

[youenn fablet]

(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.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 342738 [details]
Fix memory error issue

[WebKit Commit Bot]

Comment on attachment 342738 [details]
Fix memory error issue

Clearing flags on attachment: 342738

Committed r232846: <https://trac.webkit.org/changeset/232846>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.