Closed Bug 711895 Opened 13 years ago Closed 12 years ago

Tweak the warning options used for GCC builds

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla14

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(2 files, 6 obsolete files)

patch, v7
35.58 KB, patch
Details | Diff | Splinter Review
interdiff for v7
6.85 KB, patch
khuey
: review+
Details | Diff | Splinter Review
Attached patch patch (obsolete) — — Splinter Review 
This patch modifies things so that the GCC warnings used for C code more closely match those for C++ code, and so the JS configure.in matches the base one.

_WARNINGS_CFLAGS:
- Remove -W (a.k.a. -Wextra) because we don't use it with C++ code and it's
  pretty noisy
- Remove -Wno-unused, unnecessary with -W gone
- Add -Wno-overlength-strings, because that's a truly useless warning
- js/src/configure.in-only:  add -Wdeclaration-after-statement, to match
  configure.in
     
_WARNINGS_CXXFLAGS
- Add -Wno-overlength-strings, because that's a truly useless warning
- Remove -Wsynth, because it's from ancient GCC versions and no longer
  exists

We get a couple of hundred more warnings on Linux64 debug builds, mostly due to Wunused-but-set-variable in C files.  This isn't a great increase since we have over 1000 warnings in the first place.
Attachment #582733 - Flags: review?(ted.mielczarek)
Attachment #582733 - Flags: feedback?(jwalden+bmo)
OS: Linux → All
Hardware: x86_64 → All
There have been a couple of bugs regarding if statements with empty bodies recently. One is bug 709622, but there was also a bug in C++ code that I commented on but can't seem to find. GCC can warn about instances of this in if, else and do-while statements with -Wempty-body, which is included only in -Wextra. Does that seem like a reasonable warning to add while you're here?
Comment on attachment 582733 [details] [diff] [review]
patch

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

I guess my comments apply to both files, since it looks like the changes to each are pretty much identical.

::: configure.in
@@ +1869,1 @@
>      # -Wdeclaration-after-statement - MSVC doesn't like these

Shouldn't we make this -Werror=... then?

@@ +1870,5 @@
> +    #
> +    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wall -Wpointer-arith -Wdeclaration-after-statement"
> +    
> +    # Turn off the following warnings that -Wall/-pedantic turn on:
> +    # -Woverlength-strings - we exceed the minimum maximum lenght all the time

"length"

@@ +1930,5 @@
> +
> +    # Turn off the following warnings that -Wall/-pedantic turn on:
> +    # -Woverlength-strings - we exceed the minimum maximum lenght all the time
> +    # -Wctor-dtor-privacy - ???
> +    # -Wnon-virtual-dtor - ???

This warning detects problems when someone extends a class with fields that have destructors.  I don't think we should turn it off, at least not in new enough compilers that will silence the warning when the class in question is final.  Most classes can be made to work by just adding it, judging by what I saw fixing the first couple dozen for a clang build.  Some can't, and really those places are difficult-to-see bugs waiting to happen.

::: js/src/configure.in
@@ +1870,5 @@
> +
> +    # Turn off the following warnings that -Wall/-pedantic turn on:
> +    # -Woverlength-strings - we exceed the minimum maximum lenght all the time
> +    # -Wctor-dtor-privacy - ???
> +    # -Wnon-virtual-dtor - ???

Same applies there.
Attachment #582733 - Flags: feedback?(jwalden+bmo) → feedback-
> > +    # -Wnon-virtual-dtor - ???
> 
> This warning detects problems when someone extends a class with fields that
> have destructors.  I don't think we should turn it off, at least not in new
> enough compilers that will silence the warning when the class in question is
> final.  Most classes can be made to work by just adding it, judging by what
> I saw fixing the first couple dozen for a clang build.  Some can't, and
> really those places are difficult-to-see bugs waiting to happen.

This warning is already off, my patch didn't change this.  It did add the "???" comment which was my attempt at saying "I don't know why this is off".
> -Wempty-body

Yeah, good idea.
Comment on attachment 582733 [details] [diff] [review]
patch

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

Since the actual configure changes here are trivial, and Waldo clearly cares about the content, I'm happy to delegate the review to him.
Attachment #582733 - Flags: review?(ted.mielczarek)
Attached patch patch v2 (obsolete) — — Splinter Review 
This version:

- Adds -Wempty-body for both C and C++

- Adds -Wtype-limits for both C and C++.  Prior to this bug it was already
  enabled for C because it's part of -Wextra.

