Closed
Bug 179819
Opened 22 years ago
Closed 12 years ago
Turn uninitialized warnings into errors
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
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...
Updated•22 years ago
|
Summary: Turn uninitlized warnings into errors → Turn uninitialized warnings into errors
Comment 1•22 years ago
|
||
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
Comment 2•22 years ago
|
||
Some warnings are bogus -- particularly uninitialized variable warnings, and
compilers have major differences, even between gcc versions, in what they warn
about.
Comment 3•22 years ago
|
||
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•22 years ago
|
||
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
Reporter | ||
Comment 5•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
*ahem* We've had a VC7 tinderbox on the ports page for weeks now.
Updated•22 years ago
|
Blocks: buildwarning
Comment 9•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
Comment 18•22 years ago
|
||
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
Updated•22 years ago
|
Target Milestone: mozilla1.4beta → Future
Comment 19•22 years ago
|
||
Mass reassign to new default build assignee
Assignee: seawood → mozbugs-build
Priority: P3 → --
Comment 20•22 years ago
|
||
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.
Mass reassign of Build/Config bugs to Leaf.
Assignee: mozbugs-build → leaf
Target Milestone: Future → ---
Updated•20 years ago
|
Assignee: leaf → cmp
Updated•20 years ago
|
Product: Browser → Seamonkey
Comment 22•19 years ago
|
||
Mass reassign of open bugs for chase@mozilla.org to build@mozilla-org.bugs.
Assignee: chase → build
Comment 23•18 years ago
|
||
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
Updated•16 years ago
|
QA Contact: granrosebugs → build-config
Updated•15 years ago
|
Product: SeaMonkey → Core
Comment 24•12 years ago
|
||
In lieu of the ongoing work of FAIL_ON_WARNINGS is this bus still relevant/wanted?
Comment 25•12 years ago
|
||
This has been usurped by bug 833405.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•