1232224 - Streamline setting of compile warnings in configure.in

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nicholas Nethercote [inactive]]

In configure.in warnings are set up like this:

  enable -Wall
  enable -W1      # not part of -Wall
  if MOZ_ENABLE_WARNINGS_AS_ERRORS 
    enable -Werror=2
  fi

The |if| block makes no sense.

- If -W2 is part of -Wall, we don't need to enable -Werror=2 when
  MOZ_ENABLE_WARNINGS_AS_ERRORS is true, because -Werror (or equivalent)
  will do that anyway.

- If -W2 is not part of -Wall, then we should turn it on all the time, not
  only when MOZ_ENABLE_WARNINGS_AS_ERRORS is true.

The simpler and better way to do this would be like so:
    
  enable -Wall
  enable -W1 
  enable -W2      # but only if -W2 is not part of -Wall
    
One consequence of doing it in this streamlined is that we won't have any -Werror-foo switches used in ALLOW_COMPILER_WARNINGS directories, which is a good thing; to do otherwise makes ALLOW_COMPILER_WARNINGS behave unexpectedly.

This problem was introduced in bug 1090088, and glandium even complained about it in one of the patches in that bug that didn't land; see bug 1090088 comment 12.



  https://bugzilla.mozilla.org/show_bug.cgi?id=1090088#c12 but r+'d part 1 in
  that bug, which did something similar.
- anyway, time to fix it

Comment 1

[Nicholas Nethercote [inactive]]

I will also take this opportunity to make the C and C++ warnings as similar as possible, and also to make configure.in and js/src/configure.in identical.

Comment 2

[Chris Peterson [:cpeterson]]

