The ArrayConcat builtin didn't respect @@isConcatSp

Fix @@isConcatSpreadable in ArrayConcat builtin

The ArrayConcat builtin didn't respect @@isConcatSpreadable, which,
surprisingly, is observable if you modify an Array's Symbol.isConcatSpreadable
property. This patch bails out of the ArrayConcat builtin if any argument's
Symbol.isConcatSpreadable property is set to false, but continues otherwise,
meeting the spec's requirements.

BUG=v8:4317
LOG=N

[Camillo Bruni, 2015-08-19 12:48:07]

cbruni@chromium.org changed reviewers:
+ cbruni@chromium.org

[Camillo Bruni, 2015-08-19 12:48:07]

lgtm

[Dan Ehrenberg, 2015-08-19 17:39:50]

I am a little hesitant to push ahead with this strategy until we have benchmarks
to show it doesn't give a regression. In particular, I wanted to test the
performance of this patch against the baseline, and also against just not having
the builtin at all and going straight to the JS version. cbruni, do you have any
tests that could be used for this purpose?

[Dan Ehrenberg, 2015-08-19 17:39:54]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-19 17:40:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1247243003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1247243003/20001

[commit-bot: I haz the power, 2015-08-19 17:52:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-19 17:52:15]

Dry run: Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/8837)

[adamk, 2015-08-19 19:50:34]

adamk@chromium.org changed reviewers:
+ adamk@chromium.org

[adamk, 2015-08-19 19:50:35]

https://codereview.chromium.org/1247243003/diff/20001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1247243003/diff/20001/src/builtins.cc#newcode951
src/builtins.cc:951: fallback = !IsConcatSpreadable(isolate, obj);
As the test failures show, you can't make this call here, since there are a
bunch of non-handlified bits of state in local variables which might move if an
allocation occurs.

[Camillo Bruni, 2015-09-04 08:24:36]

On 2015/08/19 at 19:50:35, adamk wrote:
> https://codereview.chromium.org/1247243003/diff/20001/src/builtins.cc
> File src/builtins.cc (right):
> 
>
https://codereview.chromium.org/1247243003/diff/20001/src/builtins.cc#newcode951
> src/builtins.cc:951: fallback = !IsConcatSpreadable(isolate, obj);
> As the test failures show, you can't make this call here, since there are a
bunch of non-handlified bits of state in local variables which might move if an
allocation occurs.

I'm about to finish a refactoring of ArrayConcata in
https://codereview.chromium.org/1330483003 which avoids jumping to the
JavaScript code after trying the fast Builtin.
I will try to fix your issue as well using the same approach.