Warnings as errors in mozilla-central (take 2)

[Mounir Lamouri]

Hi,

Some time ago, Daniel Holbert tried to get warnings as errors in the
tree [1] and a few months ago Nicholas Nethercote tried to do the same
thing for our js engine. Both of those initiatives had a mitigate
success: currently, there are no directories with FAIL_ON_WARNINGS in
the tree and warnings as errors for the js engined moved to a specific
build in tbpl.

Nicholas opened a bug to have builds with warnings as errors in tbpl for
mozilla-central [3] to try to reduce the numbers of warnings we have
assuming some of them can be real issues.
To fix that bug I tried to take another approach than the one Daniel had
for the first iteration of warnings as errors. Indeed, he made warnings
as errors opt-out which very likely broke the builds of a few people who
had exotic configurations. Making warnings as errors opt-in makes sure
that no one who didn't ask for it will suffer from that feature but also
let us make tbpl builds warnings-free and increase our general code quality.

So, what I would like to propose is to change
--disable-warnings-as-errors to --enable-warnings-as-errors and have all
mozconfigs used by build bots to enable warnings as errors. As it is
already the case, each directory have to opt-in for warnings as errors
with FAIL_ON_WARNINGS. Basically, that means as soon as a directory is
warning-free we can be sure it will stay warning-free.
We have some obvious advantages to have warnings as errors in build bots
but it also come with a few issues. The major one I see is that updating
our build bots configuration might creates a lot of warnings which might
be fatal thus make the update process harder. But we could easily assume
that a directory warning-free in multiple compilers in multiple
platforms is unlikely to have tons of warnings if one compiler changes.
However, if that happens, we can always remove FAIL_ON_WARNINGS in the
directory.

The patches are ready to be pushed and if no one object, this is going
to happen so, please, feel free to give any feedback :)

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=557566
[2]
https://blog.mozilla.com/nnethercote/2011/04/19/warnings-as-errors-in-jssrc-take-2/
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=703121

Thanks,
--
Mounir

[Bobby Holley]

On Wed, Dec 28, 2011 at 1:37 PM, Mounir Lamouri <mou...@lamouri.fr> wrote:

> So, what I would like to propose is to change --disable-warnings-as-errors
> to --enable-warnings-as-errors
>

I have no objection to this.

have all mozconfigs used by build bots to enable warnings as errors.

I (moderately) object to this, for the same reasons that I expressed on the
js-engine thread:
http://groups.google.com/group/mozilla.dev.tech.js-engine.internals/browse_thread/thread/74f7a37507e00e30

In particular, there are a lot of pedantic warnings on MSVC that don't
appear on GCC. If we have WARNINGS_AS_ERRORS anywhere on tinderbox,
developers on Mac/Linux are going to waste a lot of time bouncing patches
for spurious reasons that they can't check for locally. The reverse
(developers on Windows) can also be true, though anecdotal evidence
indicates that it's less common.

I don't see a great way to mitigate this problem, though I remember
Nicholas was investigating whether there was a subset of warnings that
would reliably appear on all platforms. Did that ever go anywhere?

The patches are ready to be pushed and if no one object, this is going to
> happen so, please, feel free to give any feedback :)
>

The overall consensus on the js-engine thread was "we don't want this". So
I think there should be a pretty thorough discussion before proceeding on
this front.

Cheers,
bholley

[Nicholas Nethercote]

On Wed, Dec 28, 2011 at 1:56 PM, Bobby Holley <bobby...@gmail.com> wrote:
>
> I don't see a great way to mitigate this problem, though I remember
> Nicholas was investigating whether there was a subset of warnings that
> would reliably appear on all platforms. Did that ever go anywhere?

I've been looking a lot at GCC warnings only. A short summary:

* -Wreturn-type is wonderful, these warnings are almost always bugs,
and we turn on -Werror=return-type when possible to make them fatal.
This is great.

* I haven't found any other GCC warnings as unambiguously good as
-Wreturn-type. But most of the GCC warnings we have enabled are easy
to fix and worth fixing, IMO. (And bug 711895 will turn on a couple
more.) The most common one is -Wunused-but-set-variable which is
definitely worth fixing.

* The one big exception is -Winitialized. It causes lots of false
positives, and there is a wide range of opinions on how to deal with
it, and it'll be hard to get consensus, AFAICT. For example, in bug
711908 I fixed all the non-Winitialized warnings in layout, but there
were still heaps of false positive Winitialized warnings left, and
dbaron didn't want me to artificially initialize things to quell them.
Some of them could be avoided if we had a "noreturn" fatal assertion
in the default cases of switches that should never be hit.

If there is consensus that MSVC's warnings are generally less useful
than GCC's, I wonder if having a variant of FAIL_ON_WARNINGS that only
actives warnings-as-errors for GCC would be useful.

Nick

[Daniel Holbert]

On 12/28/2011 01:56 PM, Bobby Holley wrote:
> In particular, there are a lot of pedantic warnings on MSVC that don't
> appear on GCC. If we have WARNINGS_AS_ERRORS anywhere on tinderbox,
> developers on Mac/Linux are going to waste a lot of time bouncing patches
> for spurious reasons that they can't check for locally.

