Closed
Bug 87271
Opened 23 years ago
Closed 23 years ago
Useless Expression Elimination eliminating useful expression
Categories
(Core :: JavaScript Engine, defect, P3)
Tracking
()
VERIFIED
FIXED
mozilla0.9.4
People
(Reporter: gh221, Assigned: brendan)
Details
(Keywords: js1.5)
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
* TITLE/SUMMARY
Useless Expression Elimination eliminating useful expression.
* STEPS TO REPRODUCE
Compile and run the program at the bottom of this report.
* WORKAROUND
I haven't found any scripts to evaluate which have the issue other than plain
numbers. To convert a string containing a number into a number jsval, use
JS_ValueToNumber on a JSVAL_IS_STRING jsval containing your number. Thanks to
Brendan Eich for pointing that out.
* ACTUAL RESULT
Outputs "16", then "undefined", and finally "16".
* EXPECTED RESULT
Output of "16" three times.
* CONFIGURATIONS TESTED
- Windows 2000 (compiled with Visual C++ 6 SP5)
- Debian Linux (compiled with G++ 2.95.2, GNU Make 3.79.1)
* CODE AFFECTED BY BUG
JSRuntime* rt;
JSContext* cx;
JSObject* global;
JSClass global_class =
{
"global",JSCLASS_HAS_PRIVATE,
JS_PropertyStub,JS_PropertyStub,JS_PropertyStub,JS_PropertyStub,
JS_EnumerateStub,JS_ResolveStub,JS_ConvertStub,JS_FinalizeStub
};
static void testEval()
{
jsval rval;
if (JS_EvaluateScript(cx,global,"16",2,"",0,&rval))
printf("%s\n",JS_GetStringBytes(JS_ValueToString(cx,rval)));
}
static JSBool xjsx(JSContext* cx, JSObject* obj, uintN argc,
jsval* argv, jsval* rval)
{
testEval(); // Outputs "undefined" -- Bug
return JS_TRUE;
}
int main(int argc, char **argv)
{
jsval rval;
rt = JS_NewRuntime(0xFFFFFFFF);
cx = JS_NewContext(rt, 0x20000);
global = JS_NewObject(cx, &global_class, NULL, NULL);
JS_InitStandardClasses(cx, global);
JS_DefineFunction(cx, global, "x", ::xjsx, 0, 0);
testEval(); // Outputs 16
JS_EvaluateScript(cx,global,"x()",3,"",0,&rval);
testEval(); // Outputs 16
JS_DestroyContext(cx);
JS_DestroyRuntime(rt);
return 0;
}
Comment 1•23 years ago
|
||
Formally confirming bug for consideration -
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 2•23 years ago
|
||
The problem is that code-generation code looks at the top frame on the given cx
to decide whether an otherwise useless result might be wanted (by the caller of
JS_EvaluateScript, who passed &rval, e.g.). If, as in Grant's case, the top
frame is from a native function that calls JS_EvaluateScript, then the current
test for usefulness (http://lxr.mozilla.org/mozilla/source/js/src/jsemit.c#2148)
will result in false.
The compiler (parser and code generator) reuse topmost frames that seem suitable
rather than pushing their own, mostly-zeroes, compilation frames. If we pushed
such a frame, the bug would be fixed (because !cx->fp->fun would then be true).
Alternative ideas involving setting a bit in cx->fp->special seem like hacky and
risky to me. More in a bit; comments welcome.
/be
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla0.9.3
Assignee | ||
Comment 3•23 years ago
|
||
Targeting.
/be
Keywords: js1.5,
mozilla0.9.4
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Assignee | ||
Comment 4•23 years ago
|
||
One-line fix coming up.
/be
Assignee | ||
Comment 5•23 years ago
|
||
Assignee | ||
Comment 6•23 years ago
|
||
Review craved. The fix was easier than I had thought. The alternative of
pushing a dummy frame, besides being less efficient, requires care in seeding
the dummy frame's non-null members from the top-most frame on cx, so that code
such as that in LookupArgOrVar (jsemit.c) still works properly.
Grant, please test this patch and report back, if you can. Thanks.
khanson, rogerl: r= welcome.
shaver, jband: feel free to sr=!
/be
Keywords: patch
Reporter | ||
Comment 7•23 years ago
|
||
With this patch applied, the code given with the bug report now works. Also, I
note no negative effect with any of my code that relies on jseng. Looks like a
good fix. Thanks Brendan.
Comment 8•23 years ago
|
||
r=rogerl
Comment 9•23 years ago
|
||
sr=jband
Assignee | ||
Comment 10•23 years ago
|
||
Fix is in.
/be
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•