478968 - TM: equalityHelper can call toString or valueOf erroneously when tracing obj == undefined

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

obj = {toString: function() { throw 0; }};
for (i = 0; i < 5; i++)
  print(""+ (obj == undefined));

With the JIT, this throws.  Without, obj.toString is never called.  The latter is correct.

Treehydra very cleverly detected this by defining a toString method that always throws. :-P

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch and tests

Comments on wording of the side-exit documentation appreciated; I'm still not completely clear on their exact uses, and the wording probably could use some help in any case.  Will file a followup bug to deal with the high side-exit count, but would rather get this fixed now and deal with revealed inefficiencies later -- wrong is worse than slow.

I'm starting to wonder whether bug 471214 or something similar is biting all sorts of loopy code (particularly loopy code that has a style similar to the style of code I write), because I'm seeing quite a few loops lately that are exiting every go-around.  Wouldn't surprise me, although given that I seem to see side exits on different ops each time they may be quite different.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 362841 [details] [diff] [review]
Patch and tests

What are the jitstats with and without the patch?

/be

Comment 3

[Jeff Walden [:Waldo]]

Attachment: v2, with better tests

Hm, actually I think the original comment had it right.  If we have obj == undefined, we converted undefined=>NaN and Number(obj), but nothing == NaN, so it's only possible to detect error by side effects or abrupt termination.  In this patch we detect that by incrementing a count as a side effect (and checking returns just to be sure), and here we fail pre-patch and pass after.  The previous patch had a rather egregious typo of NaN for undefined, so its second test wasn't testing that the patch worked.

The jitstats after are in the patch.  Pre-patch, the side-exit count for testThrowingObjectEqUndefined is 0 as we hit the throw before we can execute a trace.  The side-exit count for testConvertibleObjectEqUndefined is 1 before because we always convert the pseudo-boolean and always call valueOf on the object and always follow the same execution path, only executing when the loop ends.  After, it's 2 because we have to side-exit to fill in the |undefined| case once, then it's smooth sailing until the loop ends.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 362995 [details] [diff] [review]
v2, with better tests

>+            bool isBool = !JSVAL_IS_VOID(l);
>+            guard(!isBool,
>+                  lir->ins2(LIR_eq, l_ins, INS_CONST(JSVAL_TO_PSEUDO_BOOLEAN(JSVAL_VOID))),
>+                  BRANCH_EXIT);
>+            if (isBool) {

Nit: would isVoid be clearer? Then you would avoid two negations via ! *and* the guard first arg and LIR-combo in second arg would use like terms (isVoid, LIR_eq(l_ins, ...VOID)).

> enum ExitType {
>+    /*
>+     * An exit from a trace because a condition relied upon at recording time
>+     * no longer holds; however, if that condition had been known to be
>+     * otherwise at record time, further tracing of an alternate path of
>+     * execution could have happened.
>+     */
>     BRANCH_EXIT,

This doesn't motivate the b-word. How about "An exit at a possible branch-point in the trace at which to attach a future secondary trace. Therefore the recorder must generate different code to handle the other outcome of the branch condition from the primary trace's outcome."

>@@ -4350,6 +4350,59 @@ function testRegexpGet() {
> testRegexpGet.expected = "hi,hi,hi,hi,hi";
> test(testRegexpGet);
> 
>+function testThrowingObjectEqUndefined()
>+{
>+  try
>+  {

Is this trace-test.js style, or is it an eclectic place (or a sacked Rome)? I mean two-space basic offset and brace on its own line?

r=me with nits.

/be

Comment 5

[Brendan Eich [:brendan]]

With baking I can make wanted case easily. Not sure about blocking but we are not at the 11th hour.

/be

Comment 6

[Jeff Walden [:Waldo]]

(In reply to comment #4)
> Nit: would isVoid be clearer?

Yeah, I think so, changed.

> How about "An exit at a possible branch-point in the trace at which to
> attach a future secondary trace. Therefore the recorder must generate
> different code to handle the other outcome of the branch condition from
> the primary trace's outcome."

Works for me, changed.

> Is this trace-test.js style, or is it an eclectic place (or a sacked Rome)? I
> mean two-space basic offset and brace on its own line?

It's the style I like most, common to nearly all JS tests I've ever written, trace-test or otherwise, and nobody's ever complained about tests having different style.  Also, if anything, it's more an unmolested Parthenon than a destroyed Rome.  :-P

Comment 7

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/a70e1f50ce17

Comment 8

[Brendan Eich [:brendan]]

I spotted some 4-space js-basic-offset invasion of your Parthenon.

trace-test.js is really a mix, altogether.

/be

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

The original author of trace-test.js (me) believes in 4-space, but my Emacs is a bit of an apostate at times.

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a70e1f50ce17

Comment 11

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/74e0721ef1c4

Comment 12

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/1c16405d7d6c
/cvsroot/mozilla/js/tests/js1_8_1/trace/trace-test.js,v  <--  trace-test.js
new revision: 1.12; previous revision: 1.11

Comment 13

[Bob Clary [:bc] (inactive)]

verified 1.9.1, 1.9.2 based on testThrowingObjectEqUndefined and testConvertibleObjectEqUndefined