Closed Bug 1076698 Opened 10 years ago Closed 10 years ago

Treat as errors some individual gcc/clang warnings that are not reported in the tree

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox34 wontfix, firefox35 fixed)

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- wontfix
firefox35 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Depends on 1 open bug)

Details

Attachments

(5 files)

Some files have no warnings and some warnings are reported no files.

We prevent warning regressions in warning-free directories with FAIL_ON_WARNINGS, but we don't systematically prevent regressions of individual warnings that are not reported anywhere in the tree. This patch treats some of those warnings as errors in all directories. Here is a green try build:

https://tbpl.mozilla.org/?tree=Try&rev=92c4981190d4

Those warnings are treated as errors regardless of FAIL_ON_WARNINGS or mozconfig --enable-warnings-as-errors. Is that too aggressive? This patch targets individual warnings that should not change with new compiler releases, whereas gcc -Wall may acquire new warnings in future gcc versions..

I started with some mostly uncontroversial warnings, common to both gcc 4.4 [1] and clang [2], that are not reported in the tree:

[1] http://gcc.gnu.org/onlinedocs/gcc-4.4.0/gcc/Warning-Options.html
[2] http://fuckingclangwarnings.com/

 -Waddress - catches suspicious uses of memory addresses
 -Wattributes - catches misuses of __attribute__
 -Wchar-subscripts - catches array index using signed char
 -Wcomment - catches nested comments
 -Wendif-labels - catches `#else FOO` and `#endif FOO` not in comment
 -Wenum-compare - catches comparison of different enum types
 -Wignored-qualifiers - catches returns types with qualifiers like const
 -Wimplicit-function-declaration - catches missing C function prototypes
 -Wimplicit-int - catches C variable declaration without a type
 -Wmissing-braces - catches aggregate initializers missing nested braces
 -Wmultichar - catches multicharacter integer constants like 'THIS'
 -Wnonnull - catches NULL used with functions arguments marked as non-null
 -Wparentheses - catches `if (a=b)` and operator precedence bugs
 -Wpointer-sign - catches mixing pointers to signed and unsigned types
 -Wpointer-to-int-cast - catches casts from pointer to different sized int
 -Wreorder - catches ctor initializer list not matching class definition order
 -Wsequence-point - catches undefined order behavior like `a = a++`
 -Wswitch - catches switches without all enum cases or default case
 -Wtrigraphs - catches unlikely use of trigraphs
 -Wunknown-pragmas - catches unexpected #pragma directives
 -Wunused-label - catches unused goto labels
 -Wunused-value - catches unused expression results
 -Wwrite-strings - catches non-const char* pointers to string literals

