Closed Bug 967927 Opened 6 years ago Closed 6 years ago

clang 3.4 build failure with --enable-warnings-as-errors - content/media/gstreamer/GStreamerMozVideoBuffer.cpp:17:1: error: unused function 'gst_moz_video_buffer_get_instance_private' [-Werror,-Wunused-function]

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: alex_y_xu, Assigned: alex_y_xu)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

1:54.47 /home/alex/gecko-dev/content/media/gstreamer/GStreamerMozVideoBuffer.cpp:17:1: error: unused function 'gst_moz_video_buffer_get_instance_private' [-Werror,-Wunused-function]
 1:54.47 G_DEFINE_TYPE(GstMozVideoBuffer, gst_moz_video_buffer, GST_TYPE_BUFFER);
 1:54.47 ^
 1:54.47 /usr/include/glib-2.0/gobject/gtype.h:1327:43: note: expanded from macro 'G_DEFINE_TYPE'
 1:54.47 #define G_DEFINE_TYPE(TN, t_n, T_P)                         G_DEFINE_TYPE_EXTENDED (TN, t_n, T_P, 0, {})
 1:54.47                                                             ^
 1:54.47 /usr/include/glib-2.0/gobject/gtype.h:1467:60: note: expanded from macro 'G_DEFINE_TYPE_EXTENDED'
 1:54.48 #define G_DEFINE_TYPE_EXTENDED(TN, t_n, T_P, _f_, _C_)      _G_DEFINE_TYPE_EXTENDED_BEGIN (TN, t_n, T_P, _f_) {_C_;} _G_DEFINE_TYPE_EXTENDED_END()
 1:54.48                                                             ^
 1:54.48 /usr/include/glib-2.0/gobject/gtype.h:1677:24: note: expanded from macro '_G_DEFINE_TYPE_EXTENDED_BEGIN'
 1:54.48 static inline gpointer \
 1:54.48                        ^
 1:54.48 <scratch space>:88:1: note: expanded from here
 1:54.48 gst_moz_video_buffer_get_instance_private
 1:54.48 ^
 1:55.43 1 error generated.

I am unsure how to fix this. Simply deleting the line results in an "undefined" error elsewhere.
Blocks: buildwarning
(FWIW, the clang-3.4 snapshot on my Ubuntu 13.10 system doesn't warn about this. So this is specific to newish versions of clang.)

Looks like G_DEFINE_TYPE defines a bunch of things, some of which we use, some of which we don't.  (and this is one of the things we don't use)

