[elements] Fix length bounds precheck for Array.prototype.concat

[elements] Fix length bounds precheck for Array.prototype.concat

BUG=chromium:599414
LOG=n

Committed: https://crrev.com/823224f3ee7959dc4731191439dcef2bcb16a043
Cr-Commit-Position: refs/heads/master@{#35269}

[Camillo Bruni, 2016-04-05 11:14:43]

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

[Camillo Bruni, 2016-04-05 11:14:43]

PTAL
the test runs in 2.5s in debug mode, I guess that is fine.

[Igor Sheludko, 2016-04-05 12:53:27]

lgtm with nits:

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

https://codereview.chromium.org/1863553003/diff/20001/src/builtins.cc#newcode...
src/builtins.cc:1561: if (FixedDoubleArray::kMaxLength < result_len) {
How about also adding:
  STATIC_ASSERT(FixedDoubleArray::kMaxLength < FixedArray::kMaxLength);
or write this check so:
  if (FixedDoubleArray::kMaxLength < result_len && 
      FixedArray::kMaxLength < result_len) {
(the compiler should be smart enough to leave only one check).

https://codereview.chromium.org/1863553003/diff/20001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1863553003/diff/20001/src/elements.cc#newcode...
src/elements.cc:2996: STATIC_ASSERT(FixedDoubleArray::kMaxLength <
kHalfOfMaxInt);
You could probably also hoist these checks out of the loop as above.

https://codereview.chromium.org/1863553003/diff/20001/src/elements.cc#newcode...
src/elements.cc:3000: DCHECK(result_len <= FixedDoubleArray::kMaxLength);
How about also adding:
  STATIC_ASSERT(FixedDoubleArray::kMaxLength < FixedArray::kMaxLength);

[Camillo Bruni, 2016-04-05 15:07:48]

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

https://codereview.chromium.org/1863553003/diff/20001/src/builtins.cc#newcode...
src/builtins.cc:1561: if (FixedDoubleArray::kMaxLength < result_len) {
On 2016/04/05 at 12:53:27, Igor Sheludko wrote:
> How about also adding:
>   STATIC_ASSERT(FixedDoubleArray::kMaxLength < FixedArray::kMaxLength);
> or write this check so:
>   if (FixedDoubleArray::kMaxLength < result_len && 
>       FixedArray::kMaxLength < result_len) {
> (the compiler should be smart enough to leave only one check).

done.

https://codereview.chromium.org/1863553003/diff/20001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1863553003/diff/20001/src/elements.cc#newcode...
src/elements.cc:2996: STATIC_ASSERT(FixedDoubleArray::kMaxLength <
kHalfOfMaxInt);
On 2016/04/05 at 12:53:27, Igor Sheludko wrote:
> You could probably also hoist these checks out of the loop as above.

done

[Camillo Bruni, 2016-04-05 15:10:58]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-04-05 15:10:59]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/1863553003/#ps60001
(title: "compare properly")

[commit-bot: I haz the power, 2016-04-05 15:11:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863553003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863553003/60001

[commit-bot: I haz the power, 2016-04-05 15:35:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-05 15:35:37]

Description was changed from

==========
[elements] Fix length bounds precheck for Array.prototype.concat

BUG=chromium:599414
LOG=n
==========

to

==========
[elements] Fix length bounds precheck for Array.prototype.concat

BUG=chromium:599414
LOG=n

Committed: https://crrev.com/823224f3ee7959dc4731191439dcef2bcb16a043
Cr-Commit-Position: refs/heads/master@{#35269}
==========

[commit-bot: I haz the power, 2016-04-05 15:35:37]

Patchset 4 (id:??) landed as
https://crrev.com/823224f3ee7959dc4731191439dcef2bcb16a043
Cr-Commit-Position: refs/heads/master@{#35269}