59659 - Warnings of variables that 'might be used uninitialized in this function' in JS

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P3)

Description

[Rich Burridge]

For more details on this problem, see:

http://bugzilla.mozilla.org/show_bug.cgi?id=59652

This bug is just for the warnings in various source files in the JavaScript
Engine module:

jsapi.c:985: warning: `entry' might be used uninitialized in this function
----
jsdhash.c:267: warning: `size' might be used uninitialized in this function
jsdhash.c:267: warning: `capacity' might be used uninitialized in this function
----
jsdtoa.c:1892: warning: `mlo' might be used uninitialized in this function
----
jsemit.c:698: warning: `beq' might be used uninitialized in this function
jsemit.c:699: warning: `pn3' might be used uninitialized in this function
----
jsfun.c:1195: warning: `aobj' might be used uninitialized in this function
----
jsinterp.c:1193: warning: `sp' might be used uninitialized in this function
jsinterp.c:1197: warning: `cs' might be used uninitialized in this function
jsinterp.c:1201: warning: `id' might be used uninitialized in this function
jsinterp.c:1216: warning: `tracefp' might be used uninitialized in this
function----
e_asin.c:110: warning: `t' might be used uninitialized in this function
----
e_exp.c:143: warning: `hi' might be used uninitialized in this function
e_exp.c:143: warning: `lo' might be used uninitialized in this function
e_exp.c:144: warning: `k' might be used uninitialized in this function
----
e_rem_pio2.c:124: warning: `z' might be used uninitialized in this function
----
e_sqrt.c:132: warning: `z' might be used uninitialized in this function
----
k_cos.c:105: warning: `qx' might be used uninitialized in this function
----
jsobj.c:532: warning: `valcnt' might be used uninitialized in this function
jsobj.c:2128: warning: `proto' might be used uninitialized in this function
----
jsopcode.c:807: warning: `forelem_done' might be used uninitialized in this
function
----

Comment 1

[Rich Burridge]

More occurances:

xpccomponents.cpp:1890: warning: `nsresult res' might be used uninitialized in
this function
----
xpcconvert.cpp:1007: warning: `nsresult rv' might be used uninitialized in this
xpcconvert.cpp:1008: warning: `double number' might be used uninitialized in
this function
xpcconvert.cpp:1288: warning: `enum XPCConvert::JSArray2Native(JSContext *, void
**, long int, unsigned int, unsigned int, const nsXPTType &, int, const nsID *,
uintN *)::CleanupMode cleanupMode' might be used uninitialized in this
functionxpcconvert.cpp:1292: warning: `JSUint32 initedCount' might be used
uninitialized in this function
----
xpcwrappedjsclass.cpp: In method `nsresult
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS *, short unsigned int, const
nsXPTMethodInfo *, nsXPTCMiniVariant *)':xpcwrappedjsclass.cpp:487: warning:
`jsval * stackbase' might be used uninitialized in this function
xpcwrappedjsclass.cpp:495: warning: `void (* older)(JSContext *, const char *,
JSErrorReport *)' might be used uninitialized in this function
xpcwrappedjsclass.cpp:496: warning: `JSBool success' might be used uninitialized
in this function
----
xpcwrappednativeclass.cpp:545: warning: `uint8 paramCount' might be used
uninitialized in this function
xpcwrappednativeclass.cpp:547: warning: `uint8 vtblIndex' might be used
uninitialized in this function
----
jsj.c:681: warning: `jEnv' might be used uninitialized in this function
jsj.c:730: warning: `java_vm' might be used uninitialized in this function
----
jsj_JSObject.c:253: warning: `java_wrapper_obj' might be used uninitialized in
this function
----

Comment 2

[Phil Schwartau]

cc'ing JS Engine group -

Comment 3

[Brendan Eich [:brendan]]

This comes up from time to time.  Rather than always initialize a set of
dependent variables, we let other variables' control dependencies govern the
definition and use of the dependent variables.  The jsapi.c case is easy to
analyze.  I don't plan to fix it.

Roger, could you give a look too, and if you think all of these are cases of the
compiler not standing on its head and spinning around enough to see that the
variables are all used only if set first, please close as INVALID.

/be

Comment 4

[Rich Burridge]

Brendan, how about taking sympathy on OEM's that have to use a compiler
that perhaps doesn't do the right thing? All you have to do is initialize
the variable to zero, and you prevent a potential problem.

Comment 5

[Brendan Eich [:brendan]]

The way jsdhash.c works, I'm not happy about two more assignments of 0 (to size
and capacity) in the LOOKUP case.  There is no real problem, and the potential
problem you allude to (someone adding a use of size or capacity in the !change,
e.g., the LOOKUP case) is a case where someone has added a bug.  The "fix" adds
two register clears, or something less efficient if size and capacity don't get
allocated into registers, for no good reason.

Say someone adds code to set change non-zero in the LOOKUP case, or adds another
case that sets change non-zero but does not initialize size or capacity.  Such a
modification is a bug that would not be caught by these warnings, whether we
"fix" the code to avoid the warnings or not -- either we have the warnings, the
bad change is made, the warnings persist; or we set size and capacity to 0 up
front, the warnings go away, and the bad uses of size and capacity load 0 and do
something silly, like try to shrink the hash table to a zero length.

Sorry to dwell on this jsdhash.c case.  I think if there are lots of "extra"
cycles involved in the function already (jsdtoa.c comes to mind), then why not
initialize early and defend against later maintainers adding a bad use?  I just
don't think we should "fix" all these by brain-off initializing to 0, and I
think it's perfectly fine, at the end of the day, to have some warnings with
your compiler for cases where we don't want to deoptimize the code to silence
the warning.

/be

Comment 6

[Brendan Eich [:brendan]]

Who will investigate these warnings and propose to fix the ones that are not in
performance-critical code?  rogerl, mccabe, ...?

/be

Comment 7

[John Bandhauer]

Changed the summary to reflect what the warnings really say.

Comment 8

[Colin Blake]

*** Bug 63600 has been marked as a duplicate of this bug. ***

Comment 9

[Patrick C. Beard]

Marking future.

Comment 10

[Aleksey Nogin]

Currently (http://tinderbox.mozilla.org/SeaMonkey/warn1010759460.5770.html) JS
code produces the following warnings:

js/jsd/jsd_step.c:117
 `hookresult' might be used uninitialized in this function

