Closed Bug 482349 Opened 15 years ago Closed 15 years ago

TM: support String(v) -- String constructor called as a converter

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla1.9.1

People

(Reporter: brendan, Assigned: gal)

References

Details

(Keywords: verified1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(3 files)

See bug 460050 comment 14. Easy patch.

/be
Flags: wanted1.9.1?
Flags: wanted1.9.1? → wanted1.9.1+
Attached patch patchSplinter Review
Attachment #366436 - Flags: review?(jwalden+bmo)
Comment on attachment 366436 [details] [diff] [review]
patch

>diff --git a/js/src/imacros.jsasm b/js/src/imacros.jsasm
>+.igroup call JSOP_CALL
>+
>+    .imacro String                          # String this obj
>+        dup                                 # String this obj obj
>+        dup                                 # String this obj obj obj
>+        getprop valueOf                     # String this obj obj valueOf
>+        ifprimtop 1                         # String this obj obj valueOf
>+        swap                                # String this obj valueOf obj
>+        string string                       # String this obj valueOf obj "string"
>+        call 1                              # String this obj rval
>+        ifprimtop 3                         # String this obj rval
>+        goto 2
>+1:      pop                                 # String this obj obj
>+2:      pop                                 # String this obj
>+        callprop toString                   # String this toString obj
>+        call 0                              # String this obj

The last here should be rval, not obj.

>+        primtop                             # String this rval
>+        goto 4                              # String this rval
>+3:      swap                                # String this rval obj
>+        pop                                 # String this rval
>+4:      call 1                              # str
>+        stop                                # str
>+    .end
>+
>+.end

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -4399,17 +4399,17 @@ TraceRecorder::monitorRecording(JSContex
>             js_Disassemble1(cx, cx->fp->script, cx->fp->regs->pc,             \
>                             (cx->fp->imacpc)                                  \
>                             ? 0                                               \
>                             : cx->fp->regs->pc - cx->fp->script->code,        \
>                             !cx->fp->imacpc, stdout);)                        \
>         flag = tr->record_##x();                                              \
>         if (x == JSOP_ITER || x == JSOP_NEXTITER || x == JSOP_APPLY ||        \
>             x == JSOP_GETELEM || x == JSOP_SETELEM || x== JSOP_INITELEM ||    \
>-            JSOP_IS_BINARY(x) || JSOP_IS_UNARY(x) ||                          \
>+            x == JSOP_CALL || JSOP_IS_BINARY(x) || JSOP_IS_UNARY(x) ||        \
>             JSOP_IS_EQUALITY(x)) {                                            \
>             goto imacro;                                                      \
>         }                                                                     \

We should add a field to the OPDEFs in jsopcode.tbl to record this, rather than bloating this ever more as we create more and more imacros.  File a followup bug?


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

>+function testString() {
>+    var q;
>+    for (var i = 0; i <= RUNLOOP; ++i) {
>+        q = [];
>+        q.push(String(void 0));
>+        q.push(String(true));
>+        q.push(String(5));
>+        q.push(String(5.5));
>+        q.push(String("5"));
>+        q.push(String([5]));
>+    }
>+    return q.join(",");
>+}
>+testString.expected = "undefined,true,5,5.5,5,5";
>+testString.jitstats = {
>+    recorderStarted: 1
>+};
>+test(testString);

Doesn't this need a sideExitIntoInterpreter: 1 jitstat as well?
Attachment #366436 - Flags: review?(jwalden+bmo) → review+
http://hg.mozilla.org/tracemonkey/rev/b8f929cd479d
Whiteboard: fixed-in-tracemonkey
(In reply to comment #2)
> We should add a field to the OPDEFs in jsopcode.tbl to record this, rather than
> bloating this ever more as we create more and more imacros.  File a followup
> bug?

Already filed: bug 476240. Not an OPDEF field or flag, the mere definition of an .igroup in imacros.jsasm should suffice to add to the list of ops handled by TraceRecorder::monitorRecording.

/be
(In reply to comment #2)
> (From update of attachment 366436 [details] [diff] [review])
> >diff --git a/js/src/imacros.jsasm b/js/src/imacros.jsasm
> >+.igroup call JSOP_CALL
> >+
> >+    .imacro String                          # String this obj
> >+        dup                                 # String this obj obj
> >+        dup                                 # String this obj obj obj
> >+        getprop valueOf                     # String this obj obj valueOf
> >+        ifprimtop 1                         # String this obj obj valueOf
> >+        swap                                # String this obj valueOf obj
> >+        string string                       # String this obj valueOf obj "string"
> >+        call 1                              # String this obj rval
> >+        ifprimtop 3                         # String this obj rval
> >+        goto 2
> >+1:      pop                                 # String this obj obj
> >+2:      pop                                 # String this obj
> >+        callprop toString                   # String this toString obj
> >+        call 0                              # String this obj

I should have paid better attention. This is wrong, String (15.5.1) called as a function calls ToString (9.8) which calls ToPrimitive on obj with hint String. ToPrimitive (9.1) with hint String calls [[DefaultValue]] (8.6.2.6), which for hint String tries toString before valueOf.

Please post a followup fix, I'll review.

/be
Comment on attachment 366503 [details] [diff] [review]
Really not sure how I missed this when reviewing

Nit: join("") gives shorter expect string, just as good IMHO. Thanks,

/be
Attachment #366503 - Flags: review?(brendan) → review+
I landed Waldo's fix:

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

I didn't change the join(",") in trace-test.js or anything -- I landed exactly what's attached here.

/be
robert-sayres-macbook-pro:dev sayrer$ ./clean-debug-tracemonkey/dist/bin/js
js> for (i = 0; i < 1000; i++) x = String(null);
null
js> 

robert-sayres-macbook-pro:dev sayrer$ ./clean-debug-tracemonkey/dist/bin/js -j
js> for (i = 0; i < 1000; i++) x = String(null);
typein:1: TypeError: String(null) is null
js>
Depends on: 482554
This fixes the test in my comment as well.

The bustage was obvious--crashing in the same mochitest across three platforms. I debugged it by doing this:

python runtests.py --test-path=dom/tests/mochitest/bugs/test_bug411103.html --debugger-interactive --debugger=gdb

That showed a null LIns getting passed to TraceRecorder::set. We shouldn't have let the the tree stay orange for 12hrs due to a sayrer-fixable tracer bug.
Depends on: 482594
http://hg.mozilla.org/mozilla-central/rev/b8f929cd479d
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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
Flags: in-testsuite+
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
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
You need to log in before you can comment on or make changes to this bug.