396684 - Patch for bug 394551 has wrong changes for js_Invoke

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

For details, see bug 394551 comment 20 and bug 394551 comment 21 as reported by Joachim Kuebart.

Comment 1

[Igor Bukanov]

Attachment: fix v1

To avoid extra ifs and for the extra clarity the fix duplicates the fast call code instead of comparing vp with argv - 2.

Comment 2

[Joachim Kuebart]

The patch from comment 1 seems to work for me: r+

Comment 3

[Igor Bukanov]

Attachment: test case for the not-coping-the-result regression

The test case builds a function containing f(0,0,...,0,Math.atan2()) with enough zero arguments to exhaust the current stack arena fully. Thus, when Math.atan2 is loaded into the stack, there would be no room for the extra 2 args required by Math.atan2 and args will be allocated from the new arena.
 
Here is a test case demo:


~/m/trunk/mozilla $ cat ~/m/y.js
var src = "return f(" +Array(10*1000).join("0,")+"Math.atan2());";

var result = new Function(src)();

if (typeof result != "number" || !isNaN(result))
    throw "unexpected result: "+uneval(result);

function f() {
    return arguments[arguments.length - 1];
}
~/m/trunk/mozilla $ ./js/src/Linux_All_DBG.OBJ/js ~/m/y.js 
uncaught exception: unexpected result: function atan2() {[native code]}

Comment 4

[Joachim Kuebart]

The other way to avoid the second if is to assign unconditionally -- in the case that vp + 2 == argv it's a no-op:

	/* Root the extra slots that are not covered by [vp..vp+2+argc). */
	i = rootedArgsFlag ? 2 + argc : 0;
	JS_PUSH_TEMP_ROOT(cx, 2 + argc + nslots - i, argv - 2 + i, &tvr);
	ok = ((JSFastNative) native)(cx, argc, argv - 2);
	*vp = argv[-2];
        JS_POP_TEMP_ROOT(cx, &tvr);

I don't know whether the smaller branch and less duplicated code would outweigh the extra read/write-cycle in cases where it's not needed.

Comment 5

[Brendan Eich [:brendan]]

Joachim, thanks for catching these -- I should have, since I wrote the moving argv code originally (with review from Scott Furman back in the day) and we had to take care to avoid losing the return value. See also the rvp member of JSInlineFrame.

/be

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #4)
> The other way to avoid the second if is to assign unconditionally -- in the
> case that vp + 2 == argv it's a no-op:
> 
>         /* Root the extra slots that are not covered by [vp..vp+2+argc). */
>         i = rootedArgsFlag ? 2 + argc : 0;
>         JS_PUSH_TEMP_ROOT(cx, 2 + argc + nslots - i, argv - 2 + i, &tvr);
>         ok = ((JSFastNative) native)(cx, argc, argv - 2);
>         *vp = argv[-2];
>         JS_POP_TEMP_ROOT(cx, &tvr);

This is better, because:

> I don't know whether the smaller branch and less duplicated code would outweigh
> the extra read/write-cycle in cases where it's not needed.

the time cost can't matter since fast natives dispatched by js_Invoke (a) should be hard (rare) cases; (b) suffer high overhead elsewhere throughout js_Invoke including load/store traffic. What's decisive here is code size and (related but not the same, although here it is aligned) source complexity, so I am in favor of Joachim's suggestion here.

/be

Comment 7

[Igor Bukanov]

Attachment: fix v2

The new version is Joachim's suggestion with extra comments. I also added skip as a local variable to make the code more descriptive.

Comment 8

[Igor Bukanov]

I checked in the patch from comment 7 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1190236193&maxdate=1190236211&who=igor%mir2.org

To Joachim Kuebart:

thanks again for finding this and improving the patch!

Comment 9

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-396684.js,v  <--  regress-396684.js
initial revision: 1.1

Comment 10

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux|mac|win