js/src/fdlibm/e_asin.c:110
 `t' might be used uninitialized in this function

js/src/fdlibm/e_exp.c:143
 `hi' might be used uninitialized in this function
 `lo' might be used uninitialized in this function

js/src/fdlibm/e_exp.c:144
 `k' might be used uninitialized in this function

js/src/fdlibm/e_rem_pio2.c:124
 `z' might be used uninitialized in this function

js/src/fdlibm/e_sqrt.c:132
 `z' might be used uninitialized in this function

js/src/fdlibm/k_cos.c:105
 `qx' might be used uninitialized in this function

js/src/jsdtoa.c:1897
 `mlo' might be used uninitialized in this function

js/src/jsinterp.c:1218
 `sp' might be used uninitialized in this function

js/src/jsinterp.c:1226
 `id' might be used uninitialized in this function

js/src/jsobj.c:2473
 `userid' might be used uninitialized in this function

js/src/jsobj.c:2474
 `getter' might be used uninitialized in this function
 `setter' might be used uninitialized in this function

js/src/jsobj.c:589
 `valcnt' might be used uninitialized in this function

js/src/liveconnect/jsj.c:698
 `jEnv' might be used uninitialized in this function

js/src/liveconnect/jsj.c:753
 `java_vm' might be used uninitialized in this function

js/src/liveconnect/jsj_JSObject.c:253
 `java_wrapper_obj' might be used uninitialized in this function

js/src/xpconnect/src/xpccomponents.cpp:1670
 `nsresult res' might be used uninitialized in this function

js/src/xpconnect/src/xpcconvert.cpp:1078
 `nsresult rv' might be used uninitialized in this function

js/src/xpconnect/src/xpcconvert.cpp:1079
 `double number' might be used uninitialized in this function

js/src/xpconnect/src/xpcconvert.cpp:1403
 `enum XPCConvert::JSArray2Native(XPCCallContext &, void **, long int, unsigned
int, unsigned int, const nsXPTType &, int, const nsID *, uintN *)::CleanupMode
cleanupMode' might be used uninitialized in this function

js/src/xpconnect/src/xpcconvert.cpp:1407
 `JSUint32 initedCount' might be used uninitialized in this function

js/src/xpconnect/src/xpcconvert.cpp:557
 `const PRUnichar * chars' might be used uninitialized in this function

js/src/xpconnect/src/xpcconvert.cpp:560
 `PRUint32 length' might be used uninitialized in this function

js/src/xpconnect/src/xpcwrappedjsclass.cpp:804
 `jsval * stackbase' might be used uninitialized in this function

js/src/xpconnect/src/xpcwrappedjsclass.cpp:813
 `void (* older)(JSContext *, const char *, JSErrorReport *)' might be used
uninitialized in this function

js/src/xpconnect/src/xpcwrappedjsclass.cpp:814
 `JSBool success' might be used uninitialized in this function

js/src/xpconnect/src/xpcwrappednative.cpp:1621
 `uint8 paramCount' might be used uninitialized in this function

