1275098 - AudioConverter.cpp is going to have Werror permafail when Gecko 49 merges to Beta on clang-based builds

Status: RESOLVED FIXED

(Core :: Audio/Video: cubeb, defect, P1)

Description

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Build bustage when Gecko 49 merges to Beta.

Only appears to affect clang-based builds, FWIW. Looks like bug 1264199 is the only thing to touch this code recently.

https://treeherder.mozilla.org/logviewer.html#?job_id=20971006&repo=try#L12628

10:28:55     INFO -  In file included from /builds/slave/try-m64-0000000000000000000000/build/src/obj-firefox/x86_64/dom/media/Unified_cpp_dom_media0.cpp:47:
10:28:55     INFO -  /builds/slave/try-m64-0000000000000000000000/build/src/dom/media/AudioConverter.cpp:260:14: error: variable 'error' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
10:28:55     INFO -    } else if (mOut.Format() == AudioConfig::FORMAT_S16) {
10:28:55     INFO -               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
10:28:55     INFO -  /builds/slave/try-m64-0000000000000000000000/build/src/dom/media/AudioConverter.cpp:270:7: note: uninitialized use occurs here
10:28:55     INFO -    if (error != RESAMPLER_ERR_SUCCESS) {
10:28:55     INFO -        ^~~~~
10:28:55     INFO -  /builds/slave/try-m64-0000000000000000000000/build/src/dom/media/AudioConverter.cpp:260:10: note: remove the 'if' if its condition is always true
10:28:55     INFO -    } else if (mOut.Format() == AudioConfig::FORMAT_S16) {
10:28:55     INFO -           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
10:28:55     INFO -  /builds/slave/try-m64-0000000000000000000000/build/src/dom/media/AudioConverter.cpp:253:12: note: initialize the variable 'error' to silence this warning
10:28:55     INFO -    int error;
10:28:55     INFO -             ^
10:28:55     INFO -              = 0
10:28:55     INFO -  1 error generated.

Comment 1

[Maire Reavy [:mreavy]]

Jean-Yves -- If you're not the right owner for this or if you think you can't get to this right away, please needinfo me to find another owner.  I want to get this resolved quickly.  Thanks!

Comment 2

[Jean-Yves Avenard [:jya]]

this is a false positive though..

mOut.Format() can *only* be AudioConfig::FORMAT_FLT on desktop or AudioConfig::FORMAT_S16 on Android...

There can be no other way...
To the point where the only case where error would be used "uninitialised" on the line above we have

"MOZ_DIAGNOSTIC_ASSERT(false, "Unsupported data type");"

it *will* crash...

What about just not making invalid warning as fatal error in the first place ? grmmmbbbll

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1275098: Silence incorrect dumb compiler warning. r?cpearce

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

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8755652 [details]
MozReview Request: Bug 1275098: Silence incorrect dumb compiler warning. r?cpearce

https://reviewboard.mozilla.org/r/54714/#review51374

Comment 5

[Pulsebot]

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

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

[Marcia Knous [:marcia]]

Tracking for 49 for potential build bustage.