TM: Type instability leads to undefined being added as a string instead of as a number

VERIFIED FIXED in mozilla1.9.1

Status

()

Core
JavaScript Engine
P1
major
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: dvander)

Tracking

(Blocks: 1 bug, {testcase, verified1.9.1})

Trunk
mozilla1.9.1
testcase, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
for each (i in [4, 'a', 'b', (void 0)]) print('' + (i + i));

Interpreter: last line is NaN

JIT: last line is undefinedundefined
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
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
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
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
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
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
Created attachment 348884 [details] [diff] [review]
proposed fix

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 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

10 years ago
Flags: blocking1.9.1? → blocking1.9.1+

Updated

10 years ago
Attachment #348884 - Flags: approval1.9.1b2? → approval1.9.1b2+
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
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Blocks: 465453

Comment 11

10 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-
Keywords: fixed1.9.1

Comment 13

9 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.