Bug 186615 - Activate -Wexit-time-destructors -and Wglobal-constructors in libwebrtc
Summary: Activate -Wexit-time-destructors -and Wglobal-constructors in libwebrtc
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-13 21:20 PDT by youenn fablet
Modified: 2018-06-15 00:45 PDT (History)
10 users (show)

See Also:


Attachments
Patch (21.48 KB, patch)
2018-06-13 21:27 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (20.26 KB, patch)
2018-06-13 21:46 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (20.32 KB, patch)
2018-06-13 21:59 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (12.23 KB, patch)
2018-06-14 10:50 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (13.82 KB, patch)
2018-06-14 11:38 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ltilve-gtk-wk2-ews for gtk-wk2 (2.72 MB, application/zip)
2018-06-14 12:31 PDT, Igalia-pontevedra EWS
no flags Details
Patch for landing (13.41 KB, patch)
2018-06-14 15:42 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-13 21:20:32 PDT
Activate -Wexit-time-destructors -and Wglobal-constructors in libwebrtc
Comment 1 youenn fablet 2018-06-13 21:27:25 PDT
Created attachment 342722 [details]
Patch
Comment 2 youenn fablet 2018-06-13 21:46:16 PDT
Created attachment 342724 [details]
Patch
Comment 3 youenn fablet 2018-06-13 21:59:19 PDT
Created attachment 342725 [details]
Patch
Comment 4 Darin Adler 2018-06-14 09:18:26 PDT
Comment on attachment 342725 [details]
Patch

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

I don’t understand why there are changes to the project file in the patch.

Patch otherwise looks pretty good; I’m a little concerned that the "never destroyed" idiom is a little awkward to cut and paste 3 or more times, but probably fine.

> Source/ThirdParty/libwebrtc/Source/webrtc/rtc_base/logging.cc:128
> +    RTC_EXCLUSIVE_LOCKS_REQUIRED(logCriticalScope()) {

I don’t understand RTC_EXCLUSIVE_LOCKS_REQUIRED well enough to be sure this is correct.

> Source/ThirdParty/libwebrtc/Source/webrtc/rtc_base/logging.cc:148
> +    streamList();

Why is this needed? Seems like the first caller to streamList will do the right thing without this change.
Comment 5 youenn fablet 2018-06-14 10:50:28 PDT
Created attachment 342742 [details]
Patch
Comment 6 youenn fablet 2018-06-14 11:38:06 PDT
Created attachment 342748 [details]
Patch
Comment 7 Igalia-pontevedra EWS 2018-06-14 12:31:10 PDT
Comment on attachment 342748 [details]
Patch

Attachment 342748 [details] did not pass gtk-wk2-ews (gtk-wk2):
Output: http://webkit-queues.webkit.org/results/8183124

New failing tests:
http/tests/misc/cached-scripts.html
Comment 8 Igalia-pontevedra EWS 2018-06-14 12:31:15 PDT
Created attachment 342754 [details]
Archive of layout-test-results from ltilve-gtk-wk2-ews for gtk-wk2

The attached test failures were seen while running run-webkit-tests on the gtk-wk2-ews.
Bot: ltilve-gtk-wk2-ews  Port: gtk-wk2  Platform: Linux-4.16.0-0.bpo.1-amd64-x86_64-with-debian-9.4
Comment 9 youenn fablet 2018-06-14 12:56:31 PDT
(In reply to Igalia-pontevedra EWS from comment #7)
> Comment on attachment 342748 [details]
> Patch
> 
> Attachment 342748 [details] did not pass gtk-wk2-ews (gtk-wk2):
> Output: http://webkit-queues.webkit.org/results/8183124
> 
> New failing tests:
> http/tests/misc/cached-scripts.html

Nice to see GTK bots actually run tests.
Error is unrelated though since the patch is about WebRTC only.
Comment 10 Darin Adler 2018-06-14 15:38:13 PDT
Comment on attachment 342748 [details]
Patch

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

> Source/ThirdParty/libwebrtc/Source/webrtc/rtc_base/logging.cc:148
> -  std::call_once(callLogCriticalScopeOnce,[] { logCriticalScope(); });
> +  std::call_once(callLogCriticalScopeOnce,[] {
> +    logCriticalScope();
> +  });

Seems like this formatting change is not needed.
Comment 11 youenn fablet 2018-06-14 15:42:35 PDT
Created attachment 342769 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2018-06-14 16:21:12 PDT
Comment on attachment 342769 [details]
Patch for landing

Clearing flags on attachment: 342769

Committed r232858: <https://trac.webkit.org/changeset/232858>
Comment 13 WebKit Commit Bot 2018-06-14 16:21:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2018-06-14 16:22:23 PDT
<rdar://problem/41143599>