482594 - TM: followup work for support String(v) -- String constructor called as a converter

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

TraceRecorder::stringify is fallible but also returns null for non-primitive t to tell its caller to abort. We should fix this.

On IRC we discussed making it call the String constructor as a function to convert (with valueOf hint "string") but that helps the only caller who might pass a non-primitive, which is the String called as a function case just added for bug 482349.

Also, we should handle null => "null".

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 366658 [details] [diff] [review]
fix

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp

>-        /* We can't stringify objects here (use imacros instead), just return NULL. */
>-        return NULL;
>+        /* Callers must deal with non-primitive (real object) values by calling an imacro.
>+           We don't try to guess about which imacro, with what valueOf hint, here. */
>+        JS_ASSERT(JSVAL_IS_NULL(v));
>+        return INS_CONSTPTR(cx->runtime->atomState.nullAtom);

Good ol' SpiderMonkey major comment style, please.


>@@ -6709,7 +6711,7 @@ TraceRecorder::functionCall(bool constru
>                 return call_imacro(call_imacros.String);
>             LIns *i = stringify(v);
>             if (!i)
>-                ABORT_TRACE("can't stringify value");
>+                return false;

We can't actually hit this, can we?  Object's taken care of by imacro, integers/doubles/integers/strings/integers/booleans/integers taken care of by stringify itself, and can insCall return NULL by itself?  I'm guessing the answer is no as I'm pretty sure this would fail everywhere otherwise; please enlighten me if it is otherwise.  This should be an assert that stringify didn't return NULL if my understanding is correct.

With a trace-test to verify this works, with recorderAborted jitstats to verify non-aborting here, r=me.

Comment 3

[Brendan Eich [:brendan]]

Attachment: fix, v2

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix, v2

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 366683 [details] [diff] [review]
fix, v2

>diff --git a/js/src/trace-test.js b/js/src/trace-test.js

>+function testNullToString()
>+{
>+  var a = [];
>+  for (var i = 0; i < 10; i++)
>+    a.push(String(null));
>+  for (i = 0; i < 10; i++) {
>+    var t = typeof a[i];
>+    if (t != "string")
>+      a.push(t);
>+  }
>+  return a.join(",");
>+}
>+testNullToString.expected = "null,null,null,null,null,null,null,null,null,null";
>+testNullToString.jitstats = {
>+  recorderStarted: 2,
>+  sideExitIntoInterpreter: 2
>+};
>+test(testNullToString);

This wants a recorderAborted: 0 as well; two side-exits should mean no aborts here, but better to be precise than miss a regression somehow.  Otherwise all looks good.

Comment 6

[Brendan Eich [:brendan]]

In tm:

http://hg.mozilla.org/tracemonkey/rev/89b61367af4c

/be

Comment 7

[Brendan Eich [:brendan]]

sayrer, did this miss the m-c bus?

/be

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/89b61367af4c

Comment 9

[Bob Clary [:bc] (inactive)]

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

Comment 10

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cee7a22c175d

Comment 11

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2

Comment 12

[Bob Clary [:bc] (inactive)]

cvsroot/mozilla/js/tests/js1_8_1/trace/trace-test.js,v  <--  trace-test.js
new revision: 1.14; previous revision: 1.13

/cvsroot/mozilla/js/tests/shell.js,v  <--  shell.js