Closed Bug 52234 Opened 24 years ago Closed 22 years ago

Kill build warnings in javascript source

Categories

(Core :: JavaScript Engine, defect, P3)

x86
Linux
defect

Tracking

()

VERIFIED INVALID

People

(Reporter: jwbaker, Assigned: brendan)

References

()

Details

Attachments

(1 file)

This patch kills 30+ build warnings in mozilla/js/src.  I believe that these are
the correct resolutions for these warnings.  All of them are uninitialized
variable warnings, and all of the ones which I fixed were bogus.

There is one legitimate warning, where we are using a pointer which could be
garbage.  I have added a FIXME comment to the source in that place, because I
don't know the right way to fix it.  It is possible that we never crash there,
but the could should be more explicit.

If you are cc on this bug, that means that tinderbox blamed you.
Keywords: patch, review
I'm not very happy with having the software burn cycles because this particular 
compiler is not smart enough to see that code of the form...

int i;
if(foo)
  i = 1;
else
  i = 2;
bar(i);

...does not use any uninitialized variables.
I won't take this patch, but I feel bad writing that since you did work to come
up with it and attach it.  These warnings are bogus, as you wrote (if what you
meant was, these warning are false alarms).  What's more, the FIXME comment you
attached is not a bug: notice the || expression governing the preceding if-then
statement, and the early return at the end of the then block.  aobj will be set
if control reaches the successor block to the if-then.

I keep a close eye on warnings produced by gcc debug builds of JS, but I can't
abide the optimizer's crying wolf.  Any modern optimizing back end (SSA based)
would know better.

BTW, the patch in bug 49816 fixes the jsparse.c warnings, with conforming style
(the JS engine never uses foo=42, always putting spaces around operators) and in
the case of op = JSOP_NOP, doing that only in the default: case of the switch,
not in an initializer.

Let me know if I've missed anything here.  Thanks -- I'll mark this WONTFIX
tomorrow unless there's something truly broken.

/be
Assignee: rogerl → brendan
Urk, forgot to be clear that the hasDefault warning for jsparse.c is valid: an
empty JS switch would lead to a UMR at ok &= hasDefault; (my face is red).  But
as I wrote, the patch for bug 49816 fixes that.

/be
Status: NEW → ASSIGNED
The practical difference between char *this; and char *this = NULL is barely
even worth mentioning (1 extra byte of object code) in a product of this scope.

My goal with this patch is to go through the build log looking for legitimate
warnings and reducing the noise.  If you don't want to commit the hush fixes,
that's fine with me, but I expect to find at least a few real bugs in the mix,
like the one in jsparse.c.

BTW Brendan you are right that my FIXME is a false alarm, but there's something
wrong when it takes someone > 5 minutes to consider the result of an if
statement.
just a question: in your experience, jband, would a compiler ever optimize out an 

initialization at the point of declaration, had there been one, in your example 

above?  Plainly it's not needed; I've seen optimizers elide assignments into dead 

variables ...

jwbaker: we have some AI in the tinderbox warning filter that takes out known to 
be useless warnings (such as those about constant expressions always true or 
false in the condition of a ?: expression, namely a JS_ASSERT, PR_ASSERT, or 
similar -- sometimes you need such runtime assertions because the C preprocessor 
can't handle sizeof).  I would hate to teach this filter about a long list of 
false alarms, but if there's a way to separate the wheat from the chaff 
reliably....

The JS engine is one of those places where every cycle counts, not that it has 
been optimized much (over the years, the emphasis has been on completeness and 
extensibility, not performance; but now there is a fair deal of performance 
pressure).

But performance quibbling aside, the main point is that I think gcc needs to do 
a better job, and I don't view it as my job to as "how high" when it says "jump" 
(but I do pay attention, thank you for the jsparse.c reminder -- I'd already 
found that via my own optimized gcc builds).

scc: if the optimizer is claiming that foo may not be set at a use point, then I 
doubt it will have the wits to remove the unnecessary initialization, were you 
to add one.  Again I say gcc has a bug, not (necessarily) any source that it 
whines about in the way this bug reports.

/be
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → REMIND
Marking Verified -
Status: RESOLVED → VERIFIED
REMIND is deprecated per bug 35839.
Status: VERIFIED → REOPENED
Resolution: REMIND → ---
If this is gcc's silliness and not ours, INVALID.
Status: REOPENED → RESOLVED
Closed: 24 years ago22 years ago
Resolution: --- → INVALID
Marking Verified Invalid -
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: