Becuase of cross-context calls, hydrogen-based Array constructor needs to ensure (issue 14846017)

[mvst...@chromium.org]

Reviewers: danno,

Message:
Hi Danno, here is the fix for the cross-context issue we discussed. Thanks
for
the look,
--Michael

Description:
Becuase of cross-context calls, hydrogen-based Array constructor needs to
ensure
the array constructor pointer passed in matches that of the current context.

BUG=

Please review this at https://codereview.chromium.org/14846017/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/arm/code-stubs-arm.cc
M src/builtins.cc
M src/code-stubs-hydrogen.cc
M src/code-stubs.h
M src/heap.cc
M src/hydrogen.h
M src/hydrogen.cc
M src/ia32/code-stubs-ia32.cc
M src/x64/code-stubs-x64.cc
M test/mjsunit/allocation-site-info.js

[mvst...@chromium.org]

Giving to Toon...

https://codereview.chromium.org/14846017/

[verw...@chromium.org]

Looking good. First round of comments.


https://codereview.chromium.org/14846017/diff/2001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/14846017/diff/2001/src/builtins.cc#newcode235
src/builtins.cc:235: ASSERT(constructor->initial_map()->elements_kind()
== kind);
It seems more resilient to just use the initial map (and its elements
kind), and make that one holey when necessary. You can keep the ASSERT
just to find this place back when you make changes.

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

https://codereview.chromium.org/14846017/diff/2001/src/code-stubs-hydrogen.cc#newcode545
src/code-stubs-hydrogen.cc:545: HValue* array_result = GetParameter(0);
Remove

https://codereview.chromium.org/14846017/diff/2001/src/code-stubs-hydrogen.cc#newcode557
src/code-stubs-hydrogen.cc:557: array_result =
array_builder.AllocateEmptyArray();
Just return.

https://codereview.chromium.org/14846017/diff/2001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/14846017/diff/2001/src/ia32/code-stubs-ia32.cc#newcode7925
src/ia32/code-stubs-ia32.cc:7925: __ mov(edx, FieldOperand(ebx,
kPointerSize));
Don't you want to use JSGlobalPropertyCell::kValueOffset here?

https://codereview.chromium.org/14846017/diff/2001/test/mjsunit/allocation-site-info.js
File test/mjsunit/allocation-site-info.js (right):

https://codereview.chromium.org/14846017/diff/2001/test/mjsunit/allocation-site-info.js#newcode300
test/mjsunit/allocation-site-info.js:300: assertTrue(new
realmBArray(1,2,3) instanceof realmBArray);
Please add another test that checks the optimized instantiate behavior.

https://codereview.chromium.org/14846017/

[mvst...@chromium.org]

Thanks Toon, the test suggestion helped me find a redundant HCheckFunction
in
the optimized case!
--Michael

https://codereview.chromium.org/14846017/diff/2001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/14846017/diff/2001/src/builtins.cc#newcode235
src/builtins.cc:235: ASSERT(constructor->initial_map()->elements_kind()
== kind);

On 2013/05/07 11:02:06, Toon Verwaest wrote:
> It seems more resilient to just use the initial map (and its elements
kind), and
> make that one holey when necessary. You can keep the ASSERT just to
find this
> place back when you make changes.

Done.

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

https://codereview.chromium.org/14846017/diff/2001/src/code-stubs-hydrogen.cc#newcode545
src/code-stubs-hydrogen.cc:545: HValue* array_result = GetParameter(0);

On 2013/05/07 11:02:06, Toon Verwaest wrote:
> Remove

Done.

https://codereview.chromium.org/14846017/diff/2001/src/code-stubs-hydrogen.cc#newcode557
src/code-stubs-hydrogen.cc:557: array_result =
array_builder.AllocateEmptyArray();

On 2013/05/07 11:02:06, Toon Verwaest wrote:
> Just return.

Done.

https://codereview.chromium.org/14846017/diff/2001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/14846017/diff/2001/src/ia32/code-stubs-ia32.cc#newcode7925
src/ia32/code-stubs-ia32.cc:7925: __ mov(edx, FieldOperand(ebx,
kPointerSize));

On 2013/05/07 11:02:06, Toon Verwaest wrote:
> Don't you want to use JSGlobalPropertyCell::kValueOffset here?

Done.

https://codereview.chromium.org/14846017/diff/2001/test/mjsunit/allocation-site-info.js
File test/mjsunit/allocation-site-info.js (right):

https://codereview.chromium.org/14846017/diff/2001/test/mjsunit/allocation-site-info.js#newcode300
test/mjsunit/allocation-site-info.js:300: assertTrue(new
realmBArray(1,2,3) instanceof realmBArray);

On 2013/05/07 11:02:06, Toon Verwaest wrote:
> Please add another test that checks the optimized instantiate
behavior.

Done.

https://codereview.chromium.org/14846017/

[verw...@chromium.org]

lgtm



https://codereview.chromium.org/14846017/

[mvst...@chromium.org]

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

https://codereview.chromium.org/14846017/