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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Alex Xu]

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.

Comment 1

[Daniel Holbert [:dholbert]]

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

Comment 2

[Daniel Holbert [:dholbert]]

("just declared as inline" = instead of "static inline")

Comment 3

[Daniel Holbert [:dholbert]]

We might need to just use a @pragma here to disable this particular warning in this file (or in this chunk of the file).

Comment 4

[Daniel Holbert [:dholbert]]

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

Comment 5

[Sebastian Dröge (:slomo)]

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.

Comment 6

[Alex Xu]

Attachment: patch v1

it's only 2 files, actually. I still don't like this solution, though.

Comment 7

[Daniel Holbert [:dholbert]]

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.

Comment 8

[Daniel Holbert [:dholbert]]

[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)?]

Comment 9

[Alex Xu]

Attachment: 0001-Bug-967927-Fix-Clang-build-warning-in-gstreamer.patch

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.

Comment 10

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

Comment 11

[Alex Xu]

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.

Comment 12

[Daniel Holbert [:dholbert]]

(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

Comment 13

[Paul Adenot (:padenot)]

I'm sure edwin will know better.

Comment 14

[Alex Xu]

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

Comment 15

[Daniel Holbert [:dholbert]]

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.

Comment 16

[Alex Xu]

Attachment: patch v2 hg format (including comment)

Comment 17

[Daniel Holbert [:dholbert]]

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

Comment 18

[Alex Xu]

Attachment: patch v3 hg format

patch edited inline, may or may not actually apply

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d22ac63d67f1

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5612a5999be3

https://tbpl.mozilla.org/php/getParsedLog.php?id=35145790&tree=Mozilla-Inbound

Comment 21

[Daniel Holbert [:dholbert]]

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)

Comment 22

[Alex Xu]

Attachment: patch v4 hg format [replacing "pragma clang" with "pragma GCC"]

Murble flurble.

GCC has -Wunused-function, so this should be safe, and probably cleaner than #ifdef.

Comment 23

[Daniel Holbert [: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.)

Comment 24

[Alex Xu]

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

Comment 25

[Daniel Holbert [:dholbert]]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc15ce931273

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cc15ce931273