- Removes -Wno-non-virtual-dtor for C++.  This has no effect because that
  warning isn't enabled by -Wall or -Wextra or -pedantic.  (It is enabled by
  -Weffc++, not that that is relevant.)

  (I tried turning -Wno-non-virtual-dtor on a while back, it gave bazillions
  of warnings, so I don't want to turn it on now.  AFAICT it gave warnings
  even for classes that didn't have explicit destructors.)

- Fixes a few places where -Wtype-limits and -Wempty-body warnings were
  triggered, esp. those in headers that were repeated many times.  Most of
  them were |x >= 0| comparisons on unsigned ints.
Attachment #582733 - Attachment is obsolete: true
Attachment #583340 - Flags: review?
Attachment #583340 - Flags: review? → review?(jwalden+bmo)
Comment on attachment 583340 [details] [diff] [review]
patch v2

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

yuv_row.h looks like third-party code imported and patched anew each time, via update.sh.  That needs someone else's review; I passed the buck to the likely correct person.

::: configure.in
@@ +1872,5 @@
> +    #
> +    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wall -Wpointer-arith -Wdeclaration-after-statement -Wempty-body -Wtype-limits"
> +    
> +    # Turn off the following warnings that -Wall/-pedantic turn on:
> +    # -Woverlength-strings - we exceed the minimum maximum lenght all the time

"length"

::: gfx/ycbcr/yuv_row.h
@@ +133,5 @@
>  #else
>  #define EMMS() asm("emms")
>  #endif
>  #else
> +#define EMMS() (void)true;

I suspect, given the other definitions, this shouldn't include a semicolon.  And the more canonical version would be ((void) 0), usually.

::: js/src/configure.in
@@ +1815,5 @@
> +    #
> +    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wall -Wpointer-arith -Wdeclaration-after-statement -Wempty-body -Wtype-limits"
> +    
> +    # Turn off the following warnings that -Wall/-pedantic turn on:
> +    # -Woverlength-strings - we exceed the minimum maximum lenght all the time

"length"
Attachment #583340 - Flags: review?(tterribe)
Attachment #583340 - Flags: review?(jwalden+bmo)
Attachment #583340 - Flags: review+
Comment on attachment 583340 [details] [diff] [review]
patch v2

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

Yes, for yuv_row.sh you need to check in a file containing the patch and add a line to update.sh that applies it.
Attachment #583340 - Flags: review?(tterribe) → review-
Attached patch patch v3 (obsolete) — — Splinter Review 
This patch does the update.sh dance.  While in there I decided to fix a -Wunused-but-set-variable warning in yuv_convert.cpp as well.
Attachment #583340 - Attachment is obsolete: true
Attachment #584523 - Flags: review?(tterribe)
Comment on attachment 584523 [details] [diff] [review]
patch v3

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

r+ with one nit.

::: gfx/ycbcr/README
@@ +25,5 @@
>  win64.patch: SSE2 optimization for Microsoft Visual C++ x64 version
>  
>  TypeFromSize.patch: Bug 656185 - Add a method to detect YUVType from plane sizes.
> +
> +QuellGccWarnings.patch: Bug 711895 - Avoid -Wempty-body warnings with GCC.

With the addition of the yuv_convert.cpp fix, this description is no longer accurate.
Attachment #584523 - Flags: review?(tterribe) → review+
-Wtype-limits doesn't work with GCC 4.2 (on Mac) so I had to add a test for that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/54bf8b9d80be
https://hg.mozilla.org/mozilla-central/rev/54bf8b9d80be
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Comment on attachment 584523 [details] [diff] [review]
patch v3

This broke l10n nightlies because it runs a compiler check without seeing if we're allowed to run compiler checks.

In the future please seek review from a build system peer before adding new compiler checks.
Attachment #584523 - Flags: review-
Comment on attachment 584523 [details] [diff] [review]
patch v3

Actually, the patch in the bug is fine.  Turns out it's the check that got added between review and landing that broke things :-/
Attachment #584523 - Flags: review-
> This broke l10n nightlies because it runs a compiler check without seeing if
> we're allowed to run compiler checks.

Can you explain more?  I just copied the code for existing compiler checks, and all platforms built successfully on the try server.

> Actually, the patch in the bug is fine.

It's not, it broke all Mac builds.
(In reply to Nicholas Nethercote [:njn] from comment #15)
> > This broke l10n nightlies because it runs a compiler check without seeing if
> > we're allowed to run compiler checks.
> 
> Can you explain more?  I just copied the code for existing compiler checks,
> and all platforms built successfully on the try server.

All of our existing compiler checks are done in | if test "$COMPILE_ENVIRONMENT"; then | blocks, because we can't rely on our localizers having a full build environment available.

> > Actually, the patch in the bug is fine.
> 
> It's not, it broke all Mac builds.

Well, I meant in the "doesn't break l10n builds" sense, not the "has absolutely no problems whatsoever" sense
> All of our existing compiler checks are done in | if test
> "$COMPILE_ENVIRONMENT"; then | blocks, because we can't rely on our
> localizers having a full build environment available.

The pre-existing CXXFLAGS checks are not done in such a block.  The CXXFLAGS check I added is right next to the pre-existing checks.

I did add a CFLAGS check where there was none before but that is shortly before the CXXFLAGS checks and also not within such a block.

So I still don't understand what went wrong.
After looking at the logs a bit closer, it looks like the build bustage on tinderbox is something different.  It looks like the flag changes here caused the compiler used on the l10n builds to hit the error at http://hg.mozilla.org/mozilla-central/annotate/fff9a6ac640a//configure.in#l7863

We might still need the COMPILE_ENVIRONMENT check though, somebody needs to try configuring with --disable-compile-environment on Linux on a machine without gcc installed.
khuey, can you post the link to the logs?  I can't find them on TBPL.
I don't suppose the config.log file from that build is available anyway?  Failing that, can you tell me exactly what is different with the i10n builds -- do they specify --disable-compile-environment?  I don't know how to test this.

Alternatively, I could change line 7849, which is this:

  CXXFLAGS="$CXXFLAGS -pedantic ${_WARNINGS_CXXFLAGS} -Wno-long-long"

by just removing the ${_WARNINGS_CXXFLAGS}, which doesn't seem necessary and is presumably involved in the failure.  But I still need to know how to test that change.
What happened, I'd say, is totally not what anyone has been thinking up to now.

The actual failure is the -pedantic long long check, in http://mxr.mozilla.org/mozilla-central/source/configure.in#7851

l10n repacks don't set --disable-compile-environment, but because they don't do any compiling, they also don't set the GCC/GXX that we set to use a halfway new GCC when we're actually building something, so their compiler, unused except in configure checks, is 4.1.2 20061011 (Red Hat 4.1.1-29).

The only thing that changed around the -pedantic long long check is that you changed the _WARNINGS_CXXFLAGS, which are passed to that AC_TRY_COMPILE, so I'd say the answer to "what happened?" is that one of the changes you made to _WARNINGS_CXXFLAGS when passed to 4.1.2 causes it to fail that check.
Actually, the l10n repacks do some compiling to get the mar library to create updates.
No, you are very fortunate that you do *not* want to mess with the -pedantic long long check. Step one in testing touching it would be "get Solaris on a SPARC box; no, not that, much much older, you want GCC 2.7.2.3 on it." Then once you finished with that, you would still have to unbreak the next use of _WARNINGS_CXXFLAGS, because the actual problem is that you added -Wempty-body, which was added to GCC in 4.3, and -Wno-overlength-strings, which was added in GCC 4.2, so in 4.1.2 you're getting two unrecognized commandline flag errors.

Dunno whether the right thing to do about that is add them conditionally on the GCC version, or add them based on the results of ac_try_compile using each of them, but one way or the other we shouldn't fail in configure, and we should have an ac_try_compile that uses the final set of warning flags while saying that's what it's doing, so broken warning flags come up as a failure in "Sanity checking warning flags..." rather than in the -pedantic long long check.
Attached patch patch v4 (obsolete) — — Splinter Review 
Full description of what this patch does:

- Introduce the MOZ_{C,CXX}_SUPPORTS_WARNINGS macros, which remove a lot of
  duplicated code.  Note that the test for Wno-extended-offsetof was
  different to all the others, I'm not sure why, but by using the macros
  throughout I've removed this difference.

- Add comments explaining every warning option that we turn on and off.  We
  had some comments, but not for all warnings.

- Make configure.in and js/src/configure.in the same w.r.t. warnings.
  Previously they had some minor differences.

- In _WARNINGS_CFLAGS:
  - Remove -W (a.k.a. -Wextra) because we don't use it with C++ code and
    it's pretty noisy.
  - Remove -Wno-unused, unnecessary with -W gone.
  - Add -Wno-overlength-strings, because that's a truly useless warning.
  - Change -Wdeclaration-after-statement to
    -Werror=declaration-after-statement, because MSVC can't handle it.
  - Add -Wtype-limits and -Wempty-body, because they find real bugs and have
    few false positives.  -Wtype-limits was previously present enabled by
    -W.
     
- _WARNINGS_CXXFLAGS
  - Add -Wno-overlength-strings, because that's a truly useless warning.
  - Remove -Wsynth, because it's from ancient GCC versions and no longer
    exists.
  - Removes -Wno-non-virtual-dtor for C++.  This has no effect because that
    warning isn't enabled by -Wall or -Wextra or -pedantic.  (It is enabled by
    -Weffc++, not that that is relevant.)
  - Add -Wtype-limits and -Wempty-body, because they find real bugs and have
    few false positives.

- Fixes a few places where -Wtype-limits and -Wempty-body warnings were
  triggered.  These were r+'d by previous reviewers so you don't need to
  worry about them.

To test these changes, in particular the new macros:

- I checked that the generated 'configure' file looks good.

- I checked that config.log looks good.

- I temporarily added a check for a bogus warning (-Woogah-boogah) and
  confirmed that the check failed and that the option wasn't used.

I'll also do a try server run.
Attachment #584523 - Attachment is obsolete: true
Attachment #604024 - Flags: review?(khuey)
Attached patch patch v5 (obsolete) — — Splinter Review 
Tweaked version.  Changes:

- For khuey: So I discovered this:

    When an unrecognized warning option is requested (e.g.,
    -Wunknown-warning), GCC will emit a diagnostic stating that the option
    is not recognized.  However, if the -Wno- form is used, the behavior is
    slightly different: No diagnostic will be produced for
    -Wno-unknown-warning unless other diagnostics are being produced. This
    allows the use of new -Wno- options with old compilers, but if
    something goes wrong, the compiler will warn that an unrecognized
    option was used.

  This is true for clang as well.  So trying to compile with -Wno-foobar
  doesn't tell you iff -Wno-foobar is actually supported.  This caused some
  build failures on try server thanks to a complicated chain of knock-on
  events involving --enable-warnings-as-errors that I can't be bothered
  describing.  The upshot is that I tweaked the macros to test -Wfoobar but
  add -Wno-foobar to the FLAGS variable when appropriate.

- For khuey: I re-enabled -Wno-unused for C code to avoid lots of warnings in
  third-party code.

- For Waldo: -Wno-extended-offsetof is a clang-only thing, hence the
  __has_warning test, which is also a clang-only thing.  But since we're
  testing a bunch of other options without using __has_warning, there
  doesn't seem any point using __has_warning just for one test.  So I've
  continued to use the new macro for testing -Wno-extended-offsetof.
  However, I have moved it into a |if test "$CLANG_CXX"| block.

- For mhommey:  -Werror=return-type found a missing return in
  android/linker.c.  I just added |return 0|, suggestions for something
  better are welcome :)

