Closed Bug 1020661 Opened 6 years ago Closed 5 years ago

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]"

Categories

(Core :: WebRTC, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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
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.)
Flags: needinfo?(rjesup)
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.
Flags: needinfo?(rjesup)
(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.
Duplicate of this bug: 1029325
Duplicate of this bug: 1056264
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.
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.
Attachment #8476174 - Attachment is obsolete: true
Attachment #8476183 - Flags: review?(rjesup)
Attachment #8476183 - Flags: review?(rjesup) → review+
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
Flags: needinfo?(dholbert)
Does it fix it if you insert this line...

#pragma GCC diagnostic ignored "-Wpragmas"

...right after "#pragma GCC diagnostic push"?
Flags: needinfo?(dholbert) → needinfo?(jduell.mcbugs)
(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 11 fixes it for me!  Thanks.
Flags: needinfo?(jduell.mcbugs)
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.)
Attachment #8477499 - Flags: review?(rjesup)
Attachment #8477499 - Flags: feedback?(jduell.mcbugs)
(This patch is better than comment 11, because it's less heavy-handed.)
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.
Attachment #8477499 - Flags: feedback?(jduell.mcbugs)
Yes, the 2nd patch also works.
Attachment #8477499 - Flags: review?(rjesup) → review+
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
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Assignee: nobody → dholbert
Target Milestone: --- → mozilla34
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.