Well, let's consider the possibilities, when a MSVC warning is
introduced in a formerly-warning-free directory:

(a) If the warning always indicates an actual bug, then it's clearly
good that it bounces, so the developer realizes this & can fix it.

(b) If the warning sometimes indicates a bug and sometimes is
ignorable, then it's still probably good that it bounces, because we
really would like a human to distinguish which of those cases we're in
(bug vs. ignorable), and the human then can address the issue or silence
the warning in a targeted way if necessary (which is easy in MSVC[1])

(c) If the warning is always useless/ignorable, then we should just
disable them tree-wide (which is also easy in MSVC)[2].

Dealing with those bounces would occupy some time, since Windows builds
are slow. Still, for the reasons above, I think it's generally better
that we catch (and either fix or silence) these issues up-front, rather
than letting them slip by undetected (potentially introducing bugs, or
at best muddying up formerly-warning-free directories).

~Daniel

[1] targeted MSVC warning disabling -- see e.g.:
http://hg.mozilla.org/mozilla-central/diff/1847cf67118a/js/src/jsscope.h
[2] tree-wide MSVC warning disabling -- see e.g.:
http://hg.mozilla.org/mozilla-central/diff/d4dbc269d83b/configure.in

[Bobby Holley]

On Wed, Dec 28, 2011 at 3:35 PM, Daniel Holbert <dhol...@mozilla.com>wrote:

> (b) If the warning sometimes indicates a bug and sometimes is ignorable,
> then it's still probably good that it bounces, because we really would like
> a human to distinguish which of those cases we're in (bug vs. ignorable),
> and the human then can address the issue or silence the warning in a
> targeted way if necessary (which is easy in MSVC[1])
>

The issue is that this is a very wide spectrum. What is the threshold for
false positives? Is a warning that indicates a bug 15 percent of the time
worth it? 30 percent? 5 percent?

If I get bit by a useless warning, I'm more likely to just fix it than I am
to start a thread on dev-platform to discuss disabling it tree-wide.
However, the fact that I land a fix doesn't mean that it was time
well-spent. This is why I prefer njn's approach of finding the most useful
warnings and enabling them on a case-by-case basis, rather than the reverse.

bholley

[Karl Tomlinson]

> [2] tree-wide MSVC warning disabling -- see e.g.:
> http://hg.mozilla.org/mozilla-central/diff/d4dbc269d83b/configure.in

Does MSVC provide a tree-wide means to still spit out and continue
on some warnings but make others errors?

cf. GCC's -Wno-error=uninitialized

[Nicholas Nethercote]

On Wed, Dec 28, 2011 at 3:48 PM, Bobby Holley <bobby...@gmail.com> wrote:
> On Wed, Dec 28, 2011 at 3:35 PM, Daniel Holbert <dhol...@mozilla.com>wrote:
>
>>   (b) If the warning sometimes indicates a bug and sometimes is ignorable,
>> then it's still probably good that it bounces, because we really would like
>> a human to distinguish which of those cases we're in (bug vs. ignorable),
>> and the human then can address the issue or silence the warning in a
>> targeted way if necessary (which is easy in MSVC[1])

I agree with you, but many others don't. I couldn't even get buy in
on enabling warnings-as-errors in the JS engine (for browser builds,
not just the hidden shell-only builds), and the JS engine has (a) lots
of manpower, (b) zero warnings already.

> If I get bit by a useless warning, I'm more likely to just fix it than I am
> to start a thread on dev-platform to discuss disabling it tree-wide.
> However, the fact that I land a fix doesn't mean that it was time
> well-spent. This is why I prefer njn's approach of finding the most useful
> warnings and enabling them on a case-by-case basis, rather than the reverse.

But I'm not actually doing that right now. As I said before, for GCC
there are basically three categories of warnings among those we have
enabled:

- Wreturn-type: always indicates a bug! Should be (and is) fatal.
- Winitialized: lots of false positives, working around can be painful.
- Everything else: sometimes indicates a bug, rarely hard to work around.

Nick

[Mounir Lamouri]

On 12/28/2011 10:56 PM, Bobby Holley wrote:
> I (moderately) object to this, for the same reasons that I expressed on the
> js-engine thread:
> http://groups.google.com/group/mozilla.dev.tech.js-engine.internals/browse_thread/thread/74f7a37507e00e30
>

> In particular, there are a lot of pedantic warnings on MSVC that don't
> appear on GCC. If we have WARNINGS_AS_ERRORS anywhere on tinderbox,
> developers on Mac/Linux are going to waste a lot of time bouncing patches

> for spurious reasons that they can't check for locally. The reverse
> (developers on Windows) can also be true, though anecdotal evidence
> indicates that it's less common.

Indeed, I tried to make content/html/content/src warning-free and making
it happen for Linux/Mac took me two runs on try while after three runs
for Windows, I gave up.
However, I was trying to convert an entire directory to make it
warning-free which is quite different from applying a patch to a
warning-free directory. AFAIK, we have no data for such case and we are
only speculating about how much burden this is going to be, right? I
mean, there is quite a difference between running one more try run
sometimes and two or three nearly all the time.

