179819 - Turn uninitialized warnings into errors

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

We have had serious bugs because of unitialized variables, including
topcrashers. Many times the compilers can catch them, yet we ignore them, either
on purpose or  by accident. The purpose of this bug is to make them into fatal
errors so that it will simply be impossible to have these errors anymore.

The only compiler that we know of at the moment capable of doing this is the one
that comes with MS VC++ 7. It can be done either by a pragma:

#pragma warning(error: 4701)

or with a compiler flag

-we4701

We should have at least one Tinderbox machine doing this.

We of course need to fix all the existing uninitialized variables before turning
this switch. I tried to list all in the depend on list...

Comment 1

[hacker formerly known as [e-mail]]

FWIW, you can accomplish the same thing with gcc using -Werror -Wuninitialized .
However, that also has the side-effect of bombing on all warnings, which isn't
necessarily a bad thing.  

Why not make it the default?  Scrounging up tinderbox machines to test a one-off
compiler flag option seems slightly wasteful when the feature is something we
could benefit from having on by default.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Some warnings are bogus -- particularly uninitialized variable warnings, and
compilers have major differences, even between gcc versions, in what they warn
about.

Comment 3

[Bradley Baetz (:bbaetz)]

gcc warns if you have:

void foo();

int g;

int bar(int a) {
  int x;
  int b = a;
  if (b)
    x = 1;

  foo();

  if (b)
    g = x;
}

 - its usage info doesn't yet cover that, although there is work being done on
that sort of stuff.

The cases where we s/b/someFunc()/, and know that the result from SomeFunc is a
simple value whoses result can't have ben changed by foo are not really false
positives, though, since the compielr can't prove much without doing whole
program analysis. In that case, if you use a temporary var for the result of
SomeFunc() (or Some-nonLocal-Var), then the compiler could avoid calling the
function the second time, which would be an improvment.

For the moment, though, we can't help that.

Comment 4

[Aleksey Nogin]

BTW, there is already a tracking bug for these warnings - see bug 59652.

Also, some of the bugs already have patches attached to them, just need to be
reviewed and/or checke in (I do not have CVS permissions) - bug 52285, bug 59673
and bug 59676

I already have a script that monitores these warnings (by checking the brad TBox
every 4 hours) and I always append a note to bugs where checkin have introduced
a new one. I would *love* to have an "official" authority to reopen such bugs -
these would at least make sure new warnings are not introduced. (I got "shouted"
on couple of times for reopening such bugs and these days I only reopen when the
warning indicates a clear bug).

In most cases I did not have much problems getting people to agree on a patch
fixing the warnings even when they were the case of a compiler stupidity, but
sometimes people are worried this may impact performance. Examples include a
"WONTFIX" resolution on bug 132148 and an "I object" by Brendan Eich on bug 59657

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I think we need to start tackling warnings starting from where we don't have an
overwhelming task ahead of us, and have some clear wins expected.

For the cases where fixing code to silence compiler "stupidity" is too expensive
(performance wise or otherwise), we should be able to use #pragmas etc. to get
rid of the warnings without actually changing the code that caused the warning.

Comment 6

[Bradley Baetz (:bbaetz)]

well, does windows handle uninited var warnings better? If so, could we somehow
run a tinderbox with the equiv of -Werror set for that?

Comment 7

[Daniel Veditz [:dveditz]]

It does, but we'll have to wait for a VC .NET tinderbox instead of VC6. There
are two different warnings, 4700 and 4701, which could be turned into errors
independently. I don't think we have any of the more severe 4700 warnings.

Comment 8

[hacker formerly known as [e-mail]]

*ahem* We've had a VC7 tinderbox on the ports page for weeks now.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So... this bug is being used as a banner under which to harass people who
explicitly say that the warnings are bogus and that making them go away would be
too painful.

Could we please clarify the situation on such warnings once and for all and stop
wasting people's time with them?

Comment 10

[Aleksey Nogin]

Well, my experience with "harrassing" people was that about the third of all the
*new* "uninitialized" warnings are real bugs (see bug 186835 for the most recent
example). Also, they quite often are the kind of bugs that would be *very* hard
to reproduce and debug (for example: bug 81851 and bug 125795). So, IMHO it's
worth "harassing" people about it. 

