Closed
Bug 482349
Opened 16 years ago
Closed 16 years ago
TM: support String(v) -- String constructor called as a converter
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.9.1
People
(Reporter: brendan, Assigned: gal)
References
Details
(Keywords: verified1.9.1, Whiteboard: fixed-in-tracemonkey)
Attachments
(3 files)
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
See bug 460050 comment 14. Easy patch.
/be
Flags: wanted1.9.1?
Updated•16 years ago
|
Flags: wanted1.9.1? → wanted1.9.1+
Assignee | ||
Comment 1•16 years ago
|
||
Assignee | ||
Updated•16 years ago
|
Attachment #366436 -
Flags: review?(jwalden+bmo)
Comment 2•16 years ago
|
||
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+
Assignee | ||
Comment 3•16 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Reporter | ||
Comment 4•16 years ago
|
||
(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
Reporter | ||
Comment 5•16 years ago
|
||
(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 6•16 years ago
|
||
Attachment #366503 -
Flags: review?(brendan)
Reporter | ||
Comment 7•16 years ago
|
||
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+
Reporter | ||
Comment 8•16 years ago
|
||
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
Comment 9•16 years ago
|
||
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>
Comment 10•16 years ago
|
||
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.
Comment 11•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 12•16 years ago
|
||
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+
Comment 13•16 years ago
|
||
Keywords: fixed1.9.1
Comment 14•16 years ago
|
||
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Comment 15•15 years ago
|
||
js1_8_1/trace/trace-test.js
http://hg.mozilla.org/tracemonkey/rev/61892f57b46a
Comment 16•15 years ago
|
||
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.
Description
•