1020661 - WebRTC build error, with GCC 4.8: "scoped_ptr.h:591:18: error: typedef 'type_must_be_complete' locally defined but not used [-Werror=unused-local-typedefs]"

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
Build with GCC 4.8 and this in your mozconfig:
ac_add_options --enable-warnings-as-errors

ACTUAL RESULTS:
{
In file included from /mozilla-central/media/webrtc/trunk/webrtc/system_wrappers/interface/aligned_malloc.h:22:0,
                 from /mozilla-central/media/webrtc/trunk/webrtc/common_video/plane.h:14,
                 from /mozilla-central/media/webrtc/trunk/webrtc/common_video/interface/i420_video_frame.h:18,
                 from /mozilla-central/media/webrtc/trunk/webrtc/video_engine/include/vie_capture.h:22,
                 from ../../dist/include/MediaEngineWebRTC.h:51,
                 from /mozilla-central/dom/media/MediaManager.cpp:49,
                 from /scratch/work/builds/mozilla-central/mozilla-central.12-12-07.08-38/obj/dom/media/Unified_cpp_dom_media0.cpp:15:
/mozilla-central/media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h: In destructor 'webrtc::scoped_array<T>::~scoped_array()':
/mozilla-central/media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h:591:18: error: typedef 'type_must_be_complete' locally defined but not used [-Werror=unused-local-typedefs]
     typedef char type_must_be_complete[sizeof(T)];
                  ^
/mozilla-central/media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h: In member function 'void webrtc::scoped_array<T>::reset(T*)':
/mozilla-central/media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h:596:18: error: typedef 'type_must_be_complete' locally defined but not used [-Werror=unused-local-typedefs]
     typedef char type_must_be_complete[sizeof(T)];
                  ^
}

(Plus more; see bug 1020643. But I'm filing this bug here specifically on the webrtc/[etc]/scoped_ptr.h issue.)

Note that https://code.google.com/p/google-breakpad/issues/detail?id=534 is filed on a similar issue in the scoped_ptr.h version in breakpad a while back. The solution there was to use an updated version of scoped_ptr.h. Not sure if we want that here; we can probably just annotate the typedef as unused or something like that.

Comment 1

[Daniel Holbert [:dholbert]]

To be clear, WebRTC itself is not marked as FAIL_ON_WARNINGS, but this is still treated as an error because this header is included in a .cpp file in dom/media/, and *that* directory's moz.build has FAIL_ON_WARNINGS:
http://mxr.mozilla.org/mozilla-central/source/dom/media/moz.build?rev=50116088c244#58

Comment 2

[Daniel Holbert [:dholbert]]

Randell, do you happen to know if this is already fixed upstream?  (Do these typedefs still exist there?)

Assuming it's not -- do we have a technique for applying local patches to webrtc in a way where they won't be lost the next time we get a new snapshot from upstream? (e.g. for skia, we have a "patches" directory of patches that we've applied to the upstream source. That way, the next time we snapshot the upstream source, we can reapply our patches as-appropriate and not end up re-breaking something that we'd previously fixed.)

Comment 3

[Daniel Holbert [:dholbert]]

Bug 1019050 is disabling these warnings (for now) in dom/media, too, FWIW.

Canceling needinfo; looks like we don't have an upstream fix yet, per comments on that bug.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Randell, do you happen to know if this is already fixed upstream?  (Do these
> typedefs still exist there?)
> 
> Assuming it's not -- do we have a technique for applying local patches to
> webrtc in a way where they won't be lost the next time we get a new snapshot
> from upstream? (e.g. for skia, we have a "patches" directory of patches that
> we've applied to the upstream source. That way, the next time we snapshot
> the upstream source, we can reapply our patches as-appropriate and not end
> up re-breaking something that we'd previously fixed.)

We have a (complex) process, since there are too many patches and we'd have to spend a lot more time mucking with that.  https://wiki.mozilla.org/Media/WebRTC/Updating_Process

It has lots of steps, but basically is backout-and-store our changes to upstream, import new stable, reapply-all-changes, resolve conflicts, fix breakage (i.e. Android), groom patches and land.  Upstream changes a LOT between updates; the final patchset size is typically around 5-15 MB.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: partial fix: suppress -Wunused-local-typedefs *just* in scoped_ptr.h

This isn't a "real" fix, but it's a more correctly-targeted way of suppressing this than what we did over in bug 1020661. In particular:

 (1) This lets us still honor this build warning in other code in dom/media and catch bugs there.

 (2) This suppresses the build warning in this header (scoped_ptr.h), for all #includers, not just those in dom/media. In particular, it makes compiling our webrtc code much quieter.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: suppress warning in scoped_ptr.h, v2

Previous patch included an unrelated chunk, and also caused a clang warning since clang #defines __GNUC__ but doesn't recognize -Wunused-local-typedefs.  So, this version checks for __GNUC__-and-not-clang.

Comment 9

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/177b9f5de1b6

Comment 10

[Jason Duell]

This breaks on my gcc:  (do we care? :) 

  gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5) 