I'm guessing that clang [past a certain version] is warning about this particular unused function because it's declared as "static", which makes it private to this .cpp file, which makes it obviously-useless when it's unused.  If it were just declared as "inline", I'll bet things would be fine. (But we can't really do anything about that, because the macro is defined in a system header.)

As a temporary workaround, you should be able to use
  ac_add_options --disable-gstreamer
to disable this code.
("just declared as inline" = instead of "static inline")
We might need to just use a @pragma here to disable this particular warning in this file (or in this chunk of the file).
(er, make that "#pragma", not "@pragma")

Alex, if you're interested in tackling this, I'll bet you can just use
 #pragma clang diagnostic ignored "-Wunused-function"
(along with push/pop) to wrap this line of code -- see code sample at the bottom of this post:
http://useyourloaf.com/blog/2011/09/20/disabling-clang-compiler-warnings.html

If you could to give that a shot and verify that it fixes the issue for you, that would be great!
Reported this to the GLib folks here: https://bugzilla.gnome.org/show_bug.cgi?id=723899

I think as a temporary workaround for Mozilla it would make sense to use -Wno-unused-function on all the GObject-related code.
Attached patch patch v1 (obsolete) — Splinter Review
it's only 2 files, actually. I still don't like this solution, though.
Attachment #8372881 - Flags: feedback?(dholbert)
Attachment #8372881 - Attachment is patch: true
Comment on attachment 8372881 [details] [diff] [review]
patch v1

Yup, this is what I had in mind. Thanks!

(In reply to Alex Xu from comment #6)
> I still don't like this solution, though.
It's not beautiful, but I don't think there's much else we can do for an unused-function-defined-in-a-system-header-macro. (aside from "using" the function to silence the warning -- but that'd be hackier)

Probably needs r+ from someone who's written/reviewed code in these files before it can land, though.
Attachment #8372881 - Flags: feedback?(dholbert) → feedback+
[I verified that I hit this bug with clang trunk, and that the patch fixes it.]
[Alex: I'm assigning this to you, since you've posted a patch. Assuming you're OK with the patch's approach, could you post an updated version, including a commit message, and request review (perhaps from padenot)?]
Assignee: nobody → alex_y_xu
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(alex_y_xu)
the git transfer doesn't seem to store the committer, so I'm not sure who to ask for review.

I'll take your word for it though.
Attachment #8372881 - Attachment is obsolete: true
Attachment #8375266 - Flags: review?(paul)
Flags: needinfo?(alex_y_xu)
Note that this is a git-format-patch, not an hg import patch. moz-git-tools requires python[sqlite], which requires a rebuild, which is scheduled for tomorrow.
I feel like there should be a comment to the upstream bug, seeing as it looks like it should be resolved soon.

I don't know how long it will take to pull in those changes though.
(In reply to Alex Xu from comment #10)
> Note that this is a git-format-patch, not an hg import patch

(FWIW, hg can import / qimport the patch you attached [including its header info] just fine -- I just confirmed locally.)


(In reply to Alex Xu from comment #11)
> it looks like [the upstream bug] should be resolved soon.
> 
> I don't know how long it will take to pull in those changes though.

I believe this macro is defined in a system header[1], not in a header anywhere in our tree.

So there's not anything we can pull in from upstream to fix this. It's more a question of how long it takes for a gtk development package [2] update to be shipped with this fix to all affected / potentially-affected mozilla developers.  (Not sure if/when that will happen, as I'm not familiar with the GTK release cycle, and/or how dilligently the various distributions pull in updates.)

[1] looks like /usr/include/glib-2.0/gobject/gtype.h on my system
[2] looks like "libglib2.0-dev" on my system
Attachment #8375266 - Flags: review?(paul) → review?(edwin)
I'm sure edwin will know better.
(In reply to Daniel Holbert [:dholbert] from comment #12)
> (In reply to Alex Xu from comment #11)
> > it looks like [the upstream bug] should be resolved soon.
> > 
> > I don't know how long it will take to pull in those changes though.
> 
> I believe this macro is defined in a system header[1], not in a header
> anywhere in our tree.
> 
> So there's not anything we can pull in from upstream to fix this. It's more
> a question of how long it takes for a gtk development package [2] update to
> be shipped with this fix to all affected / potentially-affected mozilla
> developers.  (Not sure if/when that will happen, as I'm not familiar with
> the GTK release cycle, and/or how dilligently the various distributions pull
> in updates.)

Actually, regardless of that, should a comment be added anyways? Is that common for upstream workarounds in Mozilla tree?
You mean, a code-comment next to the #pragma, mentioning that we're working around a GTK bug? That seems like it'd be handy, yeah.
Attachment #8375266 - Attachment is obsolete: true
Attachment #8380116 - Flags: review?(edwin)
Comment on attachment 8380116 [details] [diff] [review]
patch v2 hg format (including comment)

>+++ b/content/media/gstreamer/GStreamerAllocator.cpp
>@@ -46,8 +46,14 @@ typedef struct
>   GstVideoBufferPool pool;
> } MozGfxBufferPool;
> 
>+/* working around https://bugzilla.mozilla.org/show_bug.cgi?id=967927
>+ * which is for upstream bug https://bugzilla.gnome.org/show_bug.cgi?id=723899
>+ */

nit: the term "upstream" in this comment might be slightly misleading/confusing.  Per comment 12, the GTK code isn't really "upstream", because we're not importing / pulling updates from it.  Instead, individual developers have their own snapshots of the buggy GTK header, from their "devel" packages.

Might be better to call it "external" instead of "upstream".
Attached patch patch v3 hg format (obsolete) — Splinter Review
patch edited inline, may or may not actually apply
Attachment #8380116 - Attachment is obsolete: true
Attachment #8380116 - Flags: review?(edwin)
Attachment #8380336 - Flags: review?(edwin)
Keywords: checkin-needed
Comment on attachment 8380336 [details] [diff] [review]
patch v3 hg format

Ah, yes - GCC will complain about this, because it doesn't understand the #pragma.

We need to wrap this...
>+// working around GTK+ bug https://bugzilla.gnome.org/show_bug.cgi?id=723899
>+#pragma clang diagnostic push
>+#pragma clang diagnostic ignored "-Wunused-function"

...in "#ifdef __clang__" ... "#endif".

Same for this:
>+#pragma clang diagnostic pop

(in both files)
Murble flurble.

GCC has -Wunused-function, so this should be safe, and probably cleaner than #ifdef.
Attachment #8380336 - Attachment is obsolete: true
Attachment #8380771 - Flags: review?(edwin)
Attachment #8380771 - Flags: feedback?(dholbert)
Comment on attachment 8380771 [details] [diff] [review]
patch v4 hg format [replacing "pragma clang" with "pragma GCC"]

Looks reasonable to me.

(Personally, I'd still marginally prefer the #ifdef __clang__ solution as being more targeted, even though it's more verbose, but I don't prefer it strongly enough to turn this version down.)
Attachment #8380771 - Flags: feedback?(dholbert) → feedback+
Attachment #8380771 - Attachment description: patch v4 hg format → patch v4 hg format [replacing "pragma clang" with "pragma GCC"]
(In reply to Daniel Holbert [:dholbert] from comment #23)
> Comment on attachment 8380771 [details] [diff] [review]
> patch v4 hg format [replacing "pragma clang" with "pragma GCC"]
> 
> Looks reasonable to me.
> 
> (Personally, I'd still marginally prefer the #ifdef __clang__ solution as
> being more targeted, even though it's more verbose, but I don't prefer it
> strongly enough to turn this version down.)

What happens when GCC gets this diagnostic too? :)

It is a legitimate warning; there is a function declared that is not used.
Keywords: checkin-needed
(In reply to Alex Xu from comment #24)
> What happens when GCC gets this diagnostic too? :)

(GCC has the diagnostic; it just [currently] doesn't warn about 'inline' functions that are unused (even 'static inline'), which IMHO seems reasonable. But I suppose it's conceivable that it might eventually switch to doing so, like clang apparently has.)
https://hg.mozilla.org/mozilla-central/rev/cc15ce931273
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.