Since Gecko and SpiderMonkey have separate configure.in files, each with separate CFLAGS and CXXFLAGS, I tuned the warnings-as-errors for each set of files separately. I have another patch that adds a looong list clang-specific warnings, but I wanted to start small. In bug 822978 comment 3, Ted suggested that we might want to move the growing list of warning macros to compiler-opts.m4, but I have not done that here.
Remove unnecessary MOZ_*_SUPPORTS_WARNING checks for warnings supported by gcc 4.4+ and clang.
Attachment #8498674 - Flags: review?(ted)
Part 2: Treat some individual gcc/clang warnings as errors in all directories.
Attachment #8498676 - Flags: review?(ted)
(In reply to Chris Peterson (:cpeterson) from comment #1)
> Created attachment 8498674 [details] [diff] [review]
> part-1-remove-unnecessary-MOZ_SUPPORTS_WARNING-checks.patch
> 
> Remove unnecessary MOZ_*_SUPPORTS_WARNING checks for warnings supported by
> gcc 4.4+ and clang.

This patch does more than advertized. It also changes the list of warnings set.
Comment on attachment 8498674 [details] [diff] [review]
part-1-remove-unnecessary-MOZ_SUPPORTS_WARNING-checks.patch

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

::: js/src/configure.in
@@ +1176,2 @@
>      #
> +    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wall -Wdeclaration-after-statement -Wpointer-arith -Wsign-compare -Wtype-limits -Werror=int-to-pointer-cast -Werror=return-type"

One flag per line would be better.
Build command lines are going to be very long :(
clang -Weverything, unlike -Wall, enables *all* warnings. The nuclear option for compiler warnings is clang -Werror=everything and opt-out of individual warnings that need to be fixed. This approach is untenable because new clang warnings are added all the time. For future reference, this attachment is a list of all -Weverything warnings that must be disabled to build mozilla-central today with zarro warnings.

clang (purposely?) does not document its warning flags, but an unofficial generated reference is available at http://fuckingclangwarnings.com.
(In reply to Mike Hommey [:glandium] from comment #3)
> > Remove unnecessary MOZ_*_SUPPORTS_WARNING checks for warnings supported by
> > gcc 4.4+ and clang.
> 
> This patch does more than advertized. It also changes the list of warnings
> set.

I forgot to mention that I alphabetized the lists of warnings flags and comments (and moved the -Werror flags to the end of the FLAGS).

But I do see that I accidentally dropped -Wempty-body from _WARNINGS_CFLAGS (in this intermediate patch).

(In reply to Mike Hommey [:glandium] from comment #5)
> Build command lines are going to be very long :(

Unfortunately, yes. Maybe we could move all these CFLAGS to a gcc options @file?
Comment on attachment 8498676 [details] [diff] [review]
part-2-treat-some-warnings-as-errors.patch

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

These lists are definitely getting harder to read. Also, I would *really* like to have these moved into a shared location at some point, they're geting tediously long.
Attachment #8498676 - Flags: review?(ted) → review+
Attachment #8498674 - Flags: review?(ted) → review+
(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 8498674 [details] [diff] [review]
> part-1-remove-unnecessary-MOZ_SUPPORTS_WARNING-checks.patch
> 
> Review of attachment 8498674 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/configure.in
> @@ +1176,2 @@
> >      #
> > +    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wall -Wdeclaration-after-statement -Wpointer-arith -Wsign-compare -Wtype-limits -Werror=int-to-pointer-cast -Werror=return-type"
> 
> One flag per line would be better.

Mike, I agree that this one long line of flags is hard to read or diff. When you say one flag per line would be better, do you mean standalone lines?

    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wall"
    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wdeclaration-after-statement"
    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wpointer-arith"
    ...

or just adding linebreaks in the string?

    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wall
                                          -Wdeclaration-after-statement
                                          -Wpointer-arith"
Flags: needinfo?(mh+mozilla)
The former.
Flags: needinfo?(mh+mozilla)
Daniel: do you mind testing this patch with clang on Linux? This build configuration is not on TBPL, but I'd like to avoid breaking it. :) Both debug and release builds would need to be tested because they can report different warnings.

https://tbpl.mozilla.org/?tree=Try&rev=e157f2a8ebfe
Attachment #8500231 - Flags: feedback?(dholbert)
Sure!

One caveat: I'm using clang 3.5 on Ubuntu 14.10 (beta), which is kind of bleeding-edge-ish. In particular, it adds warning "-Wtautological-pointer-compare", which warns about e.g. "this == NULL".

I wouldn't expect that to matter for your patch, since you don't explicitly call out that warning in your list; but it does somehow, because when I build with your patch,  I get this build error:
{
mozilla/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:4199:14: error: comparison of array 'msg->g_call_id' not equal to a null pointer is always true [-Werror,-Wtautological-pointer-compare]
 3:16.43     if (msg->g_call_id != NULL) {
 3:16.43         ~~~~~^~~~~~~~~    ~~~~
}

I don't get that error without your patch -- I complete the build successfully.

I'm building with this mozconfig:
{
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/../obj
ac_add_options --enable-debug --disable-optimize
}
and I clobbered before building, for sanity's sake.
If I remove this line from the patch...
>    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Werror=address"
...then I get past that error from comment 12.  So, maybe remove that from the patch for now? (& spin it into a followup, if you'd really like to get it in eventually -- if you do, I'll file a bug on that fsmdef warning, though it might take a little while to fix since it's in code from upstream.)

Haven't completed a build yet, but it's looking promising.  I'll post more results tomorrow.
Yup, my debug build completes successfully if I remove that Werror=address line. So, please remove that before landing.

As for opt: I actually can't complete an opt build with clang 3.5 on Ubuntu 14.10; I filed bug 1078370 on that. But after downgrading to clang 3.4, I can successfully compile an opt build both with & without your patch here.
Attachment #8500231 - Flags: feedback?(dholbert) → feedback+
Depends on: 1078430
It looks like this broke builds that use jemalloc3, unfortunately.

The problem is that jemalloc3's configure script repeatedly attempts to compile code that triggers -Wimplicit-int on OSX, expecting at least one to succeed [1]. All of the attempts fail due to the warning being treated as an error, breaking the build process.

Is there a way to disable these warnings for external projects? Or perhaps we should just remove -Wimplicit-int for now, and upstream a patch that fixes the configure script?

1- http://dxr.mozilla.org/mozilla-central/source/memory/jemalloc/src/configure.ac#1333
Flags: needinfo?(cpeterson)
Presumably the problem goes away if you replace "static foo" with "static int foo", in that line of configure.ac?  (Or maybe there's a better fix?)

> Or perhaps we should just remove -Wimplicit-int for now,
> and upstream a patch that fixes the configure script?

This is the quickest way forward, IMO.  (This, or taking a local patch to fix configure.ac & making sure the patch gets upstreamed.) I'm not sure there's a sane way to disable these for 3rd-party code directories, in general, since these directories integrate into our build system in a variety of ways (I think).
(In reply to Daniel Holbert [:dholbert] from comment #18)
> Presumably the problem goes away if you replace "static foo" with "static
> int foo", in that line of configure.ac?  (Or maybe there's a better fix?)

Yes, it does. That would be the way to fix it upstream, I believe.
(In reply to Guilherme Gonçalves [:ggp] from comment #17)
> Is there a way to disable these warnings for external projects? Or perhaps
> we should just remove -Wimplicit-int for now, and upstream a patch that
> fixes the configure script?

Sorry. This quick fix removes -Werror=implicit-int from configure.in.

I can post a follow-up patch to add -Wno-error=implicit-int to memory/jemalloc/moz.build instead.
Attachment #8501869 - Flags: review?(ted)
Attachment #8501869 - Flags: feedback?(ggoncalves)
Flags: needinfo?(cpeterson)
Comment on attachment 8501869 [details] [diff] [review]
1076698_part-3-remove-Wimplicit-int.patch

It works for me, thanks!
Attachment #8501869 - Flags: feedback?(ggoncalves) → feedback+
Attachment #8501869 - Flags: review?(ted) → review+
Depends on: 1080351
Depends on: 1081208
Depends on: 1080851
Depends on: 1090088
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.