Jesse Ruderman Reporter
	Description • 16 years ago

js> (function(d) { for (let j = 0; j < 5; ++j) { print('' + d); } })({valueOf: function()1});
1
1
[object Object]
1
1

	Brendan Eich [:brendan] Assignee
	Comment 1 • 16 years ago

Fooey, no one uses valueOf :-P.

Ok, ok -- I fix.

/be

	Brendan Eich [:brendan] Assignee
	Comment 2 • 16 years ago

ECMA-262 specifies [[DefaultValue]] like so:

8.6.2.6 [[DefaultValue]] (hint)
When the [[DefaultValue]] method of O is called with hint String, the following steps are taken:
1. Call the [[Get]] method of object O with argument "toString".
2. If Result(1) is not an object, go to step 5.
3. Call the [[Call]] method of Result(1), with O as the this value and an empty argument list.
4. If Result(3) is a primitive value, return Result(3).
5. Call the [[Get]] method of object O with argument "valueOf".
6. If Result(5) is not an object, go to step 9.
7. Call the [[Call]] method of Result(5), with O as the this value and an empty argument list.
8. If Result(7) is a primitive value, return Result(7).
9. Throw a TypeError exception.

When the [[DefaultValue]] method of O is called with hint Number, the following steps are taken:

1. Call the [[Get]] method of object O with argument "valueOf".
2. If Result(1) is not an object, go to step 5.
3. Call the [[Call]] method of Result(1), with O as the this value and an empty argument list.
4. If Result(3) is a primitive value, return Result(3).
5. Call the [[Get]] method of object O with argument "toString".
6. If Result(5) is not an object, go to step 9.
7. Call the [[Call]] method of Result(5), with O as the this value and an empty argument list.
8. If Result(7) is a primitive value, return Result(7).
9. Throw a TypeError exception.

When the [[DefaultValue]] method of O is called with no hint, then it behaves as if the hint were Number, unless O is a Date object (section 15.9), in which case it behaves as if the hint were String.

The above specification of [[DefaultValue]] for native objects can return only primitive values. If a host object implements its own [[DefaultValue]] method, it must ensure that its [[DefaultValue]] method can return only primitive
values.

Addition passes no hint, other ops pass "number". We implement Date conversion a bit different, but it works out to the same answers.

Please check this and ask questions. I'll invite mrbkap to review too.

/be

	Andreas Gal :gal
	Comment 3 • 16 years ago

How do we know that at runtime the objects will have the same combination of methods? We could get entirely different combinations when we run the trace. /me is confused.

	Brendan Eich [:brendan] Assignee
	Comment 4 • 16 years ago

