Closed Bug 59659 Opened 25 years ago Closed 17 years ago

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

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED WORKSFORME
Future

People

(Reporter: rich.burridge, Unassigned)

References

Details

(Keywords: js1.5)

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 ----
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 ----
cc'ing JS Engine group -
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
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.
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
Blocks: 59652
Who will investigate these warnings and propose to fix the ones that are not in performance-critical code? rogerl, mccabe, ...? /be
Keywords: js1.5
Changed the summary to reflect what the warnings really say.
Summary: Occurances of uninitialized variables being used before being set. → Warnings of variables that "might be used uninitialized" in JS
*** Bug 63600 has been marked as a duplicate of this bug. ***
Marking future.
Target Milestone: --- → Future
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
Reassigning to Kenton -
Assignee: rogerl → khanson
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?
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.
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
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;
ugly
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.
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
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 ;-)
Depends on: 53593
Depends on: 132148
> 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...
Blocks: 149801
Depends on: 189563
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
Assignee: khanson → general
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
> 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.
No longer blocks: 179819
Summary: Warnings of variables that "might be used uninitialized" in JS → Warnings of variables that 'might be used uninitialized in this function' in JS
> > 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
-> default qa
QA Contact: pschwartau → general
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.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.