1345609 - enable -Wformat for libstagefright

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement)

Description

[Tom Tromey :tromey]

After a discussion on irc about a missing warning for an invalid printf
format in libstagefright, we found:

https://dxr.mozilla.org/mozilla-central/source/media/libstagefright/moz.build#164

These lines can be removed now, and then the fallout fixed up.

Comment 1

[Tom Tromey :tromey]

I looked and this seems to be the only remaining moz.build that disables this.

Comment 2

[Tom Tromey :tromey]

Attachment: Bug 1345609 - add -Werror=format to libstagefright;

Review commit: https://reviewboard.mozilla.org/r/118342/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118342/

Comment 3

[Tom Tromey :tromey]

This worked locally but only a try will really tell.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8845124 [details]
Bug 1345609 - add -Werror=format to libstagefright;

https://reviewboard.mozilla.org/r/118342/#review120194

Thank you for this.

Comment 5

[Tom Tromey :tromey]

Something is fishy here because if there are real format string errors in libstagefright,
I would have expected this patch to catch them, but it didn't:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5de0c2e5e3e3a8608508cf7debe23ee41f509d10
How did you find those failures?

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Not sure what you mean...
In the Try you're linking, open a build log and search "warning: format" to find them.
(Or were you expecting them to appear as errors?)

When working on bug 1345671, I just applied your patch locally, and did a build&fix loop until clean.
In my own Try, Stagefright warnings are all gone:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b76ca7bd01713a5c780315a1915597e44098893

Comment 7

[Tom Tromey :tromey]

> (Or were you expecting them to appear as errors?)

Yes, I was.
I was under the impression that try runs were done with warnings as errors,
but looking at the log I see this only seems to be the case for js/.
I think we probably need -Werror=format in build/moz.configure/warnings.configure

Comment 8

[Tom Tromey :tromey]

Doing that found some more errors (on top of bug 1345671) locally.
Pushing this through try again, though, reminds me of why we didn't
do this -- webrtc build problems.

Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=693e1e7ef42cec919c022d981591ef5c31a56417

There's some discussion starting here: https://bugzilla.mozilla.org/show_bug.cgi?id=1060419#c396

Comment 9

[Tom Tromey :tromey]

I am trying the more localized approach of enabling -Werror=format just for
libstagefright.  Presumably though I should go through most of the tree, adding this
to various moz.build files.

Comment 10

[Tom Tromey :tromey]

This one built "ok"; I am hoping those build failures are not my fault.
The logs say:
   MOZ_BUILD_DATE must be provided as an environment var on Taskcluster
... which doesn't mean anything to me.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6150900d1b525e21c589211d5c357d172d297a41

I'll attach the updated patch momentarily.

Comment 11

[Tom Tromey :tromey]

Comment on attachment 8845124 [details]
Bug 1345609 - add -Werror=format to libstagefright;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118342/diff/1-2/

Comment 12

[Tom Tromey :tromey]

Comment on attachment 8845124 [details]
Bug 1345609 - add -Werror=format to libstagefright;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118342/diff/2-3/

Comment 13

[Tom Tromey :tromey]

Comment on attachment 8845124 [details]
Bug 1345609 - add -Werror=format to libstagefright;

I don't remember how to get mozreview to re-request a review,
so I'm tinkering with the flags here.

Comment 14

[Tom Tromey :tromey]

A try push without my patch also shows those build failures.
I will try to either rebase next week or see if there's something else
to do about these.  I wouldn't want to land anything like this without
a clean build.

Comment 15

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8845124 [details]
Bug 1345609 - add -Werror=format to libstagefright;

https://reviewboard.mozilla.org/r/118342/#review121182

Strange that I didn't see these issues. Thank you for fixing them.

Comment 16

[Tom Tromey :tromey]

I'll attach a rebased version in a sec.
Running through try again to verify that the build issues from last week were transient.

Comment 17

[Tom Tromey :tromey]

Looking good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c739867c71e09c8388d2cdef7a2a7e4bab4317b6

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f8c4fb52e61
add -Werror=format to libstagefright; r=gerald

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8f8c4fb52e61