1001975 - (Wuninitialized) Allow clang and gcc's -Wuninitialized warnings to be treated as errors in FAIL_ON_WARNINGS directories

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: Wuninitialized.patch

gcc's -Wuninitialized and -Wmaybe-uninitialized warnings were excluded from warnings-as-errors builds in bug 716541 and bug 903513, respectively. This patch reverts bug 716541, allowing -Wuninitialized warnings to be treated as errors in FAIL_ON_WARNINGS directories.

gcc's -Wuninitialized has a well-deserved reputation for reporting false positives. However, recent releases of gcc (since 2011 [1]) have separated -Wuninitialized's less reliable checks into a new -Wmaybe-uninitialized warning. -Wuninitialized warnings are rarely false positives these days.

mozilla-central hit zero -Wuninitialized warnings a few months ago (as reported by clang for OS X and gcc 4.6 and 4.7 for Android). Since then, I've been watching for -Wuninitialized regressions. There have about ten regressions and, AFAICT, only one was a false positive (and it was a tricky case that only affected gcc 4.6 where a member variable that was initialized in one function and read in another). Given the severity of uninitialized variable bugs and gcc's much improved -Wuninitialized checks, I suggest that we remove configure.in's -Wno-error=uninitialized so we can treat -Wuninitialized like other warnings-as-errors. (I tested -Werror=uninitialized, but clang fails with some seeming unrelated errors about ambiguous operator!= overloads.)

The majority of Gecko's -Wmaybe-uninitialized warnings are false positives where an out-parameter is not set when a function returns an error (even if the caller handles the error return correctly). Personally, I'm in favor of disabling -Wmaybe-uninitialized, but that is a different debate and this patch does not change configure.in's disabling of -Wmaybe-uninitialized.

[1] http://gcc.gnu.org/ml/gcc/2013-07/msg00177.html

Green Try build with -Wuninitialized:
https://tbpl.mozilla.org/?tree=Try&rev=e6e9afdc0460

Comment 1

[Mike Hommey [:glandium]]

Comment on attachment 8413327 [details] [diff] [review]
Wuninitialized.patch

Review of attachment 8413327 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +6838,5 @@
>      # -Wmaybe-uninitialized - too many false positives
>      # -Wdeprecated-declarations - we don't want our builds held hostage when a
>      #   platform-specific API becomes deprecated.
> +    MOZ_C_SUPPORTS_WARNING(-W, uninitialized, ac_c_has_uninitialized)
> +    MOZ_CXX_SUPPORTS_WARNING(-W, uninitialized, ac_cxx_has_uninitialized)

That sounds dangerous, as gcc 4.4 doesn't support maybe-uninitialized, and we're building with gcc 4.4 on b2g. Sure, we're not building b2g with --enable-warnings-as-errors ttbomk, but we might some day, and surely -Werror=uninitialized is not going to help.

So I'd suggest only enabling -Werror=uninitialized when -Wmaybe-uninitialized is supported.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Mike Hommey [:glandium] from comment #1)
> That sounds dangerous, as gcc 4.4 doesn't support maybe-uninitialized, and
> we're building with gcc 4.4 on b2g. Sure, we're not building b2g with
> --enable-warnings-as-errors ttbomk, but we might some day

Note that we definitely can't build with GCC 4.4 and --enable-warnings-as-errors until bug 915555 is addressed somehow... and I get the feeling from the lack of activity on that bug vs. high level of pain it causes (in terms of buildspew) that it's not easy to fix.

Given that: maybe we should consider GCC 4.4 a special case, instead of trying to fix this in a generic (but complex*) way?  And in the unlikely event that we can get it to build our code with --enable-warnings-as-errors aside from this bug, we could just wrap cpeterson's code here in a "if not GCC 4.4" conditional?

*(I'm calling the generic solution "complex" because we already have a macro for "add this warning flag if the compiler supports it", but we don't have a macro for "add this warning flag if the compiler supports this *other* warning flag". I don't know autoconf, so I don't have a good feeling for how hard that is.)

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Mike Hommey [:glandium] from comment #1)
> That sounds dangerous, as gcc 4.4 doesn't support maybe-uninitialized, and
> we're building with gcc 4.4 on b2g. Sure, we're not building b2g with
> --enable-warnings-as-errors ttbomk, but we might some day, and surely
> -Werror=uninitialized is not going to help.

I imagine B2G would update its toolchain (to a version of gcc that includes -Wmaybe-uninitialized) for performance reasons before they attempt to compile with --enable-warnings-as-errors using gcc 4.4.

> So I'd suggest only enabling -Werror=uninitialized when
> -Wmaybe-uninitialized is supported.

clang does not support -Wmaybe-uninitialized, but I could test on -Wsometimes-uninitialized (a clang-specific warning).

Comment 4

[Chris Peterson [:cpeterson]]

Bug 1144155 bumped our minimum gcc version to 4.7, which is the version that added -Wmaybe-uninitialized to make gcc's -Wuninitialized actually usable. My WIP patches no longer need to check for -Wmaybe-uninitialized with MOZ_C*_SUPPORTS_WARNING.

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: Wuninitialized-v2.patch

Enable -Werror=uninitialized warnings as errors.

Thanks to bug 1144155, gcc 4.7 is now the minimum supported gcc version. gcc 4.7 moved the less reliable -Wuninitialized checks to a new -Wmaybe-uninitialized warning. gcc and clang's -Wuninitialized warnings now have very few false positives. mozilla-central currently has no -Wuninitialized warnings.

clang has a -Wsometimes-uninitialized warning that is similar to gcc's -Wmaybe-uninitialized, but much more reliable. mozilla-central currently has no -Wsometimes-uninitialized warnings.

Green try build:

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

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8587199 [details] [diff] [review]
Wuninitialized-v2.patch

Review of attachment 8587199 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/configure.in
@@ +1239,5 @@
>          _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Werror=unknown-pragmas"
>          _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Werror=write-strings"
>  
>          MOZ_C_SUPPORTS_WARNING(-Werror=, non-literal-null-conversion, ac_c_has_werror_non_literal_null_conversion)
> +        MOZ_C_SUPPORTS_WARNING(-Werror=, sometimes-uninitialized, ac_c_clang_has_sometimes_uninitialized)

I think you can drop the clang in the cache variable name.

Comment 7

[Chris Peterson [:cpeterson]]

Thanks. I removed clang_ from the name, as you requested.

https://hg.mozilla.org/integration/mozilla-inbound/rev/253bcbb38a7c

Comment 8

[Chris Peterson [:cpeterson]]

I had to back this out because it broke Linux PGO build:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4699b65bf6e5

https://treeherder.mozilla.org/logviewer.html#?job_id=8581912&repo=mozilla-inbound

20:23:17 INFO - /builds/slave/m-in-l64-pgo-00000000000000000/build/src/js/src/jit/x86-shared/MoveEmitter-x86-shared.cpp: In member function 'void js::jit::MoveEmitterX86::emitGeneralMove(const js::jit::MoveOperand&, const js::jit::MoveOperand&)':
20:23:17 INFO - /builds/slave/m-in-l64-pgo-00000000000000000/build/src/js/src/jit/x86-shared/MoveEmitter-x86-shared.cpp:198:42: error: '<anonymous>.js::jit::Operand::disp_' may be used uninitialized in this function [-Werror=uninitialized]

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/631f0244af63

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/631f0244af63