582161 - TM: new Array(non-unsigned-32-bit integer) works incorrectly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

[jwalden@find-waldo-now src]$ dbg/js -j
js> var vs = [1, 1, 1, 1, 3.5, 1];
js> for (var i = 0, sz = vs.length; i < sz; i++) new Array(vs[i]);
[,]
js> 
[jwalden@find-waldo-now src]$ dbg/js
js> var vs = [1, 1, 1, 1, 3.5, 1];
js> for (var i = 0, sz = vs.length; i < sz; i++) new Array(vs[i]);
typein:2: RangeError: invalid array length

The latter behavior is correct.

Comment 1

[Jeff Walden [:Waldo]]

We only manage to get the negative-length case right by some amount of dumb luck: -1, or whatever, gets converted to mega-huge and is passed to js_NewArrayWithSlots, but -- right now -- when we try to growDenseArrayElements in js_NewArrayWithSlots we hit the return-false below, go back to the interpreter, and retry.  I don't think this is the method of failure we really want here, happening to do okay because an obscured overflow check happens to catch it.

JSObject::growDenseArrayElements (this=0x7ffff1a035a0, cx=0x8faca0, oldcap=0, newcap=4294967295) at ../jsarray.cpp:314
314	    JS_ASSERT(isDenseArray());
315	    JS_ASSERT(newcap >= ARRAY_CAPACITY_MIN);
316	    JS_ASSERT(newcap >= oldcap);
317	
318	    if (newcap > MAX_DSLOTS_LENGTH32) {
319	        if (!JS_ON_TRACE(cx))
320	            js_ReportAllocationOverflow(cx);
321	        return JS_FALSE;
322	    }

Comment 2

[Paul Biggar]

The test case from comment 1 is now working. It was probably fixed during the conversion to flexible length JSObject in bug 584917. I'm going to add it to the repo anyway.

The second problem -- that it's a mess in there -- is definitely true, but I can't find any actual errors in there. We're very unclear about when to use jsuint and when to use jsint, but i'm cleaning this up as part of bug 581180.

Comment 3

[Brendan Eich [:brendan]]

Outside of the public API, we should use int32 and uint32 (no _t please) instead of jsint and jsuint. ECMA-262 is not gonna grow to 64-bit int types and EIBTI.

/be

Comment 4

[Paul Biggar]

http://hg.mozilla.org/tracemonkey/rev/06f233f9259b

This is just the test case, no review required.

Comment 5

[Jeff Walden [:Waldo]]

Did you check whether the song and dance routine of comment 1 is still in place?  Even if this now is observably correct, I'd still like to see that fixed.

Comment 6

[Paul Biggar]

(In reply to comment #5)
> Did you check whether the song and dance routine of comment 1 is still in
> place?  Even if this now is observably correct, I'd still like to see that
> fixed.

I take it you mean comment 2. The song-and-dance has changed considerably, but its still sort-of singing and dancing. The tracer now calls js_NewEmptyArray, which has a check for len < -1, so the exact problem is fixed. But the multitude of array allocation functions occasionally confuse the signed-ness, which I'm sorting out as part of 581180.

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/06f233f9259b