Now, if the warnings are ever turned into errors, then finally I would not have
to waste my time tracking these things down and the "harrassing" would be
finally be done automatically by the TBox. Also, this would make it harder to
ignore the warning which would prevent cases like the one descibed in bug 96870
comment #61. There a person spent two days chaising a bug caused by an
uninitalized variable (one of the dups of bug 125795) - I did my best in letting
people know the variable was being used uninitialized, reopened bug 96870 which
caused it, but was ignored.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So you encountered the usual issue -- that half the developers do not read their
bugmail.  That's not a reason to persist when the developers _do_ take the time
to read the mail, look at the code, analyze it, cite the gcc bug that causes it
to issue the incorrect warning, and _then_ mark the bug wontfix.

Comment 12

[Aleksey Nogin]

Well, one problem with wontfix'ing the warnings is that it's always possible
(and I remember it actually happening, although do not remember in which bug)
for a code to be modified in a way that a "safe" warning becomes an "unsafe"
one. Of course, if the warning is wontfix'ed, then such a change will go
unnoticed...

Also, wontfix'ing warnings would make it harder to turn such warnings into errors.

Note that we can not just wait until gcc is smarter - no matter how smart gcc
will always make mistakes, and it is not neccessarily a bug - the problem it is
solving is undecidable:

  char c;
  f();
  putc(c);

Being able to figure out whether c is ever used uninitialized in the code above
is equivalent to being able to solve the halting problem for f.

So we have to figure out what to do when gcc is wrong - just blaming it on gcc
is not going to work (at least not always).

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yes.  That's what my comment requested -- figure out in _this_ bug what to do in
those cases.  _Then_ ask people to deal.  Don't make comments like "perhaps we
should turn the warning off" in other bugs if you can't offer a way to do it. 
That's what this bug is for, as far as I can tell.

Comment 14

[Brendan Eich [:brendan]]

In the example,

  char c;
  f();
  putc(c);

if c is a local variable (storage class auto), then it's easy to prove that f
can't initialize it without hacking with aliases to stack memory in an OS- and
machine-dependent fashion.

What bothers me about many of these warnings is that gcc can do better and not
warn, without having to solve the halting problem.

/be

Comment 15

[Nicholas Allen]

Does VC++ at least find all the warnings that are actually valid?

If so, any spuriously flagged bit of code can have:

#pragma warning(disable: 4701)

...

#pragma warning(default: 4701)

wrapped around the function containing it to shut the compiler up when we are
smart enough to demonstrate that it's wrong.

Comment 16

[Aleksey Nogin]

Re: comment #14

Yeah, but f can be unterminating and then c is never actually used. The point
was that "can a variable be used uninitialized" is equivalent to halting problem
and thus is undecidable.

Re: comment #15

As far as I know the only TBox that currently generates these warnings is brad
(Linux gcc), so VC++ pragma woudn't probably help much. 

I could be mistaken, though (may be brad is the only such *clobber* build, but
if we indeed turn these warnings into errors clobbering would no longer be
necessary to detect these warnings).

Comment 17

[Boris Zbarsky [:bzbarsky]]

Read comment 0 and comment 1 of this bug please.... I doubt we will be turning
this on for gcc any time soon given the limitation listed in comment 1.  So we
are in fact talking about VC++ if we intend to make these into errors.

Comment 18

[Brendan Eich [:brendan]]

Re: comment #16, if f doesn't return, I will take the "c may be used before it
is set" warning, gladly.  Any way you interpret that silly code fragment,
there's a bug (an unintended iloop, an unset variable use, or dead code after an
intentional iloop with a useless declaration before it).

Let's keep this down to earth, per comment #17.  Cite real examples from live
code, for a start.

/be

Comment 19

[hacker formerly known as [e-mail]]

Mass reassign to new default build assignee

Comment 20

[Steven Augart]

Bug #202594 has a patch waiting for review that fixes five uninitialized
variable warnings under gcc 3.2; four of them ones that were clearly potential
uses of uninitialized memory.

Comment 21

[(default assignee for unassigned bugs. does not receive bugmail)]

Mass reassign of Build/Config bugs to Leaf.

Comment 22

[Chase Phillips]

Mass reassign of open bugs for [e-mail] to [e-mail].

Comment 23

[J. Paul Reed [:preed]]

Mass re-assign of bugs that aren't on the build team radar, so bugs assigned to [e-mail] reflects reality.

If there is a bug you really think we need to be looking at, please *email* [e-mail] with a bug number and explanation.

Comment 24

[filidautore]

In lieu of the ongoing work of FAIL_ON_WARNINGS is this bus still relevant/wanted?

Comment 25

[Gregory Szorc [:gps]]

This has been usurped by bug 833405.