(In reply to Nicholas Nethercote [:njn] from comment #0)
> - If -W2 is part of -Wall, we don't need to enable -Werror=2 when
>   MOZ_ENABLE_WARNINGS_AS_ERRORS is true, because -Werror (or equivalent)
>   will do that anyway.

My goal was to enable -Werror for warnings that were not reported in any directories, including directories that could not be fully FAIL_ON_WARNINGS because they had other warnings. This was a "two-dimensional" attack on warnings:

1. Some directories had no warnings and could be FAIL_ON_WARNINGS.
2. Some warnings were in no directories and could be -Werror in all directories.

I agree that this configure.in code no longer makes sense now that we have enabled warnings-as-errors by default.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Streamline setting of compile warnings in configure.in

The main changes are to the warnings, which are as follows.

- Kept -Wall.

- Part of -Wall or on by default; remove all mentions:
  * -Waddress
  * -Wchar-subscripts
  * -Wcomment
  * -Wconversion-null
  * -Wendif-labels
  * -Wenum-compare
  * -Wimplicit-function-declaration
  * -Wint-to-pointer-cast
  * -Wmissing-braces
  * -Wmultichar
  * -Wnonnull
  * -Wparentheses
  * -Wpointer-sign
  * -Wpointer-to-int-cast (C only)
  * -Wreorder
  * -Wreturn-type
  * -Wsequence-point
  * -Wsign-compare (C++ only)
  * -Wtrigraphs
  * -Wuninitialized
  * -Wunknown-pragmas
  * -Wunused-label
  * -Wunused-value
  * -Wwrite-strings (C++ only)

- Part of -Wextra; kept where present, added where missing:
  * -Wempty-body
  * -Wignored-qualifiers (not added for C++ code; many fixes needed to enable)
  * -Wtype-limits

- Part of -pedantic; kept where present, added where missing:
  * -pointer-arith

- C++ only, kept:
  * -Wno-invalid-offsetof
  * -Woverloaded-virtual

- Clang-only, kept:
  * -Wnon-literal-null-conversion (affected by a clang bug; see the big comment
    in the code)
  * -Wrange-loop-analysis (C++ only)
  * -Wno-unused-local-typedef

- This no longer exists?  I kept it to be safe:
  * -Winline-new-delete


Other changes:

- Some C warnings incorrectly used the CXX macros. Fixes that.

- Moves comments about warnings closer to the lines where they are defined, to
  make it easier to keep the comments consistent with the code.

- Reorders things a little, e.g. so that all enabled warnings are before all
  disabled warnings.

The C and C++ warnings are now very similar, in both configure.in and
js/src/configure.in.

Comment 4

[Nicholas Nethercote [inactive]]

We *really* need to find a way to refactor this code so that these changes don't have to be made in four different places each time. This patch is a precursor to that, because it minimizes the differences between the four places.

Comment 5

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=088223487eb7&selectedJob=14683067

Comment 6

[Chris Peterson [:cpeterson]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> We *really* need to find a way to refactor this code so that these changes
> don't have to be made in four different places each time. This patch is a
> precursor to that, because it minimizes the differences between the four
> places.

I have some WIP patches that consolidate all the warning configure code and move the list of -W flags into a separate text file.

Comment 7

[Nicholas Nethercote [inactive]]

> I have some WIP patches that consolidate all the warning configure code and
> move the list of -W flags into a separate text file.

Can you post them in bug 1233297? The patches here are likely to clash horribly with anything you've done. I was going to do the refactoring via compiler-opts.m4 but if you've already started via a different approach I'm happy to let you continue.

Comment 8

[Chris Peterson [:cpeterson]]

Comment on attachment 8699328 [details] [diff] [review]
Streamline setting of compile warnings in configure.in

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

LGTM!

::: configure.in
@@ +1440,1 @@
>      # -Wtype-limits - catches overflow bugs, few false positives

I think we should remove all these warning comments, but I don't care too much. The comments double the size of the _WARNINGS_CFLAGS code blocks. If someone wants to know more about a warning, they can read the documentation on gcc.gnu.org or fuckingclangwarnings.com.

@@ +1562,5 @@
> +    _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Wno-invalid-offsetof"
> +
> +    # -Wno-inline-new-delete - we inline 'new' and 'delete' in mozalloc
> +    # -Wno-unused-local-typedef - catches unused typedefs, which are commonly used in assertion macros
> +    MOZ_CXX_SUPPORTS_WARNING(-Wno-, inline-new-delete, ac_cxx_has_wno_inline_new_delete)

As you noted, -Winline-new-delete no longer seems to exist. There is no reference to it in the clang mirror on GitHub, yet clang on OS X silently accepts -Winline-new-delete.

https://github.com/llvm-mirror/clang/search?utf8=%E2%9C%93&q=inline-new-delete

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> Try looks good:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=088223487eb7&selectedJob=14683067

Whoops, https://treeherder.mozilla.org/#/jobs?repo=try&revision=c81ff1711bd6 is better because it has all platforms rather than just OS X.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8699328 [details] [diff] [review]
Streamline setting of compile warnings in configure.in

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

One thing this does change is that for directories with ALLOW_COMPILER_WARNINGS, we currently have all the -Werror=... flags that you're removing. It makes sense that we don't keep them, but it's worth mentioning in the commit message.

(Typo in the commit message: -pointer-arith instead of -Wpointer-arith)

::: configure.in
@@ -1448,5 @@
> -    # -Wpointer-sign - catches mixing pointers to signed and unsigned types
> -    # -Wpointer-to-int-cast - catches casts from pointer to different sized int
> -    # -Wreturn-type - catches missing returns, zero false positives
> -    # -Wsequence-point - catches undefined order behavior like `a = a++`
> -    # -Wsign-compare - catches comparison of signed and unsigned types

As you mention in the commit message, -Wsign-compare is in -Wall for C++ only. We were setting it in both WARNINGS_CFLAGS and WARNING_CXXFLAGS, so you're effectively removing it from CFLAGS, which you shouldn't.

@@ -1549,5 @@
> -    # -Wrange-loop-analysis - catches copies during range-based for loops.
> -    # -Wreturn-type - catches missing returns, zero false positives
> -    # -Wsequence-point - catches undefined order behavior like `a = a++`
> -    # -Wsign-compare - catches comparison of signed and unsigned types
> -    # -Wswitch - catches switches without all enum cases or default case

You didn't mention this one in your commit message, but it's part of -Wall.

::: js/src/configure.in
@@ +1199,3 @@
>      # -Wnon-literal-null-conversion - catches expressions used as a null pointer constant
> +    # -Wsometimes-initialized - catches some uninitialized values
> +    MOZ_C_SUPPORTS_WARNING(-W, non-literal-null-conversion, ac_c_has_non_literal_null_conversion)

Might be worth mentioning that this is not a problem somehow in js code, contrary to gecko?

@@ +1268,3 @@
>      # -Wrange-loop-analysis - catches copies during range-based for loops.
> +    # -Wsometimes-initialized - catches some uninitialized values
> +    MOZ_CXX_SUPPORTS_WARNING(-W, non-literal-null-conversion, ac_cxx_has_non_literal_null_conversion)

Likewise

Comment 11

[Nicholas Nethercote [inactive]]

> ::: js/src/configure.in
> @@ +1199,3 @@
> >      # -Wnon-literal-null-conversion - catches expressions used as a null pointer constant
> > +    # -Wsometimes-initialized - catches some uninitialized values
> > +    MOZ_C_SUPPORTS_WARNING(-W, non-literal-null-conversion, ac_c_has_non_literal_null_conversion)
> 
> Might be worth mentioning that this is not a problem somehow in js code,
> contrary to gecko?

I'll just put the workaround into js/src/configure.in too. That will make factoring out this code easier.

Comment 12

[Nicholas Nethercote [inactive]]

> I think we should remove all these warning comments, but I don't care too much.

I ended up keeping them. It's useful to have explanations for the ones added beyond -Wall, and the ones turned off from -Wall. And once the duplication is factored out there won't be so much repetition.

Comment 13

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/470ab2722530f603fde64de3554cbf16b39787db
Bug 1232224 - Streamline setting of compile warnings in configure.in. r=glandium,cpeterson.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/470ab2722530