- For jwatt:  -Wtype-limits combined with --enable-warnings-as-errors found
  this:

    nsSVGOuterSVGFrame.cpp:701:3: error: comparison of unsigned
                                  expression >= 0 is always true

  I've changed mRedrawSuspendCount from a PRUint32 to a PRInt32, which
  matches the same-named member in nsSVGSVGElement.cpp.  However, the
  assertion still doesn't look quite right -- should it be this?

    NS_ASSERTION(mRedrawSuspendCount > 0, "unbalanced suspend count!");

  (I.e. '>' instead of '>='.)
Attachment #604024 - Attachment is obsolete: true
Attachment #604024 - Flags: review?(khuey)
Attachment #604301 - Flags: review?(mh+mozilla)
Attachment #604301 - Flags: review?(khuey)
Attachment #604301 - Flags: review?(jwatt)
Attachment #604301 - Flags: review?(jwalden+bmo)
(In reply to Nicholas Nethercote [:njn] from comment #28)
> - For mhommey:  -Werror=return-type found a missing return in
>   android/linker.c.  I just added |return 0|, suggestions for something
>   better are welcome :)

That line is actually unreachable, but gcc can't know that. I'd put a MOZ_NOT_REACHABLE or whatever it's called.
Comment on attachment 604301 [details] [diff] [review]
patch v5