js/src/xpconnect/src/xpcwrappednativeinfo.cpp:309
 `struct JSString * str' might be used uninitialized in this function

js/src/xpconnect/src/xpcwrappednativejsops.cpp:256
 `const char * name' might be used uninitialized in this function

js/src/xpconnect/src/xpcwrappednativejsops.cpp:897
 `class XPCWrappedNative * oldResolvingWrapper' might be used uninitialized in
this function

js/src/xpconnect/src/xpcwrappednativeproto.cpp:172
 `class ClassInfo2WrappedNativeProtoMap * map' might be used uninitialized in
this function

js/src/xpconnect/src/xpcwrappednativeproto.cpp:173
 `struct XPCLock * lock' might be used uninitialized in this function

Comment 11

[Phil Schwartau]

Reassigning to Kenton -

Comment 12

[John Bandhauer]

cc'ing dbradley in case he wants to have a look at the xpconnect set of
warnings. (note that these days you can see a lot of this sort of warnings on
the 'gcc295' tinderbox build). Like brendan, I continue to hold te opinion that
these compilers are just being stupid. Every time I've looked into this sort of
thing I've found something of the form:

int foo;
if(x)
  foo = y;
else
  foo = z;
bar(foo);
A reasonably smart compiler could easily see that foo can't help but be
initilized. *Other* compilers warn us of legitimate cases where vars are used
uninitialized and we *always* keep that problem under control.

I don't buy the argument that we should double initilize or rework our logic
just to quiet warnings from a stupid compiler. Can this *particular* warning be
disabled with a switch without disabling similar but more useful warnings?

Comment 13

[David Bradley]

I agree, I've always thought this was rather lame on the compiler's part. It
waters down a very useful warning.

I've run XPConnect at -W4 under VC++ and took a look at a number of those
warnings some time ago. I found the ones I looked at to be of the nature jband
describes. I don't think I got through all of them, but did look at many.

I know for VC++ you'd could put in a pragma around the code. I'm not sure
putting in a bunch of platform specific pragmas with corresponding ifdef's is
all that attractive, either.

Comment 14

[Brendan Eich [:brendan]]

Better to suppress warnings outside of using source pragmas (e.g., by filtering
warning output, which goes to the tinderbox wall of warning shame).  Slamm used
to do that for us when he was tending the t.w.o.w.s.  I don't think we should
ugly up the source with pragmas.

/be

Comment 15

[David Bradley]

Would a comment before or on the line with the warning be too intrusive. An
example of what I'm thinking of is below. I've toyed around with a filter
program that filter based upon such a scheme.

int x;
switch (y){
...
}

// $IGNORE_UNINITIALIZED_VARIABLE
return x;

Comment 16

[John Bandhauer]

ugly

Comment 17

[David Bradley]

Are you against modifying source or just this particular style. If it's just the
style we could use a simpler tag on the same line that would turn off the
warning for the line.return x; // $WR_OKTurning off a warning at the compiler
level could hide valid cases. Maybe our reviews are enough to offset that.

Comment 18

[Brendan Eich [:brendan]]

Foo, let's fix higher priority stuff and spare the source code such graffiti.  I
guess I'm just dismayed that some compilers can't report warnings well.  I still
think we can post-process without magic comments, but admit that's fragile.

/be

Comment 19

[David Bradley]

Yes compilers should be a little smarter by now about such things. On the other
hand, I suspect their busy trying to get them standard conformant ;-)

Comment 20

[Aleksey Nogin]

> js/src/fdlibm/e_exp.c:143
>  `hi' might be used uninitialized in this function
>  `lo' might be used uninitialized in this function
>
> js/src/fdlibm/e_exp.c:144
>  `k' might be used uninitialized in this function

timeless brough to my attention the path
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/fdlibm/e_exp.c&mark=143-150,176,178,182-183,185#133
that would cause the above to be indeed used uninitialized. However there is the
following weird piece of code:

176     else if(hx < 0x3e300000)  {	/* when |x|<2**-28 */
177       if(really_big+x>one) return one+x;/* trigger inexact */
178     }

that looks really weird. What is that "if" in line 177 doing here? Can it ever
be false? If we are to believe the "|x|<2**-28", then obviously "1.0e+300 + x"
is always bigger than 1.0. And if that "really_big+x>one" will ever be false,
then things will indeed end up being used uninitialized...

Comment 21

[Aleksey Nogin]

7 more warnings were added by bug 85721 commit. Now the full list of
"uninitialized" warnings in JS is:

js/src/fdlibm/e_asin.c:110
 `t' might be used uninitialized in this function

js/src/fdlibm/e_exp.c:143
 `hi' might be used uninitialized in this function
 `lo' might be used uninitialized in this function

js/src/fdlibm/e_exp.c:144
 `k' might be used uninitialized in this function