(In reply to comment #3)
> How do we know that at runtime the objects will have the same combination of
> methods? We could get entirely different combinations when we run the trace.
> /me is confused.

We talked about this in the imacros bug. Shapes guard method *values*, not just names. Tracing guards return type. The has*Method probes are just prolog to the JSOP_CALLPROP ops in the imacros, which indeed trace to shape-guards.

/be

	Andreas Gal :gal
	Comment 5 • 16 years ago

Yes, but what happens if the CALLPROP shape guard fails? How do we get back into the right imacro for this case? (the _other_ path)

	Andreas Gal :gal
	Comment 6 • 16 years ago

IMO for this to work there should be only one method you call and its implemented differently in each object as needed. But dispatching at recording time doesn't work right as far as I can tell.

	Andreas Gal :gal
	Comment 7 • 16 years ago

What would also work is to make this decision in the imacro, so jump around in the imacro to select what method to call. But it has to be in the imacro. Since thats where we return to and re-start recording to capture the other path.

	Brendan Eich [:brendan] Assignee
	Comment 8 • 16 years ago

Yeah, I need to implement the exact logic cited in comment 2 from ECMA-262 in the imacro body. This will reduce the number of imacros. Thanks,

/be

	Andreas Gal :gal
	Comment 9 • 16 years ago

Comment on attachment 348921 [details] [diff] [review]
proposed fix

As discussed in the bug this has to all happen inside the same snippet, not by dispatching up front.

	Brendan Eich [:brendan] Assignee
	Comment 12 • 16 years ago

This is much more general, but still could be extended to support things like C pre-processor constants as immediate operands, e.g. (see the iter_imacros still hand-coded as C++ array initializers in jstracer.cpp).

Alas this does not bootstrap. I really could use some advice from jimb and/or bsmedberg to get it right. I'd like to use Narcissus, if possible. More on IRC.

Attaching now to get reviewers started, even though this isn't quite ready for checkin, because it will blow up the build system unless I hardcode to #include "imacros.c" and commit imacros.c.out from the objdir as srcdir/imacros.c!

/be

	Brendan Eich [:brendan] Assignee
	Comment 13 • 16 years ago

Many fixes, including separating null from object type -- necessary to pass trace-test.js tests such as testObjectOrderedCmp2. I also moved the lengthy Mandelbrot stuff down to next to last in trace-test.js

/be

	Brendan Eich [:brendan] Assignee
	Comment 14 • 16 years ago

Comment on attachment 352281 [details] [diff] [review]
proposed fix, take 3

I'd welcome mrbkap's review too. More the merrier, really.

/be

	Jesse Ruderman Reporter
	Comment 15 • 16 years ago

> We depend on the snarf function from the js shell, defined
> only for the Narcissus metacircular evaluator (#ifdef NARCISSUS).

Does this mean my shell shouldn't have snarf()?  I'm fine with it being defined
for normal js shells, as long as there's a way for paranoid users to disable
load() and snarf().

	Jesse Ruderman Reporter
	Comment 16 • 16 years ago

js> for each (let c in [null, null, null, {}]) { }
Assertion failure: *(JSObject**)slot == NULL, at ../jstracer.cpp:1409

	Jesse Ruderman Reporter
	Comment 17 • 16 years ago

js> for each (let j in [null, 2, 3]) { }
Assertion failure: (*m == JSVAL_TNULL) ? JSVAL_IS_NULL(*vp) : *m == JSVAL_TAG(*vp), at ../jstracer.cpp:2222

	Brendan Eich [:brendan] Assignee
	Comment 18 • 16 years ago

(In reply to comment #15)
> > We depend on the snarf function from the js shell, defined
> > only for the Narcissus metacircular evaluator (#ifdef NARCISSUS).
> 
> Does this mean my shell shouldn't have snarf()?

No, stale comment. I'll fix.

> I'm fine with it being defined
> for normal js shells, as long as there's a way for paranoid users to disable
> load() and snarf().

What's the way to disable load? Whatever it is could apply to snarf too.

/be

	Jesse Ruderman Reporter
	Comment 19 • 16 years ago

js> for (var j = 0; j < 3; ++j) { 1 & new Date; }
Assertion failed: (!lhs->isQuad() && !rhs->isQuad()) || (lhs->isQuad() && rhs->isQuad()) (../nanojit/Nativei386.cpp:925)

js> for (var j = 0; j < 3; ++j) { 1 & Date; }
Assertion failure: !JS_ON_TRACE(cx), at ../jsobj.cpp:3776

	Jesse Ruderman Reporter
	Comment 20 • 16 years ago

js> for each (let x in [1, {}, 1, null, 1, {}, 1, null, 1]) { }
Assertion failure: !ti->stackTypeMap.matches(ti_other->stackTypeMap), at ../jstracer.cpp:3035

	Jesse Ruderman Reporter
	Comment 21 • 16 years ago

js> e = <x/>; for (j=0;j<3;++j) { 3 | e; } "PASS";
typein:1: TypeError:  is not a function

	Brendan Eich [:brendan] Assignee
	Comment 22 • 16 years ago

(In reply to comment #18)
> > I'm fine with it being defined
> > for normal js shells, as long as there's a way for paranoid users to disable
> > load() and snarf().
> 
> What's the way to disable load? Whatever it is could apply to snarf too.

Jesse mentioned the one true way: delete load -- delete snarf works too.

Debugging the fuzz-found bugs. Keep them coming.

/be

	Brendan Eich [:brendan] Assignee
	Comment 23 • 16 years ago

Fixed comment 16, comment 17, and comment 20 testcases.

/be

	Brendan Eich [:brendan] Assignee
	Comment 24 • 16 years ago

Bugzilla interdiff shows a crucial fix to a latent JSVAL_TAG abusage bug in TraceRecorder::checkType (look for uint8 vt).

/be

	Jesse Ruderman Reporter
	Comment 25 • 16 years ago

js> for each (let _ in [{}, {}, null, null, null, null]) { }
Assertion failure: !ti->stackTypeMap.matches(ti_other->stackTypeMap), at ../jstracer.cpp:3037

	Brendan Eich [:brendan] Assignee
	Comment 26 • 16 years ago

This fixes all the fuzz-found problems except the E4X one in comment 21. I will deal with that tomorrow morning, but in the mean time, this is ready for review and more fuzz-testing.

/be

	Jesse Ruderman Reporter
	Comment 27 • 16 years ago

js> for (let i in (function() { for (var j = 0; j < 3; ++j) yield; })()) { }
Assertion failure: !JS_ON_TRACE(cx), at ../jsobj.cpp:3776

	Jesse Ruderman Reporter
	Comment 28 • 16 years ago

js> for (let i = 0; i < 2; ++i) { ({}) + 3; }
Assertion failure: !JS_ON_TRACE(cx), at ../jsobj.cpp:3776

	Brendan Eich [:brendan] Assignee
	Comment 29 • 16 years ago

Correct kshape updating logic on "brandfill" in js_FillPropertyCache, for comment 27 and comment 28 testcases.

/be

	Jesse Ruderman Reporter
	Comment 30 • 16 years ago

cat ~/q.js
for (let d=0;d<2;++d) for (let a=0;a<1;++a) "" + (d && function(){});

~/tracemonkey/js/src/debug-valueof6/js -j ~/q.js
Assertion failed: "need a way to EOT now, since this is trace end": 0 (../nanojit/LIR.cpp:984)

	Brendan Eich [:brendan] Assignee
	Comment 31 • 16 years ago

David, could use your help.

Why does the testcase in comment 30 work interactively (as a one-line program typed to the interactive shell, I mean) but assertbotch when sourced from a file?

/be

	Robert Sayre
	Comment 32 • 16 years ago

Could be latent. Bug 463783 showed the same symptom.

	Jesse Ruderman Reporter
	Comment 35 • 16 years ago

js> for (var j = 0; j < 2; ++j) { if (null > "") { } }
Assertion failure: JSVAL_IS_NUMBER if int/double, objects should have been handled at start of method, at ../jstracer.cpp:4728

Happens with 6b and 7, both applied to 56029d4f6305.  Does not happen with 56029d4f6305 alone, so not simply a regression from Waldo's change in bug 465255.

	Brendan Eich [:brendan] Assignee
	Comment 36 • 16 years ago

(Eight takes is nothing. From the wikipedia entry for the film "Shane":

"Jack Palance had problems with horses and Alan Ladd with guns. The scene where Shane practices shooting in front of Joey required 116 takes. A scene where Jack Palance mounts his horse was actually a shot of him dismounting, but played in reverse. As well, the original planned introduction of Wilson galloping into town was replaced with him simply walking in on his horse, which was noted as improving the entrance by making him seem more threatening."

Next patch will be walked into town.

/be)

	Brendan Eich [:brendan] Assignee
	Comment 37 • 16 years ago

(In reply to comment #35)
> Happens with 6b and 7, both applied to 56029d4f6305.  Does not happen with
> 56029d4f6305 alone, so not simply a regression from Waldo's change in bug
> 465255.

It was a bad merge by yours truly -- Waldo and I collided at a level hg could not see. Fixed by take 8.

/be

	Andreas Gal :gal
	Comment 38 • 16 years ago

Comment on attachment 352639 [details] [diff] [review]
proposed fix, take 8

I did not review the js source of the assembler and load/snarf changes.

	Brendan Eich [:brendan] Assignee
	Comment 39 • 16 years ago

At Andreas's urging, I committed to tm, a bit undertested. Will file followups on comment 21 and any other probs that don't cause a back-out!

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

/be

	Robert Sayre
	Comment 40 • 16 years ago

shoot. I had to back this out for three reasons:

1.) fails a scriptaculous mochitest on linux and macintel
    should be possible to run in isolation with runtests.py --path=...

2.) fails to build on mac ppc (probably an ifdef checking for mac but not intel)

3.) fails to build on windows because off_t is not defined there

http://hg.mozilla.org/tracemonkey/rev/5a26ec73cf0d

	Jeff Walden [:Waldo]
	Comment 41 • 16 years ago

(In reply to comment #40)
>     should be possible to run in isolation with runtests.py --path=...

Of course what was meant was --test-path=dom/tests/mochitest/ajax/scriptaculous or somesuch, not --path.

	Brendan Eich [:brendan] Assignee
	Comment 42 • 16 years ago

Are 2 and 3 due to the js.cpp changes? I'm reverting them to avoid trouble (it's easy to fix them too but not important right now, given the by-hand generation of imacros.c.out).

I'll look at 1 when I have time (off work today). Thanks for the back-out help.

/be

	Andreas Gal :gal
	Comment 43 • 16 years ago

I am fixing up 2 and 3. New patch will be posted shortly.

	Andreas Gal :gal
	Comment 45 • 16 years ago

Comment on attachment 352812 [details] [diff] [review]
v9

Can't use hg diff until the file generated is added.

	Brendan Eich [:brendan] Assignee
	Comment 47 • 16 years ago

Unless I goofed up subtly, it looks like mq doesn't keep track of hg add'ed files.

/be

	Brendan Eich [:brendan] Assignee
	Comment 48 • 16 years ago

I'm thinking FAIL_VOID is not useful -- we could eliminate it by using FAIL_JSVAL. Comments welcome.

/be

	Jesse Ruderman Reporter
	Comment 49 • 16 years ago

Are you planning to fix bug 469239 before landing this patch?

	Brendan Eich [:brendan] Assignee
	Comment 50 • 16 years ago

Jason, could you please review the interdiff against last patch (I hope bugzilla doesn't botch it). I didn't remove FAIL_VOID since that's useful for fallibles that return a boolean. I changed OBJECT_FAIL_VOID to be named simply OBJECT_FAIL (as opposed to the extant OBJECT_FAIL_NULL, which means null as failure code where success means a non-null object). OBJECT_FAIL now uses FAIL_JSVAL, so String_p_match* return jsval. Let me know if anything is amiss here, thanks.

Jesse, this fixes bug 469239.

/be

	Brendan Eich [:brendan] Assignee
	Comment 51 • 16 years ago

Landed in tm again, r=gal -- will fix anything left over that anyone wants in a new bug, assuming this sticks:

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

I removed OBJECT_FAIL altogether, it was misleading to have this duplicate JSVAL_FAIL -- Jason, holler if that's wrong.

/be

	Brendan Eich [:brendan] Assignee
	Comment 53 • 16 years ago

Trivial followup fix:

http://hg.mozilla.org/tracemonkey/rev/370f4468be2f

/be

	Brendan Eich [:brendan] Assignee
	Comment 54 • 16 years ago

More from Andreas:

http://hg.mozilla.org/tracemonkey/rev/1329b1e3edd3
http://hg.mozilla.org/tracemonkey/rev/7c898d7b2e9e

/be

	Jason Orendorff [:jorendorff]
	Comment 55 • 16 years ago

OBJECT_FAIL_VOID was faster than JSVAL_FAIL, which requires unboxing on trace.  But, the less bizarre calling conventions, the better.

If we're going to use JSVAL_FAIL for String_p_match, I think we could revert my changes in bug 453564.

	Brendan Eich [:brendan] Assignee
	Comment 56 • 16 years ago

(In reply to comment #55)
> OBJECT_FAIL_VOID was faster than JSVAL_FAIL, which requires unboxing on trace.

The split between JSVAL_TNULL and JSVAL_OBJECT in typemap-types requires most of unbox_jsval, plus the resumeAfter logic. So it seemed better to use FAIL_JSVAL and avoid adding more resumeAfter fail-types and special cases.
 
> If we're going to use JSVAL_FAIL for String_p_match, I think we could revert my
> changes in bug 453564.

Just the first hunk, right?

/be

	Robert Sayre
	Comment 57 • 16 years ago

merged to mc

	Jason Orendorff [:jorendorff]
	Comment 58 • 16 years ago

(In reply to comment #56)
> (In reply to comment #55)
> > If we're going to use JSVAL_FAIL for String_p_match, I think we could revert
> > my changes in bug 453564.
> 
> Just the first hunk, right?

No, I think we can revert it all.  (Well--we'd keep the extern js_String_p_match -> static String_p_match change and the test.)

The problem in that bug was that the recorded return type of String.prototype.match was boolean, but the traceable native returned JSObject*.  Since we've changed the traceable native to return jsval, that conflict can't happen anymore.

	Jason Orendorff [:jorendorff]
	Comment 59 • 16 years ago

Filed bug 471897 for that.

	Robert Sayre
	Comment 60 • 16 years ago

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d9084a656268
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d50da676e473
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ff1345ee6580
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a7179884097e

	Bob Clary [:bc] (inactive)
	Comment 61 • 16 years ago

(In reply to comment #21)
> js> e = <x/>; for (j=0;j<3;++j) { 3 | e; } "PASS";
> typein:1: TypeError:  is not a function

I still see this with -j.

	Brendan Eich [:brendan] Assignee
	Comment 62 • 16 years ago

(In reply to comment #61)
> (In reply to comment #21)
> > js> e = <x/>; for (j=0;j<3;++j) { 3 | e; } "PASS";
> > typein:1: TypeError:  is not a function
> 
> I still see this with -j.

Eek. New bug please?

/be

	Bob Clary [:bc] (inactive)
	Comment 63 • 16 years ago

Checking in js1_8/regress/regress-465460-01.js
Checking in js1_8/regress/regress-465460-02.js
Checking in js1_8/regress/regress-465460-03.js
Checking in js1_8/regress/regress-465460-04.js
Checking in js1_8/regress/regress-465460-05.js
Checking in js1_8/regress/regress-465460-06.js
Checking in js1_8/regress/regress-465460-07.js
Checking in js1_8/regress/regress-465460-08.js
Checking in js1_8/regress/regress-465460-09.js
Checking in js1_8/regress/regress-465460-10.js
Checking in js1_8/regress/regress-465460-11.js
Checking in js1_8/regress/regress-465460-12.js

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

filed Bug 475153 for js1_8/regress/regress-465460-07.js - 'TypeError: is not a function'

	Bob Clary [:bc] (inactive)
	Comment 64 • 16 years ago

v 1.9.1, 1.9.2 modulo Bug 475153 js1_8/regress/regress-465460-07.js