0:15.05 /home/jj/hg/c/t/media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h:120:32: error: unknown option after ‘#pragma GCC diagnostic’ kind [-Werror=pragmas]
 0:15.05 cc1plus: all warnings being treated as errors

Comment 11

[Daniel Holbert [:dholbert]]

Does it fix it if you insert this line...

#pragma GCC diagnostic ignored "-Wpragmas"

...right after "#pragma GCC diagnostic push"?

Comment 12

[Daniel Holbert [:dholbert]]

(and yeah, I think we do care -- if you've been successfully building w/ GCC 4.6.3, you can't be the only person using that particular version, and we shouldn't break such builds just on account of this build-warning-fixing. :)  We can hack around it somehow.)

(I'd (incorrectly) thought that -Wunused-local-typedefs had existed in GCC for multiple versions [though it was only added to -Wall in GCC 4.8].  But it looks like it didn't exist in GCC 4.6, based on comment 10 and also based on http://stackoverflow.com/questions/21997107/how-to-make-cc1plus-error-unrecognized-command-line-option-a-warnings-with . So maybe we should just make the #ifdef a bit more intelligent, to scope this to GCC 4.8 and newer.)

Comment 13

[Jason Duell]

Comment 11 fixes it for me!  Thanks.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: followup: only try to disable the warning for GCC 4.8 and above

jduell, does this fix the bustage for you? (I think it should.)

I copypasted this logic from:
http://mxr.mozilla.org/mozilla-central/source/gfx/angle/src/compiler/preprocessor/ExpressionParser.cpp?rev=72a70862ee28&mark=82-84#82
... with "8" substituted for "7" in the minor version.

(This is only really an issue for >= GCC 4.8, since version 4.8 is when this warning was included in -Wall.)

Comment 15

[Daniel Holbert [:dholbert]]

(This patch is better than comment 11, because it's less heavy-handed.)

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8477499 [details] [diff] [review]
followup: only try to disable the warning for GCC 4.8 and above

(In reply to Daniel Holbert [:dholbert] from comment #14)
> jduell, does this fix the bustage for you? (I think it should.)

I've just verified locally that it fixes the bustage, actually. (Installed GCC 4.6, built, hit the error, applied this patch, and got past it.)

I also verified that this still silences the warning-spam with GCC 4.8.

Comment 17

[Jason Duell]

Yes, the 2nd patch also works.

Comment 18

[Daniel Holbert [:dholbert]]

Thanks, Jason & Randell.

FWIW, the first cset was merged to central on Thursday:
  https://hg.mozilla.org/mozilla-central/rev/177b9f5de1b6

And I've pushed the followup (to make GCC <= 4.6 happy) to inbound:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/3746808d32b1