Closed Bug 179819 Opened 22 years ago Closed 12 years ago

Turn uninitialized warnings into errors

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: hjtoi-bugzilla, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

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...
Summary: Turn uninitlized warnings into errors → Turn uninitialized warnings into errors
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.
Priority: -- → P3
Target Milestone: --- → mozilla1.4beta
Some warnings are bogus -- particularly uninitialized variable warnings, and
compilers have major differences, even between gcc versions, in what they warn
about.
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.
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
Depends on: 132148
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.
well, does windows handle uninited var warnings better? If so, could we somehow
run a tinderbox with the equiv of -Werror set for that?
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.
*ahem* We've had a VC7 tinderbox on the ports page for weeks now.
Blocks: buildwarning
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?
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.
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.
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).
Depends on: 188124
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.
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
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.
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).
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.
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
Depends on: 188640
Depends on: 189563
Target Milestone: mozilla1.4beta → Future
Mass reassign to new default build assignee
Assignee: seawood → mozbugs-build
Priority: P3 → --
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.
Depends on: 202594
Mass reassign of Build/Config bugs to Leaf.
Assignee: mozbugs-build → leaf
Target Milestone: Future → ---
No longer depends on: 188124
No longer depends on: 126475
No longer depends on: 126457
No longer depends on: 59675
No longer depends on: 132141
No longer depends on: 59659
No longer depends on: 202594
No longer depends on: 59668
No longer depends on: 132145
Assignee: leaf → cmp
Product: Browser → Seamonkey
Mass reassign of open bugs for chase@mozilla.org to build@mozilla-org.bugs.
Assignee: chase → build
Mass re-assign of bugs that aren't on the build team radar, so bugs assigned to build@mozilla-org.bugs reflects reality.

If there is a bug you really think we need to be looking at, please *email* build@mozilla.org with a bug number and explanation.
Assignee: build → nobody
QA Contact: granrosebugs → build-config
Product: SeaMonkey → Core
In lieu of the ongoing work of FAIL_ON_WARNINGS is this bus still relevant/wanted?
This has been usurped by bug 833405.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.