See comment 29.
Attachment #604301 - Flags: review?(mh+mozilla) → review-
Comment on attachment 604301 [details] [diff] [review]
patch v5

r=jwatt for the SVG and SMIL changes.

(In reply to Nicholas Nethercote [:njn] from comment #28)
> However, the
>   assertion still doesn't look quite right -- should it be this?
> 
>     NS_ASSERTION(mRedrawSuspendCount > 0, "unbalanced suspend count!");

You're right, it should be using ">". No need to fix that though, since the suspend redraw code is all going away in the patch in bug 734079.
Attachment #604301 - Flags: review?(jwatt) → review+
If fact you could just drop the change to nsSVGOuterSVGFrame given that. (Especially since it may cause some tests to start triggering that assertion and fail, or result in Jesse triggering it with his fuzzers and spending time filing pointless bugs.) Up to you though.
(In reply to Nicholas Nethercote [:njn] from comment #28)
>   This is true for clang as well.  So trying to compile with -Wno-foobar
>   doesn't tell you iff -Wno-foobar is actually supported.  This caused some
>   build failures on try server thanks to a complicated chain of knock-on
>   events involving --enable-warnings-as-errors that I can't be bothered
>   describing.  The upshot is that I tweaked the macros to test -Wfoobar but
>   add -Wno-foobar to the FLAGS variable when appropriate.

Clang's -Wunknown-warning-option and -Wno-unknown-warning-option might be relevant here. -Wno-unknown-warning-option was recently added as a default flag (bug 731316).
(In reply to Jonathan Watt [:jwatt] from comment #32)
> If fact you could just drop the change to nsSVGOuterSVGFrame given that.

I can't drop the change, it's causing build failures because of --enable-warnings-as-errors.
khuey, why'd you remove your review request?  If it's because glandium gave an r- for the part he reviewed, that doesn't affect the part you're reviewing...
Comment on attachment 604301 [details] [diff] [review]
patch v5

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

::: gfx/ycbcr/README
@@ +25,5 @@
>  win64.patch: SSE2 optimization for Microsoft Visual C++ x64 version
>  
>  TypeFromSize.patch: Bug 656185 - Add a method to detect YUVType from plane sizes.
> +
> +QuellGccWarnings.patch: Bug 711895 - Avoid some GCC compilation warnings.

Your patch is missing this file, isn't it?

::: js/src/configure.in
@@ +1896,5 @@
> +
> +    # Turn off the following warnings that -Wall/-pedantic turn on:
> +    # -Woverlength-strings - we exceed the minimum maximum length all the time
> +    # -Wctor-dtor-privacy - ???
> +    # -Winvalid-offsetof - ???

In C++98 it's not legal to have offsetof(Type, field) if Type is not a POD type.  Pointers to members are supposed to address this, but people have tried really hard on some of our code that triggered it and couldn't get a fix going, and given the perf-sensitiveness of it, they gave up and just disabled the warning.  C++11 permits this in at least some cases, so making any effort to fix the warning is even less worthwhile than just turning it off.

@@ +1897,5 @@
> +    # Turn off the following warnings that -Wall/-pedantic turn on:
> +    # -Woverlength-strings - we exceed the minimum maximum length all the time
> +    # -Wctor-dtor-privacy - ???
> +    # -Winvalid-offsetof - ???
> +    # -Wvariadic-macros - ???

-Wno-variadic-macros turns off warnings when compiling variadic macros in C++:

#define FOO(...) __VA_ARGS__

Variadic macros only arrived in C++11, so support in anything less is an extension, and gcc's warning about this so people are aware (and can disable the warning if needed).

::: js/src/jsgc.cpp
@@ -1434,5 @@
>  void
>  JSCompartment::reduceGCTriggerBytes(size_t amount)
>  {
>      JS_ASSERT(amount > 0);
> -    JS_ASSERT(gcTriggerBytes - amount >= 0);

This is what happens when people try to internalize compiler optimizations too much, rather than just letting the code read naturally and accepting whatever smarts the compiler provides.  :-\
Attachment #604301 - Flags: review?(jwalden+bmo) → review+
> Clang's -Wunknown-warning-option and -Wno-unknown-warning-option might be
> relevant here. -Wno-unknown-warning-option was recently added as a default
> flag (bug 731316).

Thanks for the suggestion!   Unfortunately, GCC lacks those, and this code is mostly shared between GCC and clang.  So I'm happy to stick with a less "proper" approach that works with both compilers.
Attached patch patch v6 (obsolete) — — Splinter Review 
This version adds the MOZ_NOT_REACHED to android/linker.c as requested.
Attachment #604301 - Attachment is obsolete: true
Attachment #606430 - Flags: review?(mh+mozilla)
Comment on attachment 606430 [details] [diff] [review]
patch v6

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

r+, with nits.

::: build/autoconf/compiler-opts.m4
@@ +18,5 @@
> +dnl the flags variable.
> +
> +AC_DEFUN([MOZ_C_SUPPORTS_WARNING],
> +[
> +    AC_CACHE_CHECK(whether the C compiler supports $1$2, $3,

You should be able to compute $3 from $2, which would simplify caller's task.

By the way, why separate -W and foobar?

@@ +23,5 @@
> +        [
> +            AC_LANG_SAVE
> +            AC_LANG_C
> +            _SAVE_CFLAGS="$CFLAGS"
> +            CFLAGS="$CFLAGS -W$2"

Any particular reason this is not $1$2 ?

@@ +32,5 @@
> +            CFLAGS="$_SAVE_CFLAGS"
> +            AC_LANG_RESTORE
> +        ])
> +    if test "${$3}" = "yes"; then
> +        _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} $1$2"

I think I'd prefer for that to be done by the caller, and have the function have the "usual" pattern: func(..., do_when_yes, do_when_no)

::: other-licenses/android/linker.c
@@ +1925,5 @@
>        return *(unsigned int *)(si->base + si->plt_rela[num].r_offset);
>      }
>  #endif
> +    MOZ_NOT_REACHED("plt_reloc");
> +    return 0;

You shouldn't need that return, when using MOZ_NOT_REACHED.
Attachment #606430 - Flags: review?(mh+mozilla) → review+
> By the way, why separate -W and foobar?
> 
> @@ +23,5 @@
> > +        [
> > +            AC_LANG_SAVE
> > +            AC_LANG_C
> > +            _SAVE_CFLAGS="$CFLAGS"
> > +            CFLAGS="$CFLAGS -W$2"
> 
> Any particular reason this is not $1$2 ?

From the patch:

+dnl GCC and clang will fail if given an unknown warning option like -Wfoobar. 
+dnl But later versions won't fail if given an unknown negated warning option
+dnl like -Wno-foobar.  So when we are check for support of negated warning 
+dnl options, we actually test the positive form, but add the negated form to 
+dnl the flags variable.


> > +    MOZ_NOT_REACHED("plt_reloc");
> > +    return 0;
> 
> You shouldn't need that return, when using MOZ_NOT_REACHED.

Look in mfbt/Assertions.h.  On some platforms MOZ_NOT_REACHED is defined like this:

  #    define MOZ_NOT_REACHED(reason)    ((void)0)

So I think it's worth having.
(In reply to Nicholas Nethercote [:njn] from comment #40)
> Look in mfbt/Assertions.h.  On some platforms MOZ_NOT_REACHED is defined
> like this:
> 
>   #    define MOZ_NOT_REACHED(reason)    ((void)0)
> 
> So I think it's worth having.

Well, linker.c is only built on Android, where I doubt MOZ_NOT_REACHED is that.
> Well, linker.c is only built on Android, where I doubt MOZ_NOT_REACHED is
> that.

MOZ_NOT_REACHED is not visible from linker.c.  Are you happy with just adding the |return 0;| ?
(In reply to Nicholas Nethercote [:njn] from comment #42)
> > Well, linker.c is only built on Android, where I doubt MOZ_NOT_REACHED is
> > that.
> 
> MOZ_NOT_REACHED is not visible from linker.c.

Why is that?

> Are you happy with just adding the |return 0;| ?

If MOZ_NOT_REACHED is not available, it's not like there's much choice :)
> > MOZ_NOT_REACHED is not visible from linker.c.
> 
> Why is that?

AFAICT none of the files in other-licenses/android/ #include any Mozilla header files.
(In reply to Nicholas Nethercote [:njn] from comment #44)
> > > MOZ_NOT_REACHED is not visible from linker.c.
> > 
> > Why is that?
> 
> AFAICT none of the files in other-licenses/android/ #include any Mozilla
> header files.

Because they haven't needed to so far.
https://hg.mozilla.org/mozilla-central/rev/0124795a8cbb
Status: REOPENED → RESOLVED
Closed: 13 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla12 → mozilla14
I'm trying to port this patch over to Thunderbird's configure.in (Bug 734080) and I'm not getting the behaviour I expect. The config.log is showing me that the compiler is complaining about the unknown option:

-- snipped from config.log
configure:6277: checking whether the C++ compiler supports -Wno-extended-offsetof
configure:6300: /usr/bin/clang++ -c  -fno-strict-aliasing -Wextended-offsetof  conftest.C 1>&5
warning: unknown warning option '-Wextended-offsetof' [-Wunknown-warning-option]
1 warning generated.
-- end of snip

but my C++ warning flags still include -Wno-extended-offsetof; config.status has:

s%@CXXFLAGS@%-fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-overlength-strings -Wno-ctor-dtor-privacy -Wno-invalid-offsetof -Wno-variadic-macros -Wno-c++0x-extensions -Wno-extended-offsetof -fno-strict-aliasing -fno-common -pthread -DNO_X11 -pipe%g

This is with an older version of clang: OS X 10.6, Xcode 4.0.2, Apple clang version 2.0 (tags/Apple/clang-139) (based on LLVM 2.9svn)


It appears that, at least on my system, AC_TRY_COMPILE is answering 'yes' despite the unknown-warning-option warning.

Just tried a test build of the browser only, in the mozilla tree, and I see the same issue - so I don't think it's something I introduced in the port to Thunderbird.
Did Thunderbid pick up bug 731316? See comment 33.
Adding -Werror to the AC_TRY_COMPILE calls in autoconf/compile-opts.m4 makes this work for me. Can we assume that all compilers support -Werror, or does this need additional trickery?


diff --git a/build/autoconf/compiler-opts.m4 b/build/autoconf/compiler-opts.m4
--- a/build/autoconf/compiler-opts.m4
+++ b/build/autoconf/compiler-opts.m4
@@ -91,17 +91,17 @@ dnl the flags variable.
 
 AC_DEFUN([MOZ_C_SUPPORTS_WARNING],
 [
     AC_CACHE_CHECK(whether the C compiler supports $1$2, $3,
         [
             AC_LANG_SAVE
             AC_LANG_C
             _SAVE_CFLAGS="$CFLAGS"
-            CFLAGS="$CFLAGS -W$2"
+            CFLAGS="$CFLAGS -Werror -W$2"
             AC_TRY_COMPILE([],
                            [return(0);],
                            $3="yes",
                            $3="no")
             CFLAGS="$_SAVE_CFLAGS"
             AC_LANG_RESTORE
         ])
     if test "${$3}" = "yes"; then
@@ -111,17 +111,17 @@ AC_DEFUN([MOZ_C_SUPPORTS_WARNING],
 
 AC_DEFUN([MOZ_CXX_SUPPORTS_WARNING],
 [
     AC_CACHE_CHECK(whether the C++ compiler supports $1$2, $3,
         [
             AC_LANG_SAVE
             AC_LANG_CPLUSPLUS
             _SAVE_CXXFLAGS="$CXXFLAGS"
-            CXXFLAGS="$CXXFLAGS -W$2"
+            CXXFLAGS="$CXXFLAGS -Werror -W$2"
             AC_TRY_COMPILE([],
                            [return(0);],
                            $3="yes",
                            $3="no")
             CXXFLAGS="$_SAVE_CXXFLAGS"
             AC_LANG_RESTORE
         ])
     if test "${$3}" = "yes"; then
diff --git a/js/src/build/autoconf/compiler-opts.m4 b/js/src/build/autoconf/compiler-opts.m4
--- a/js/src/build/autoconf/compiler-opts.m4
+++ b/js/src/build/autoconf/compiler-opts.m4
@@ -91,17 +91,17 @@ dnl the flags variable.
 
 AC_DEFUN([MOZ_C_SUPPORTS_WARNING],
 [
     AC_CACHE_CHECK(whether the C compiler supports $1$2, $3,
         [
             AC_LANG_SAVE
             AC_LANG_C
             _SAVE_CFLAGS="$CFLAGS"
-            CFLAGS="$CFLAGS -W$2"
+            CFLAGS="$CFLAGS -Werror -W$2"
             AC_TRY_COMPILE([],
                            [return(0);],
                            $3="yes",
                            $3="no")
             CFLAGS="$_SAVE_CFLAGS"
             AC_LANG_RESTORE
         ])
     if test "${$3}" = "yes"; then
@@ -111,17 +111,17 @@ AC_DEFUN([MOZ_C_SUPPORTS_WARNING],
 
 AC_DEFUN([MOZ_CXX_SUPPORTS_WARNING],
 [
     AC_CACHE_CHECK(whether the C++ compiler supports $1$2, $3,
         [
             AC_LANG_SAVE
             AC_LANG_CPLUSPLUS
             _SAVE_CXXFLAGS="$CXXFLAGS"
-            CXXFLAGS="$CXXFLAGS -W$2"
+            CXXFLAGS="$CXXFLAGS -Werror -W$2"
             AC_TRY_COMPILE([],
                            [return(0);],
                            $3="yes",
                            $3="no")
             CXXFLAGS="$_SAVE_CXXFLAGS"
             AC_LANG_RESTORE
         ])
     if test "${$3}" = "yes"; then
(In reply to Gregory Szorc [:gps] from comment #50)
> Did Thunderbid pick up bug 731316? See comment 33.

We weren't using build/autoconf/compiler-opts.m4; I'm adding that as part of porting this fix to Thunderbird, but it doesn't suppress the -Wno-extended-offsetof warning even for Firefox. Bug 731316 puts -Wno-unknown-warning-option too late on the command line, so it only actually helps for the -Wno-return-type-c-linkage flag.
njn, this patch breaks the Android build on Mac:

mozilla/central/other-licenses/android/getaddrinfo.c: In function '_test_connect':
mozilla/central/other-licenses/android/getaddrinfo.c:493: error: ISO C90 forbids mixed declarations and code
mozilla/central/other-licenses/android/getaddrinfo.c:497: error: ISO C90 forbids mixed declarations and code
mozilla/central/other-licenses/android/getaddrinfo.c: In function '_dns_getaddrinfo':
mozilla/central/other-licenses/android/getaddrinfo.c:1849: error: ISO C90 forbids mixed declarations and code

Our Android build machines are Linux, so try builds don't catch Android build breaks for Mac users.
Depends on: 737587
-Werror=declaration-after-statement breaks Android build on Mac.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Chris Peterson (:cpeterson) from comment #53)
> njn, this patch breaks the Android build on Mac:
> 
> mozilla/central/other-licenses/android/getaddrinfo.c: In function
> '_test_connect':
> mozilla/central/other-licenses/android/getaddrinfo.c:493: error: ISO C90
> forbids mixed declarations and code
> mozilla/central/other-licenses/android/getaddrinfo.c:497: error: ISO C90
> forbids mixed declarations and code
> mozilla/central/other-licenses/android/getaddrinfo.c: In function
> '_dns_getaddrinfo':
> mozilla/central/other-licenses/android/getaddrinfo.c:1849: error: ISO C90
> forbids mixed declarations and code

That should be trivial to fix, right?  Either by moving the code around, or if that can't be done for some reason, changing -Werror=declaration-after-statement to -Wdeclaration-after-statement in configure.in.
 
> Our Android build machines are Linux, so try builds don't catch Android
> build breaks for Mac users.

Sigh.
(In reply to Nicholas Nethercote [:njn] from comment #55)
> (In reply to Chris Peterson (:cpeterson) from comment #53)
> > Our Android build machines are Linux, so try builds don't catch Android
> > build breaks for Mac users.
> 
> Sigh.

The toolchain should be the same, though, so I guess it might really be a case of NDK r5 vs. NDK r6 or r7
(In reply to Gregory Szorc [:gps] from comment #47)
> This has taken us from ~4200 warnings in Clang to ~2600!
> http://jenkins.gregoryszorc.com:9000/job/mozilla-inbound-linux-x64-optimized-
> llvm-tip/

Good.  I suspect removing -Wextra from C builds is the main change -- I removed it because we weren't using it for C++ builds and many of the warnings it turns on are more annoying than useful.
Blocks: 737677
> -- snipped from config.log
> configure:6277: checking whether the C++ compiler supports
> -Wno-extended-offsetof
> configure:6300: /usr/bin/clang++ -c  -fno-strict-aliasing
> -Wextended-offsetof  conftest.C 1>&5
> warning: unknown warning option '-Wextended-offsetof'
> [-Wunknown-warning-option]
> 1 warning generated.
> -- end of snip
> 
> but my C++ warning flags still include -Wno-extended-offsetof; config.status
> has:
> 
> s%@CXXFLAGS@%-fno-rtti -fno-exceptions -Wall -Wpointer-arith
> -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body
> -Wno-overlength-strings -Wno-ctor-dtor-privacy -Wno-invalid-offsetof
> -Wno-variadic-macros -Wno-c++0x-extensions -Wno-extended-offsetof
> -fno-strict-aliasing -fno-common -pthread -DNO_X11 -pipe%g
> 
> This is with an older version of clang: OS X 10.6, Xcode 4.0.2, Apple clang
> version 2.0 (tags/Apple/clang-139) (based on LLVM 2.9svn)

I see the same behaviour with version 3.0 (tags/Apple/clang-211.12).


> Adding -Werror to the AC_TRY_COMPILE calls in autoconf/compile-opts.m4 makes
> this work for me. Can we assume that all compilers support -Werror, or does
> this need additional trickery?

That works for me too.  -Werror works with gcc 4.2 on Mac, which is one of the older compilers we have to deal with, and I would guess it works on much older compilers as well.

Thanks for identifying this, Irving.  I filed bug 737677 as a follow-up.
Blocks: 737678
> > mozilla/central/other-licenses/android/getaddrinfo.c:1849: error: ISO C90
> > forbids mixed declarations and code
> 
> That should be trivial to fix, right?  Either by moving the code around, or
> if that can't be done for some reason, changing
> -Werror=declaration-after-statement to -Wdeclaration-after-statement in
> configure.in.

I filed bug 737678 for making it non-fatal.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Apparently, this broke the l10n nightlies again. http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mozilla-l10n-ak&legend=0&norules=1&mindate=1332248263&maxdate=1332248548 worked, http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mozilla-l10n-ak&legend=0&norules=1&mindate=1332263543&maxdate=1332263690 is busted, across locales, with this bug being the obvious candidate.

Error message from http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-l10n-ak/1332263603.1332263634.17841.gz&fulltext=1 is

checking whether C++ compiler has -pedantic long long bug... yes
configure: error: Your compiler appears to have a known bug where long long is miscompiled when using -pedantic.  Reconfigure using --disable-pedantic. 
program finished with exit code 1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Axel Hecht [:Pike] from comment #60)
> Apparently, this broke the l10n nightlies again.

Argh.  I suspect the problem is that I failed to check if -Wno-overlength-strings is supported.

What should I do?  Check for -Wno-overlength-strings and hope that's the problem?  Back the patch out?  Repeatedly backing patches out for bustage that doesn't show up on the try server isn't much fun :(
Status: REOPENED → ASSIGNED
Attached patch patch, v7 — — Splinter Review 
Yet another attempt.  The only changes made are to address problems found after the second landing attempt (now backed out):

- I'm now checking to see if -Wno-overlength-strings is supported.

- I added -Werror to MOZ_{C,CXX}_SUPPORTS_WARNING, as per bug 737677.  This fixes a slight clang botch.

- I changed -Werror=declaration-after-statement to -Wdeclaration-after-statement, to avoid some android-on-mac bustage, as per bug 737678.

- The nsSVGOuterSVGFrame.h change is gone because that code was removed by a subsequent commit.
Attachment #606430 - Attachment is obsolete: true
Attachment #608230 - Flags: review?(khuey)
Attached patch interdiff for v7 — — Splinter Review 
In case it's useful, here's the interdiff between what was backed out and v7.
khuey: review ping!
Comment on attachment 608231 [details] [diff] [review]
interdiff for v7

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

Please get a layout peer to look at the SVG thing.

::: b/layout/svg/base/src/nsSVGOuterSVGFrame.h
@@ +176,4 @@
>  
>    nsAutoPtr<gfxMatrix> mCanvasTM;
>  
> +  PRUint32 mRedrawSuspendCount;

As far as I can tell, this variable is unused?
Attachment #608231 - Flags: review+
> Please get a layout peer to look at the SVG thing.

I'm just reverting an SVG change that was in the earlier patch, so no need.
https://hg.mozilla.org/mozilla-central/rev/f64f62213f61
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
(In reply to Jeff Walden from comment #7)
> > +#define EMMS() (void)true;
> I suspect, given the other definitions, this shouldn't include a semicolon. 
> And the more canonical version would be ((void) 0), usually.
When I put something in to silence my compiler error locally I tried PR_BEGIN_MACRO PR_END_MACRO
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: