Closed
Bug 465483
Opened 16 years ago
Closed 16 years ago
TM: Type instability leads to undefined being added as a string instead of as a number
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.9.1
People
(Reporter: jruderman, Assigned: dvander)
References
Details
(Keywords: testcase, verified1.9.1)
Attachments
(1 file)
918 bytes,
patch
|
brendan
:
review+
sayrer
:
approval1.9.1b2+
|
Details | Diff | Splinter Review |
for each (i in [4, 'a', 'b', (void 0)]) print('' + (i + i)); Interpreter: last line is NaN JIT: last line is undefinedundefined
Comment 1•16 years ago
|
||
Andreas, JSVAL_BOXED breaks the inductive proof of type stability we depend on here and elsewhere. If we never guard on a particular type, JSOP_NEXTITER can return a value of any type, which unboxed onto the operand stack will flow into other ops (the + in (i + i) here) and be subject to the wrong operation (in this case stringify will convert undefined to string, whereas we should call js_BooleanOrUndefinedToNumber). /be
Comment 2•16 years ago
|
||
The JSVAL_BOXED design does handle a side-exit in the op that resumes with a boxed value on or near top of stack. It just does nothing for the case where that op does not side-exit, as in this bug's testcase. Must fix for 1.9.1/fx3.1. I'm not sure we need this for b2. Type stability is the norm in for-in loops. Cc'ing sayrer so he can help keep an eye on it. /be
Assignee: general → brendan
Severity: normal → major
Status: NEW → ASSIGNED
Flags: blocking1.9.1?
OS: Mac OS X → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1
Comment 3•16 years ago
|
||
I'm wrong about no guarding -- TraceRecorder::record_IteratorNextComplete does use unbox_jsval. But it's running in the wrong order with respect to the JSOP_ADD record calls! Investigating. /be
Comment 4•16 years ago
|
||
Of course, the loop does put nextiter after add: js> function f(){for each (i in [4, 'a', 'b', (void 0)]) print('' + (i + i));} js> dis(f) main: 00000: newinit 3 00002: zero 00003: int8 4 00005: initelem 00006: one 00007: string "a" 00010: initelem 00011: int8 2 00013: string "b" 00016: initelem 00017: int8 3 00019: zero 00020: void 00021: initelem 00022: endinit 00023: iter 3 00025: goto 49 (24) 00028: forname "i" 00031: callname "print" 00034: string "" 00037: name "i" 00040: name "i" 00043: add 00044: add 00045: call 1 00048: pop 00049: nextiter 00050: ifne 28 (-22) 00053: enditer 00054: stop This is the bug. The last iteration, which was not recorded, pushed a string on the stack, which add could assume was a string by the inductive proof. But the recorder sees nextiter after add and guards on undefined, which carries around the loop edge and into the unguarded add machine code. /be
Comment 5•16 years ago
|
||
Our type stability verification does not consider stack slots. But the stack is not empty on the for-in's ifne loop edge. The fix should not be to make the loop unstable, since the last iteration usually predicts the type of the next value the iterator return. We just need another guard, it seems to me. Andreas? /be
Comment 6•16 years ago
|
||
It so happens that the recorder sees the instability in this case, so it should abort. But if it saw the next value had the same type as the previous then it could do better. To see the previous and next values is hard because the stack slot for the value is not retained throughout the loop -- but it does live across the loop edge. The place to look is in TraceRecorder::record_IteratorNextComplete. It has the next value, undefined, on the stack. So if it could look upstream far enough, it could see the string coming off the stack at the loop header (flowing into the forname, offset 28 in comment 4) and abort or guard. /be
Assignee | ||
Comment 7•16 years ago
|
||
Brendan asked me to steal - coercing void to string isn't safe, and is a hold over from the NaN coercion days that multitrees obsoleted.
Assignee: brendan → danderson
Attachment #348884 -
Flags: review?(brendan)
Comment 8•16 years ago
|
||
Comment on attachment 348884 [details] [diff] [review] proposed fix I hope this is the last of it -- I didn't check (please double-check). We should take this for b2, it's safe and sound. Without it we may crash on taking undefined as a string (not stringifying it, actually dereferencing it). /be
Attachment #348884 -
Flags: review?(brendan)
Attachment #348884 -
Flags: review+
Attachment #348884 -
Flags: approval1.9.1b2?
Updated•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Updated•16 years ago
|
Attachment #348884 -
Flags: approval1.9.1b2? → approval1.9.1b2+
Assignee | ||
Comment 9•16 years ago
|
||
Pushed fix as changeset: http://hg.mozilla.org/tracemonkey/rev/19c01c290419
Comment 10•16 years ago
|
||
Pushed to m-c as well. Watch your step when merging (not sure how that works): http://hg.mozilla.org/mozilla-central/rev/829fa2d1e9dc
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 11•16 years ago
|
||
Checking in js1_8/regress/regress-465483.js; /cvsroot/mozilla/js/tests/js1_8/regress/regress-465483.js,v <-- regress-465483.js initial revision: 1.1 done
Flags: in-testsuite+
Flags: in-litmus-
Comment 12•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/d98fcf0fde87
Updated•16 years ago
|
Keywords: fixed1.9.1
Comment 13•15 years ago
|
||
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•