Fix invalid array length check in TransitionElementsKindStub.

Fix invalid array length check in TransitionElementsKindStub.

The stub used to check the length of the JS array to see if
there's a need to duplicate the elements backing store. This
way it will not duplicate the elements array when going from
double to object even if the elements array is not the empty
fixed array. Later on it will then store pointers into a
FixedDoubleArray.

The native code stub used to check whether elements points to
the empty_fixed_array singleton instead of testing the length.
The Hydrogen stub does that as well now.

R=danno@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=15701

[mvstanton, 2013-07-16 13:01:07]

Hi Benedikt, I think there might be one issue, have a look.
thanks,
-Michael

https://codereview.chromium.org/19289009/diff/1/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/19289009/diff/1/src/code-stubs-hydrogen.cc#ne...
src/code-stubs-hydrogen.cc:553: if_builder.Then();
There might still be an issue here. What if you are going from a 0 size DOUBLE
array to a 0 size TAGGED array. I think that you need to do more than just set
the map of the array. You also need to point the elements property to the
factory->fixed_array_map(), because it's currently pointing to the
factory->fixed_double_array_map(). I guess this would be the "else" case for
your IfBuilder.

[Dmitry Lomov (no reviews), 2013-07-16 13:13:02]

Also, I think the regression test is in order.

[mvstanton, 2013-07-16 13:28:54]

My previous comment is wrong in the specifics: we are careful in the code to
ensure that an array with 0 capacity is always the empty fixed array, it doesn't
matter if our map points to smi, double, or tagged elements kind. So the code
will work as is, but its more direct and obvious to instead see if the elements
pointer is the empty fixed array.

On 2013/07/16 13:13:02, Dmitry Lomov (chromium) wrote:
> Also, I think the regression test is in order.

It's an interesting case. You have to have an empty SMI literal array,
transition it to double, then allocate from the same site again. This time,
don't insert a double as the first element, but a tagged value. It's pretty much
the only way we end up with an empty DOUBLE array, and then want to transition
to an empty tagged array.

[Benedikt Meurer, 2013-07-16 18:06:51]

On 2013/07/16 13:28:54, mvstanton wrote:
> My previous comment is wrong in the specifics: we are careful in the code to
> ensure that an array with 0 capacity is always the empty fixed array, it
doesn't
> matter if our map points to smi, double, or tagged elements kind. So the code
> will work as is, but its more direct and obvious to instead see if the
elements
> pointer is the empty fixed array.

Done.

> On 2013/07/16 13:13:02, Dmitry Lomov (chromium) wrote:
> > Also, I think the regression test is in order.
> 
> It's an interesting case. You have to have an empty SMI literal array,
> transition it to double, then allocate from the same site again. This time,
> don't insert a double as the first element, but a tagged value. It's pretty
much
> the only way we end up with an empty DOUBLE array, and then want to transition
> to an empty tagged array.

I'm unable to get a proper test case for the TransitionElementsKindStub bug.
I just have a test case for the same bug in ElementsTransitionsAndStoreStub.

[Benedikt Meurer, 2013-07-17 08:25:35]

Added test case. PTAL.

[danno, 2013-07-17 08:31:13]

lgtm

[Benedikt Meurer, 2013-07-17 08:32:34]

Committed patchset #5 manually as r15701 (presubmit successful).