js/src/fdlibm/e_rem_pio2.c:124
 `z' might be used uninitialized in this function

js/src/fdlibm/e_sqrt.c:132
 `z' might be used uninitialized in this function

js/src/fdlibm/k_cos.c:105
 `qx' might be used uninitialized in this function

js/src/jsdtoa.c:1018
 `d' might be used uninitialized in this function

js/src/jsdtoa.c:1257
 `rv0' might be used uninitialized in this function

js/src/jsdtoa.c:987
 `a' might be used uninitialized in this function

js/src/jsinterp.c:1346
 `sp' might be used uninitialized in this function

js/src/jsinterp.c:1354
 `id' might be used uninitialized in this function

js/src/jsregexp.c:1836
 `rangeStart' might be used uninitialized in this function

js/src/jsregexp.c:2270
 `result' might be used uninitialized in this function

js/src/jsregexp.c:2902
 `parsub' might be used uninitialized in this function

js/src/jsregexp.c:425
 `errPos' might be used uninitialized in this function

js/src/jsregexp.c:426
 `parenIndex' might be used uninitialized in this function

js/src/jsregexp.c:430
 `op' might be used uninitialized in this function

js/src/jsregexp.c:661
 `rangeStart' might be used uninitialized in this function

js/src/xpconnect/src/xpcconvert.cpp:1304
 `double number' might be used uninitialized in this function

js/src/xpconnect/src/xpcconvert.cpp:1637
 `JSUint32 initedCount' might be used uninitialized in this function

js/src/xpconnect/src/xpcconvert.cpp:665
 `const PRUnichar*chars' might be used uninitialized in this function

js/src/xpconnect/src/xpcconvert.cpp:668
 `PRUint32 length' might be used uninitialized in this function

js/src/xpconnect/src/xpcwrappedjsclass.cpp:945
 `jsval*stackbase' might be used uninitialized in this function

js/src/xpconnect/src/xpcwrappednativeinfo.cpp:298
 `JSString*str' might be used uninitialized in this function

js/src/xpconnect/src/xpcwrappednativejsops.cpp:256
 `const char*name' might be used uninitialized in this function

js/src/xpconnect/src/xpcwrappednativejsops.cpp:900
 `XPCWrappedNative*oldResolvingWrapper' might be used uninitialized in this function

js/src/xpconnect/src/xpcwrappednativeproto.cpp:172
 `ClassInfo2WrappedNativeProtoMap*map' might be used uninitialized in this function

js/src/xpconnect/src/xpcwrappednativeproto.cpp:173
 `XPCLock*lock' might be used uninitialized in this function

Comment 22

[Brendan Eich [:brendan]]

Isn't this a dup?

I'm not going to change the jsinterp.c code to work around bogus warnings.

The jsregexp.c code rogerl wrote added some warnings, one of which indicated
real troublue.  I've fixed that in the patch in bug 223273.  The remaining
warnings are annoying, and I'll probably make them go away with yet another
regexp re-do (this time restoring the old Nav 4.x-era bytecoded NFA I wrote,
revamping it to do the Perl5-ish stuff in ECMA-262 Edition 3, with no loss of
performance from 4.x days).

I'm inclined to WONTFIX this bug.

/be

Comment 23

[Aleksey Nogin]

> Isn't this a dup?

Dup of what?

> I'm not going to change the jsinterp.c code to work around bogus warnings.
...
> I'm inclined to WONTFIX this bug.

This bug is about all such warnings in all of the js code, not just the
jsinterp.c file.

Comment 24

[Brendan Eich [:brendan]]

> > Isn't this a dup?
> 
> Dup of what?

Another bug I recalled.  If Serge can't find it, perhaps it was closed already.

> > I'm not going to change the jsinterp.c code to work around bogus warnings.
> ...
> > I'm inclined to WONTFIX this bug.
> 
> This bug is about all such warnings in all of the js code, not just the
> jsinterp.c file.

Nice job quoting me selectively.  I clearly was listing other js/src/*.c files
and talking about how warnings there were now all benign, although annoying.

Warnings about files in js/src/xpconnect/src are *not* appropriate for a bug
filed against "JavaScript Engine" -- clue: XPConnect.  Comment 0 mentioned only
js/src/*.c files, but comment 21 crosses the line.

This bug is still on the verge of being WONTFIXed by me.  Selective quoting of
my words, and lumping xpconnect warnings into a js bug, don't make it safer from
that fate.

/be

Comment 25

[Bob Clary [:bc] (inactive)]

-> default qa

Comment 26

[Robert Sayre]

I don't get any of these on today's trunk. Please refile on specific warnings if you do, rather than keep this ancient, noisy bug open.