Based on that, I think we should begin by experimenting this. When the
patches will be pushed, only directories with FAIL_ON_WARNINGS will be
affected so we can opt-in a few directories and see what happens. As
this thread shows we have multiple ways to reduce the burden and based
on how big it is, we could try to use one of those.

If I've followed correctly the discussion, those are the different
things that have been mentioned:
- Make some warnings non-fatal (-Wnoerror);
- Only make a set of warning fatal instead of all of them;
- Create a FAIL_ON_WARNINGS variant to have it happen only with GCC.

In addition of those, we could add:
- Create a DONT_FAIL_ON_MSVC_WARNINGS that could be added when
FAIL_ON_WARNINGS is used;
- Don't use --enable-warnings-as-errors on Windows builds;
- Try to have a warning parity between all platforms by adding warnings
that appear on MSVC to our GCC builds (if that's possible).

It goes without saying that some proposal are more extreme than others.

So does experimenting a basic warnings as errors then experimenting one
of those 6 proposals to reduce the burden if it happens to be too high
seems reasonable?

Thanks,
--
Mounir

[Ted Mielczarek]

Yes, MSVC allows you to disable specific warnings by number, like /wd1234:
http://msdn.microsoft.com/en-us/library/thxezb7y.aspx

Apparently you can also turn specific warnings into errors like
/we1234, which I was unaware of.

-Ted

[Kyle Huey]

On Thu, Dec 29, 2011 at 7:23 AM, Ted Mielczarek <t...@mielczarek.org> wrote:

> Apparently you can also turn specific warnings into errors like
> /we1234, which I was unaware of.
>

Yep, and we use that now to make 'foo == bar;' a compile error (and maybe
some others that I don't remember).

- Kyle

[Mounir Lamouri]

On 12/29/2011 01:23 PM, Ted Mielczarek wrote:
> On Wed, Dec 28, 2011 at 9:56 PM, Karl Tomlinson<moz...@karlt.net> wrote:

> Yes, MSVC allows you to disable specific warnings by number, like /wd1234:
> http://msdn.microsoft.com/en-us/library/thxezb7y.aspx
>

> Apparently you can also turn specific warnings into errors like
> /we1234, which I was unaware of.

I believe he was asking for the exact opposite: when all warnings are
errors, how to make some of them being warnings instead of errors? With
GCC, you can do -Werror -Wno-error=foo
Disabling a warning is a solution but a bit aggressive because the
warning doesn't even appear when the idea is to make it non-fatal.

--
Mounir

[Justin Lebar]

+1 on warnings-as-errors just on GCC.

It seems that this would get us 95% of the benefit of
warnings-as-errors with only 50% of the pain.

If warnings-as-errors is a smashing success, we can always turn it on
for Windows too. But I'm in favor of figuring it out on GCC first.

On Thu, Dec 29, 2011 at 8:44 AM, Mounir Lamouri <mou...@lamouri.fr> wrote:
> On 12/29/2011 01:23 PM, Ted Mielczarek wrote:
>>
>> On Wed, Dec 28, 2011 at 9:56 PM, Karl Tomlinson<moz...@karlt.net>  wrote:
>>>>

>> Yes, MSVC allows you to disable specific warnings by number, like /wd1234:
>> http://msdn.microsoft.com/en-us/library/thxezb7y.aspx
>>
>> Apparently you can also turn specific warnings into errors like
>> /we1234, which I was unaware of.
>
>
> I believe he was asking for the exact opposite: when all warnings are
> errors, how to make some of them being warnings instead of errors? With GCC,
> you can do -Werror -Wno-error=foo
> Disabling a warning is a solution but a bit aggressive because the warning
> doesn't even appear when the idea is to make it non-fatal.
>
> --
> Mounir
>

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Kyle Huey]

On Thu, Dec 29, 2011 at 9:32 AM, Justin Lebar <justin...@gmail.com>wrote:

> +1 on warnings-as-errors just on GCC.
>
> It seems that this would get us 95% of the benefit of
> warnings-as-errors with only 50% of the pain.
>

Is this because MSVC spits out more ignorable warnings or because most of
our developers are building with gcc?

- Kyle

[Ehsan Akhgari]

On Thu, Dec 29, 2011 at 9:32 AM, Justin Lebar <justin...@gmail.com>wrote:

> +1 on warnings-as-errors just on GCC.
>
> It seems that this would get us 95% of the benefit of
> warnings-as-errors with only 50% of the pain.
>

> If warnings-as-errors is a smashing success, we can always turn it on
> for Windows too. But I'm in favor of figuring it out on GCC first.
>

If we decide to go down this path, we should probably make sure to
differentiate between clang and gcc, because clang in my experience is more
strict (and also produces _way more useful_ warnings) than gcc.

Also, FWIW, I'm in favor of this plan!

--
Ehsan
<http://ehsanakhgari.org/>

[Daniel Holbert]

On 12/29/2011 06:48 AM, Kyle Huey wrote:
> On Thu, Dec 29, 2011 at 9:32 AM, Justin
Lebar<justin...@gmail.com>wrote:
>
>> +1 on warnings-as-errors just on GCC.
>>
>> It seems that this would get us 95% of the benefit of
>> warnings-as-errors with only 50% of the pain.
>>
>

> Is this because MSVC spits out more ignorable warnings or because most of
> our developers are building with gcc?

It's a combination of those as well as other factors, I'd imagine.

One other benefit of a gcc-only approach: our non-windows builders are
faster (and more numerous) than our windows builders, so if you happen
to accidentally introduce a warning (on Try or m-c/m-i), you'll find out
about it relatively quickly, and you can bounce possible-fixes with
relatively quick turnaround. (as compared to the wait times if you
introduced an MSVC-only warning)

I'd also give my +1 to making warnings-as-errors GCC-only.

[Daniel Holbert]

On 12/29/2011 02:54 AM, Mounir Lamouri wrote:
> If I've followed correctly the discussion, those are the different
> things that have been mentioned:

> - Create a FAIL_ON_WARNINGS variant to have it happen only with GCC.

Or:
- Convert the existing FAIL_ON_WARNINGS to be GCC-only (e.g.
"FAIL_ON_WARNINGS_GCC"). Then, if we later decide to support MSVC
warnings-as-errors (even just for local builds), we could add a
FAIL_ON_WARNINGS_MSVC (and even FAIL_ON_WARNINGS_CLANG) variant.

> In addition of those, we could add:

> - Don't use --enable-warnings-as-errors on Windows builds;

FWIW, I'd be on board with simply applying this tweak to your existing
patches and calling it good for now. (We can always do the
s/WARNINGS/WARNINGS_GCC/ variable-name-tweak later on, if this
experiment pans out and we decide that per-compiler variables are worth it.)

~Daniel

[Steve Fink]

On 12/29/2011 02:54 AM, Mounir Lamouri wrote:

> However, I was trying to convert an entire directory to make it
> warning-free which is quite different from applying a patch to a
> warning-free directory. AFAIK, we have no data for such case and we
> are only speculating about how much burden this is going to be, right?
> I mean, there is quite a difference between running one more try run
> sometimes and two or three nearly all the time.

We have data from the JS tree, which is currently warnings-free. (Where
"currently" is a little fuzzy; it tends to regularly accrete a few
warnings for a little while, though they generally get fixed within a
week.) And the conclusion of most -- but not all -- of the JS developers
is that default warnings-as-errors is still too annoying.

Personal anecdote: all of my local mozconfigs build with
--enable-sm-fail-on-warnings, which means warnings-as-errors for the JS
tree only. It's a nuisance, though not quite enough of one to make me
stop doing it. (Or rather, the nuisance factor of accidentally landing a
new warning is still larger in my mind than the nuisance of
warnings-as-errors.)

Specifically what is annoying is that I frequently add temporary code
with warnings -- 90% of the time, that means printf format warnings (I
don't remember whether size_t should be %d, %u, or %lu, and because I'm
on a little-endian platform and the code is only for debugging anyway, I
don't care.) I suppose if I were smart I would selectively turn off the
fatal nature of just that warning. I don't want to turn it off entirely
because it catches a lot of printf("%s", number) problems for me.

Also, I think it's important to consider the effects of
warnings-as-errors not just on regular contributors, but on first-time
patch authors as well. They're different because (1) our patch-landing
workflow is already too hairy before you're used to it; (2) first-timers
don't have direct try server access to see the warnings on other
platforms; and (3) I'd guess that first-timers are somewhat more willing
to spend time chasing down minor problems like warnings as long as they
have the tools to do so. (Not to mention 1b, our patch-landing workflow
is too hairy even after you're used to it.)

Those points argue both ways, and I don't know what the balance is, but
I just want to make sure we consider our contributor on-ramp in relation
to this topic.

[Mounir Lamouri]

On 12/29/2011 06:16 PM, Daniel Holbert wrote:
>> In addition of those, we could add:
>> - Don't use --enable-warnings-as-errors on Windows builds;
>
> FWIW, I'd be on board with simply applying this tweak to your existing
> patches and calling it good for now. (We can always do the
> s/WARNINGS/WARNINGS_GCC/ variable-name-tweak later on, if this
> experiment pans out and we decide that per-compiler variables are worth
> it.)

Does anyone has an objection against trying this?

--
Mounir

[Karl Tomlinson]

I'm fine with trying that if WARNINGS_AS_ERRORS includes
-Wno-error=uninitialized, or something similar is in place to
address comments 4 and 16.

[Bobby Holley]

This sounds fine to me (though I don't develop on Windows). I'm happy to
give it a shot.

bholley

On Thu, Dec 29, 2011 at 12:30 PM, Mounir Lamouri <mou...@lamouri.fr> wrote:

> On 12/29/2011 06:16 PM, Daniel Holbert wrote:
>
>> In addition of those, we could add:
>>> - Don't use --enable-warnings-as-errors on Windows builds;
>>>
>>
>> FWIW, I'd be on board with simply applying this tweak to your existing
>> patches and calling it good for now. (We can always do the
>> s/WARNINGS/WARNINGS_GCC/ variable-name-tweak later on, if this
>> experiment pans out and we decide that per-compiler variables are worth
>> it.)
>>
>
> Does anyone has an objection against trying this?
>

> --
> Mounir
>
> ______________________________**_________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/**listinfo/dev-platform<https://lists.mozilla.org/listinfo/dev-platform>
>

[Nicholas Nethercote]

On Fri, Dec 30, 2011 at 7:30 AM, Mounir Lamouri <mou...@lamouri.fr> wrote:
>
> Does anyone has an objection against trying this?

I suggest trying something conservative -- e.g. only make GCC warnings
fatal, and exclude -Winitialized -- with the upfront promise to
revisit this in, say, 2 weeks.

Except... we currently have a lot of GCC warnings. So this could only
be done for modules that don't have any warnings right now. Which
ones are candidates?

Nick

[Ted Mielczarek]

On Wed, Dec 28, 2011 at 4:37 PM, Mounir Lamouri <mou...@lamouri.fr> wrote:
> Hi,
>
> Some time ago, Daniel Holbert tried to get warnings as errors in the tree
> [1] and a few months ago Nicholas Nethercote tried to do the same thing for
> our js engine. Both of those initiatives had a mitigate success: currently,
> there are no directories with FAIL_ON_WARNINGS in the tree and warnings as
> errors for the js engined moved to a specific build in tbpl.

philor pointed out in IRC that this plan will take the entire tree a
step further than the JS engine did. Currently, there are separate
Spidermonkey shell builds which fail on warnings, and they are not
displayed on tbpl by default. This means that while there are
indicators that new warnings have been added to JS code, they don't
cause anyone's patches to be backed out, and presumably only people
that actually care about tracking warnings pay attention to them. The
plan of record here will cause people's patches to be backed out if
they add new warnings in certain areas of the codebase.

I agree with the motivation here, I'm just not sure the pain it's
going to cause is worth it, as evidenced by our previous attempts to
make this happen.

-Ted

[Jeff Walden]

On 12/30/2011 06:42 AM, Ted Mielczarek wrote:
> and presumably only people
> that actually care about tracking warnings pay attention to them

The consistently cantankerous philor would say, regarding this point, that the set of such people is the empty set. These days I suspect that's because inbound means the original JS person isn't watching for it (and I guess inbound sheriffs don't watch for JS warnings-as-errors failures? maybe they should, but then the JS anti-fatal-warning haters ;-) won't get to have their cake and eat it too). Every so often when he pokes me about it, and I'll get angry and go fix them. Same thing usually happens when I do the occasional build on Windows, too, because nobody watches for those failures, so they linger much longer.

As I see it, this delayed warning-fixing makes for the worst of all worlds as you'll never know if your fresh tree will actually be warning-free. Thus I am in favor of anything which makes it more likely people's initial pushes will be warning-free, including insta-backouts for new warnings.

Jeff

[Kyle Huey]

On Fri, Dec 30, 2011 at 4:01 PM, Jeff Walden <jwald...@mit.edu> wrote:

> The consistently cantankerous philor would say, regarding this point,
> that the set of such people is the empty set. These days I suspect that's
> because inbound means the original JS person isn't watching for it (and I
> guess inbound sheriffs don't watch for JS warnings-as-errors failures?
> maybe they should, but then the JS anti-fatal-warning haters ;-) won't get
> to have their cake and eat it too). Every so often when he pokes me about
> it, and I'll get angry and go fix them. Same thing usually happens when I
> do the occasional build on Windows, too, because nobody watches for those
> failures, so they linger much longer.
>

If you really want us to start backing out patches that add warnings to the
js engine I'll be more than happy to oblige. I expect that the js team
will either decide that the warnings as errors isn't important or move back
to tracemonkey rather quickly.

- Kyle

[Jeff Walden]

On 12/30/2011 06:20 PM, Kyle Huey wrote:
> If you really want us to start backing out patches that add warnings to the
> js engine I'll be more than happy to oblige. I expect that the js team
> will either decide that the warnings as errors isn't important or move back
> to tracemonkey rather quickly.

I don't think anyone wants to move back to TraceMonkey, because then we'd have to do our own merge duty, and it's a big time sink for the poor sa^Houl. (Have I mentioned that inbound sheriffs are awesome?)

At the very least, JS people should be poked when they introduce warnings so that they know they need fixing. I don't think that extra step would meet with too much resistance. I hope. :-\

Jeff

[Robert Kaiser]

Mounir Lamouri schrieb:

> So, what I would like to propose is to change

> --disable-warnings-as-errors to --enable-warnings-as-errors and have all

> mozconfigs used by build bots to enable warnings as errors.

Actually, we have tried for a while to have buildbots mozconfigs to be
as small as possible by making everything set on all of them the
defaults and to have developers by default run as near as possible to
the compilation we're doing for our official builds. I don't think it's
a good idea to break that.
If we believe that warnings should be errors on our official build
machines, then developers should see the same on their machines unless
they set some special mozconfig flag, IMHO.

Of course, we can make configure to set a set of flags by default that
only makes those warnings be errors that we really care about, and only
on the compilers where they are useful - and then we can have a general
mozconfig flag that turns on or off all of them (or set a different set).

Robert Kaiser

[Daniel Holbert]

On 12/31/2011 06:32 AM, Robert Kaiser wrote:
> If we believe that warnings should be errors on our official build
> machines, then developers should see the same on their machines unless
> they set some special mozconfig flag, IMHO.

That was precisely the strategy I used when I attempted this before.

However, the problem was that developers don't use the same
compiler-versions as our build machines, and different compilers produce
different warnings -- so developers end up with random local brokenness
even though tinderbox was green. dbaron put it well here:
http://groups.google.com/group/mozilla.dev.platform/msg/215b7bcb1a4ee751

[Neil]

Daniel Holbert wrote:

> However, the problem was that developers don't use the same
> compiler-versions as our build machines, and different compilers
> produce different warnings -- so developers end up with random local
> brokenness even though tinderbox was green.

I actually use a compiler version that errors out while other versions
just warn, so I lose out here :-(

--
Warning: May contain traces of nuts.

[Nicholas Nethercote]

On Fri, Dec 30, 2011 at 11:42 PM, Ted Mielczarek <t...@mielczarek.org> wrote:
>
> I agree with the motivation here, I'm just not sure the pain it's
> going to cause is worth it, as evidenced by our previous attempts to
> make this happen.

Like I said earlier, can we try this for 2 weeks and then come back to
discuss? Everyone is just speculating at the moment.

(And recall that we're talking about turning on fatal warnings only
for GCC, and only for a small number of modules, so the experience of
the JS team -- of which I am a member! -- is of limited relevance
because the conditions they have are quite different.)

Nick

[Nicholas Nethercote]

On Sun, Jan 1, 2012 at 8:47 AM, Daniel Holbert <dhol...@mozilla.com> wrote:
>>
>> If we believe that warnings should be errors on our official build
>> machines, then developers should see the same on their machines unless
>> they set some special mozconfig flag, IMHO.
>

> That was precisely the strategy I used when I attempted this before.
>

> However, the problem was that developers don't use the same
> compiler-versions as our build machines, and different compilers produce
> different warnings -- so developers end up with random local brokenness even

> though tinderbox was green. dbaron put it well here:
> http://groups.google.com/group/mozilla.dev.platform/msg/215b7bcb1a4ee751

Correct, we can't do that. I tried it in the JS engine a while back
and had to revert due to frequent breakage on unusual platforms.

The great thing about doing it only on TBPL is that you find out if
you introduced a warning when you send your patch to the try server.
(You are sending your patches to the try server before landing,
right?)

Nick

[Boris Zbarsky]

On 12/31/11 8:07 PM, Nicholas Nethercote wrote:
> (You are sending your patches to the try server before landing,
> right?)

Not all of them, by any means. Try server cycles aren't quite free, so
I tend to only send patches that have what I consider a reasonable (>
10% or so) chance of breaking something.

-Boris

[Nicholas Nethercote]

On Mon, Jan 2, 2012 at 4:06 AM, Boris Zbarsky <bzba...@mit.edu> wrote:
>>
>> (You are sending your patches to the try server before landing,
>> right?)
>
> Not all of them, by any means.  Try server cycles aren't quite free, so I
> tend to only send patches that have what I consider a reasonable (> 10% or
> so) chance of breaking something.

Sure, me too, for certain patches. But if you've deemed a patch is
low-risk enough to not bother with try server, it seems unlikely that
it'll introduce a warning-as-error that you don't see locally. (It's
not guaranteed, of course, but if you're landing without testing on
try server there's always a chance you'll break something anyway.)

But I expect that this case will be rare, because most patches are
tested via the try server. I could be wrong, which is why I keep
suggesting turning on some fatal warnings for a two week trial period.

Nick

[Boris Zbarsky]

On 1/1/12 3:23 PM, Nicholas Nethercote wrote:
> Sure, me too, for certain patches. But if you've deemed a patch is
> low-risk enough to not bother with try server, it seems unlikely that
> it'll introduce a warning-as-error that you don't see locally.

In a sane world, yes. In practice, some of gcc's warnings are downright
batty, and whether they appear or not depends on random things. Yes, I
realize we're probably not going to make those particular warnings into
errors, per prior discussion.

The other important bit is that a lot of the gcc warnings only appear in
opt builds, which is the opposite of the usual setup where a debug build
will tend to show problems (via assertions, etc) better than an opt
build. I almost never build opt locally except for performance testing,
so if warning-as-error is enabled for opt-only warnings, I'd pretty much
have to push every single patch I write to try.

> (It's
> not guaranteed, of course, but if you're landing without testing on
> try server there's always a chance you'll break something anyway.)

Well, sure. There's a chance even if I _did_ test on try server
(witness PGO-only bustage). It's a matter of probabilities.

> But I expect that this case will be rare, because most patches are
> tested via the try server.

I would love to see data on this...

> I could be wrong, which is why I keep
> suggesting turning on some fatal warnings for a two week trial period.

That seems like a reasonable step, yes.

-Boris

[Daniel Holbert]

On 01/01/2012 12:37 PM, Boris Zbarsky wrote:
> The other important bit is that a lot of the gcc warnings only appear in
> opt builds

From my recollection of my last anti-build-warning crusade, the main
warning in this category was "$x may be used uninitialized", and nn's
already advocated making that a non-error. So we don't need to worry
about that one.

The other big opt-only warning that I recall is for unused variables, in
constructs like:
{
bool success = DoSomething();
NS_ABORT_IF_FALSE(success, "we should have succeeded");
}
In these cases, the bustage-fix is pretty trivial -- just use
mozilla::DebugOnly<bool>.

Beyond those two categories, I don't remember any other opt-only warnings
that we hit frequently.

~Daniel

[Nicholas Nethercote]

On Mon, Jan 2, 2012 at 7:37 AM, Boris Zbarsky <bzba...@mit.edu> wrote:
>
> In a sane world, yes.  In practice, some of gcc's warnings are downright
> batty, and whether they appear or not depends on random things.  Yes, I
> realize we're probably not going to make those particular warnings into
> errors, per prior discussion.

Can you specify which warnings you think are batty? I've looked
through all the GCC warnings recently and experimented extensively
with getting rid of the ones that appear in Mozilla. Apart from
-Winitialized, which has already been mentioned as a difficult mixed
bag, the only ones I think that we currently use that are clearly not
worthwhile are -Woverlength-strings (which triggers about 22 warnings,
and which bug 711895 will disable) and -Wmain (which triggers about 11
warnings).

> The other important bit is that a lot of the gcc warnings only appear in opt
> builds, which is the opposite of the usual setup where a debug build will
> tend to show problems (via assertions, etc) better than an opt build.

Can you give examples? dholbert mentioned -Winitialized and ones
relating to variables that are unused in opt builds due to them only
being used in assertions. Are there any other warnings that only show
up in opt builds?

>  I
> almost never build opt locally except for performance testing, so if
> warning-as-error is enabled for opt-only warnings, I'd pretty much have to
> push every single patch I write to try.

Can you estimate what fraction of your patches you don't push to try?
For me it's a small number, maybe 10%.

(BTW, in case it's of interest, I've included some stats below about
which warnings get triggered for linux64 debug builds. They're from a
build that's a few weeks old so the numbers are a little out of date.
And I think I missed some in my post-processing.)

Nick

-----------------------------------------------------------------------------
currently enabled warnings (82408:5b4a90374698)
-----------------------------------------------------------------------------
total: 972 warnings

( 1) 173 (17.8%, 17.8%): -Wunused-but-set-variable [-Wall]
( 2) 134 (13.8%, 31.6%): -Wuninitialized [-Wall/-Wextra]
( 3) 126 (13.0%, 44.5%): -Wenum-compare [-Wall]
( 4) 70 ( 7.2%, 51.7%): -Woverloaded-virtual [C++ only,
specified explicitly]
( 5) 69 ( 7.1%, 58.8%): -Wformat [-Wall]
( 6) 65 ( 6.7%, 65.5%): -Wsign-compare [-Wextra [but
occurs for C++ code??]]
( 7) 44 ( 4.5%, 70.1%): -Wint-to-pointer-cast [default]
( 8) 42 ( 4.3%, 74.4%): -Wwrite-strings [default in C++]
( 9) 35 ( 3.6%, 78.0%): -Wparentheses [-Wall]
(10) 34 ( 3.5%, 81.5%): -Wmissing-field-initializers [-Wextra,
WARNINGS_CFLAGS only]
(11) 29 ( 3.0%, 84.5%): -Wpointer-to-int-cast [C only, default]
(12) 24 ( 2.5%, 86.9%): -Wunused-variable [-Wall]
(13) 23 ( 2.4%, 89.3%): -Wunused-result [default]
(14) 22 ( 2.3%, 91.6%): -Woverlength-strings [-pedantic]
(15) 15 ( 1.5%, 93.1%): -Wreorder [C++ only, -Wall]
(16) 11 ( 1.1%, 94.2%): -Wmain [default in C++,
-Wall/-pedantic in C]
(17) 11 ( 1.1%, 95.4%): -Wcpp [default]
(18) 9 ( 0.9%, 96.3%): -Wswitch [-Wall]
(19) 9 ( 0.9%, 97.2%): -Wdeprecated-declarations [default]
(20) 8 ( 0.8%, 98.0%): -Wunused-function [-Wall]
(21) 6 ( 0.6%, 98.7%): -Wconversion-null [C++ only, default]
(22) 4 ( 0.4%, 99.1%): -Wold-style-declaration [C only, -Wextra]
(23) 3 ( 0.3%, 99.4%): -Woverflow [default]
(24) 2 ( 0.2%, 99.6%): -Wtype-limits [-Wextra,
WARNINGS_CFLAGS only]
(25) 1 ( 0.1%, 99.7%): -Waddress [-Wall]
(26) 1 ( 0.1%, 99.8%): -Wclobbered [-Wextra,
WARNINGS_CFLAGS only]
(27) 1 ( 0.1%, 99.9%): -Wvla [-pedantic]
(28) 1 ( 0.1%,100.0%): -Wunused-value [-Wall]

[Jeff Walden]

On 01/01/2012 07:25 PM, Nicholas Nethercote wrote:
> Can you estimate what fraction of your patches you don't push to try?
> For me it's a small number, maybe 10%.

Historically I've pushed almost none. In the last six months, maybe, I've started to use try more. Maybe 10-20% now? It depends on the patch. If it's JS code that shell tests will exercise, I run them (shell only, not browser) and count that good enough. It's rare that this backfires to the point of backing out completely. mfbt stuff I've done lately I run past try (in build-only mode) routinely, having been burned from the cross-compiler whole-source speciality of it. (A cost to centralization, but one I count well worth it.)

I don't much think about how I use try, to be honest, but I definitely skip try more often than I use it, on a per-bug basis. And I'm sure I'm nowhere close to 90% of my patches being try'd first.

Jeff

[Robert Kaiser]

Nicholas Nethercote schrieb:

> On Sun, Jan 1, 2012 at 8:47 AM, Daniel Holbert<dhol...@mozilla.com> wrote:
>>>
>>> If we believe that warnings should be errors on our official build
>>> machines, then developers should see the same on their machines unless
>>> they set some special mozconfig flag, IMHO.
>>
>> That was precisely the strategy I used when I attempted this before.
>>
>> However, the problem was that developers don't use the same
>> compiler-versions as our build machines, and different compilers produce
>> different warnings -- so developers end up with random local brokenness even
>> though tinderbox was green. dbaron put it well here:
>> http://groups.google.com/group/mozilla.dev.platform/msg/215b7bcb1a4ee751
>
> Correct, we can't do that. I tried it in the JS engine a while back
> and had to revert due to frequent breakage on unusual platforms.

That's why I also said "we can make configure to set a set of flags by

default that only makes those warnings be errors that we really care

about, and only on the compilers where they are useful" - which could be
a useful compromise, I think.

> (You are sending your patches to the try server before landing,
> right?)

I rarely ever have done that, because 1) in the role I took on in the
last year I don't have anything to do with writing application code and
2) I have never written C/C++ code. But then, if I'd do code that would
profit from that, I'd surely run it by try.

Robert Kaiser

[Kyle Huey]

On Mon, Jan 2, 2012 at 1:20 PM, Robert Kaiser <ka...@kairo.at> wrote:

> Nicholas Nethercote schrieb:

>
>> (You are sending your patches to the try server before landing,
>> right?)
>>
>
> I rarely ever have done that, because 1) in the role I took on in the last
> year I don't have anything to do with writing application code and 2) I
> have never written C/C++ code. But then, if I'd do code that would profit
> from that, I'd surely run it by try.
>

If you're not writing C/C++ code, then this decision has no impact on you
;-)

- Kyle

[Robert Kaiser]

Kyle Huey schrieb:

> If you're not writing C/C++ code, then this decision has no impact on you
> ;-)

That doesn't mean that I can't be interested in the topic.

And apart from that, what you say isn't even completely true, I guess.
For one thing, I'm still the module owner for the comm-central build
system, which parallels or inherits a lot of configuration settings,
including optimization flags. For the other, I'm analyzing crash data in
my main job, and even if the compiler warning/error flags might not
actively impact crash data, fixing warnings may even have its
connections to fixing code constructs that may play some role in some
crashes - I've seen stranger things happen in this project. ;-)

Robert Kaiser

[Kyle Huey]

On Tue, Jan 3, 2012 at 9:18 AM, Robert Kaiser <ka...@kairo.at> wrote:

> Kyle Huey schrieb:
>
> If you're not writing C/C++ code, then this decision has no impact on you
>> ;-)
>>
>
> That doesn't mean that I can't be interested in the topic.
>

Sure. That wasn't meant to be as dismissive as it probably came off as.
My point was that not sending patches to the tryserver won't be a problem
if you don't mess with C/C++ much.

- Kyle

[Boris Zbarsky]

On 1/1/12 8:25 PM, Nicholas Nethercote wrote:
> On Mon, Jan 2, 2012 at 7:37 AM, Boris Zbarsky<bzba...@mit.edu> wrote:
>>
>> In a sane world, yes. In practice, some of gcc's warnings are downright
>> batty, and whether they appear or not depends on random things. Yes, I
>> realize we're probably not going to make those particular warnings into
>> errors, per prior discussion.
>
> Can you specify which warnings you think are batty?

-Winitialized may cover them all; there were several different warnings,
but they all seemed to do with variable initialization, iirc.

>> The other important bit is that a lot of the gcc warnings only appear in opt
>> builds, which is the opposite of the usual setup where a debug build will
>> tend to show problems (via assertions, etc) better than an opt build.
>
> Can you give examples? dholbert mentioned -Winitialized and ones
> relating to variables that are unused in opt builds due to them only
> being used in assertions. Are there any other warnings that only show
> up in opt builds?

There may well not be. It's been a while since I looked at this, but I
do think that the main ones here once again involved variable
initialization, so presumably that falls under -Winitialized.

> Can you estimate what fraction of your patches you don't push to try?

I just looked at the 10 newest bugs I fixed (by filing time, not fix
time), and of those 2 were not C++ changes and of the remaining 8 I
believe I pushed 2 to try.

Note that there is a bit of survivor bias here, though: the things that
get pushed to try tend to actually fail on try, so get pushed multiple
times until they pass. They also tend to not get fixed as quickly as
the ones that don't fail tests (because the test failures need fixing
and all), so the latter may be overrepresented in fixed bugs.

In general, I probably have a smaller fraction of non-C++ changes, and
maybe a slightly higher fraction of things I push to try, but I doubt
the "pushed to try" fraction is higher than 40% or so.

Maybe that just means that I'm a hidebound idiot who's not used to try
and should use it more, of course. Quite plausible, actually. ;)

-Boris