See bug 460050 comment 14. Easy patch. /be

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?

(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

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>

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.

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

v 1.9.1, 1.9.2

js1_8_1/trace/trace-test.js http://hg.mozilla.org/tracemonkey/rev/61892f57b46a

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