Closed
Bug 59659
Opened 24 years ago
Closed 16 years ago
Warnings of variables that 'might be used uninitialized in this function' in JS
Categories
(Core :: JavaScript Engine, defect, P3)
Core
JavaScript Engine
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 ----
Reporter | ||
Comment 1•24 years ago
|
||
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•24 years ago
|
||
cc'ing JS Engine group -
Comment 3•24 years ago
|
||
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
Reporter | ||
Comment 4•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
Who will investigate these warnings and propose to fix the ones that are not in performance-critical code? rogerl, mccabe, ...? /be
Keywords: js1.5
Comment 7•24 years ago
|
||
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
Comment 10•23 years ago
|
||
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 12•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
ugly
Comment 17•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•22 years ago
|
||
> 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•21 years ago
|
||
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
Comment 22•21 years ago
|
||
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•21 years ago
|
||
> 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.
Updated•21 years ago
|
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
Comment 24•21 years ago
|
||
> > 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 26•16 years ago
|
||
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: 16 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•