[stubs] Port KeyedStoreIC_Megamorphic stub to Turbofan

[stubs] Port KeyedStoreIC_Megamorphic stub to Turbofan

BUG=v8:5269, v8:5561
Committed: https://crrev.com/5c03cb79228e4312e9722e46c681c0632eb55f2b
Cr-Commit-Position: refs/heads/master@{#40896}

[Jakob Kummerow, 2016-10-31 15:42:08]

Description was changed from

==========
[stubs] Port KeyedStoreIC_Megamorphic stub to Turbofan
==========

to

==========
[stubs] Port KeyedStoreIC_Megamorphic stub to Turbofan

BUG=v8:5269
==========

[Jakob Kummerow, 2016-11-04 18:55:58]

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

[Jakob Kummerow, 2016-11-04 18:55:59]

PTAL.

[Jakob Kummerow, 2016-11-04 18:58:26]

https://codereview.chromium.org/2444353002/diff/80001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (left):

https://codereview.chromium.org/2444353002/diff/80001/src/code-stub-assembler...
src/code-stub-assembler.cc:2228: FillFixedArrayWithValue(to_kind, new_elements,
capacity, new_capacity,
CopyFixedArrayElements() does this too, no need to do it twice.

https://codereview.chromium.org/2444353002/diff/80001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/2444353002/diff/80001/src/ic/ic.cc#newcode1675
src/ic/ic.cc:1675: if (is_vector_set()) {
Drive-by fix.

[Igor Sheludko, 2016-11-07 14:43:39]

https://codereview.chromium.org/2444353002/diff/80001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (left):

https://codereview.chromium.org/2444353002/diff/80001/src/code-stub-assembler...
src/code-stub-assembler.cc:2228: FillFixedArrayWithValue(to_kind, new_elements,
capacity, new_capacity,
On 2016/11/04 18:58:26, Jakob Kummerow wrote:
> CopyFixedArrayElements() does this too, no need to do it twice.

Acknowledged.

https://codereview.chromium.org/2444353002/diff/80001/src/ic/keyed-store-gene...
File src/ic/keyed-store-generic.cc (right):

https://codereview.chromium.org/2444353002/diff/80001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:65: STATIC_ASSERT(sizeof(CodeStubAssembler) ==
sizeof(KeyedStoreGenericAssembler));
Maybe put it near the reinterpret cast.

https://codereview.chromium.org/2444353002/diff/80001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:159: StoreObjectFieldNoWriteBarrier(receiver,
JSObject::kMapOffset, holey_map);
I don't think we can skip write barrier here.

https://codereview.chromium.org/2444353002/diff/80001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:402: IntPtrGreaterThan(elements_kind,
IntPtrConstant(LAST_FAST_ELEMENTS_KIND)),
s/IntPtr/Int32/ as elements_kind is a 32-bit value.

https://codereview.chromium.org/2444353002/diff/80001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:455: GotoIf(IntPtrGreaterThanOrEqual(
Same here and below

[Jakob Kummerow, 2016-11-07 16:55:25]

Thanks for the review. Comments addressed, PTAL.

https://codereview.chromium.org/2444353002/diff/80001/src/ic/keyed-store-gene...
File src/ic/keyed-store-generic.cc (right):

https://codereview.chromium.org/2444353002/diff/80001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:65: STATIC_ASSERT(sizeof(CodeStubAssembler) ==
sizeof(KeyedStoreGenericAssembler));
On 2016/11/07 14:43:39, Igor Sheludko wrote:
> Maybe put it near the reinterpret cast.

Done.

https://codereview.chromium.org/2444353002/diff/80001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:159: StoreObjectFieldNoWriteBarrier(receiver,
JSObject::kMapOffset, holey_map);
On 2016/11/07 14:43:39, Igor Sheludko wrote:
> I don't think we can skip write barrier here.

Done.

https://codereview.chromium.org/2444353002/diff/80001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:402: IntPtrGreaterThan(elements_kind,
IntPtrConstant(LAST_FAST_ELEMENTS_KIND)),
On 2016/11/07 14:43:39, Igor Sheludko wrote:
> s/IntPtr/Int32/ as elements_kind is a 32-bit value.

Done.

https://codereview.chromium.org/2444353002/diff/80001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:455: GotoIf(IntPtrGreaterThanOrEqual(
On 2016/11/07 14:43:39, Igor Sheludko wrote:
> Same here and below

Done.

[Igor Sheludko, 2016-11-07 17:02:59]

lgtm

[Jakob Kummerow, 2016-11-07 17:03:07]

The CQ bit was checked by jkummerow@chromium.org

[commit-bot: I haz the power, 2016-11-07 17:03:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jakob Kummerow, 2016-11-07 17:04:25]

Description was changed from

==========
[stubs] Port KeyedStoreIC_Megamorphic stub to Turbofan

BUG=v8:5269
==========

to

==========
[stubs] Port KeyedStoreIC_Megamorphic stub to Turbofan

BUG=v8:5269,v8:5561
==========

[commit-bot: I haz the power, 2016-11-07 17:24:07]

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

[commit-bot: I haz the power, 2016-11-07 17:24:07]

Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/15797)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[Jakob Kummerow, 2016-11-10 13:52:21]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2016-11-10 13:52:22]

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/2444353002/#ps140001
(title: "rebased")

[commit-bot: I haz the power, 2016-11-10 13:52:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 14:23:20]

Description was changed from

==========
[stubs] Port KeyedStoreIC_Megamorphic stub to Turbofan

BUG=v8:5269,v8:5561
==========

to

==========
[stubs] Port KeyedStoreIC_Megamorphic stub to Turbofan

BUG=v8:5269,v8:5561
==========

[commit-bot: I haz the power, 2016-11-10 14:23:21]

Committed patchset #8 (id:140001)

[Benedikt Meurer, 2016-11-10 18:24:56]

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

[Benedikt Meurer, 2016-11-10 18:24:57]

Awesome!

[krasin1, 2016-11-11 22:25:59]

For the reference, this broke UBSan Vptr bots in Chromium:
https://build.chromium.org/p/chromium.fyi/builders/UBSanVptr%20Linux/builds/1496

python ../../v8/tools/run.py ./mksnapshot --startup_src gen/v8/snapshot.cc
--random-seed 314159265 --startup_blob snapshot_blob.bin
../../v8/src/ic/keyed-store-generic.cc:72:9: runtime error: member call on
address 0x7fffef529cb0 which does not point to an object of type
'v8::internal::KeyedStoreGenericAssembler'
0x7fffef529cb0: note: object is of type 'v8::internal::CodeStubAssembler'
 8f 7f 00 00  68 04 37 12 8f 7f 00 00  b0 a6 45 13 8f 7f 00 00  7e 00 00 00 3b
03 5f 13  5b 79 09 12
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'v8::internal::CodeStubAssembler'

#0 
v8::internal::KeyedStoreGenericGenerator::Generate(v8::internal::CodeStubAssembler*,
v8::internal::CodeStubAssembler::StoreICParameters const*,
v8::internal::LanguageMode) () at ../../v8/
src/ic/keyed-store-generic.cc:72
#1  0x0000555555d7d368 in KeyedStoreICMegamorphic () at
../../v8/src/builtins/builtins-handler.cc:57
#2  0x0000555555defab7 in BuildWithCodeStubAssemblerCS () at
../../v8/src/builtins/builtins.cc:109
#3  0x0000555555de9cbc in SetUp () at ../../v8/src/builtins/builtins.cc:153
#4  0x00005555565cc729 in Init () at ../../v8/src/isolate.cc:2536
#5  0x0000555555c8c7f4 in SnapshotCreator () at ../../v8/src/api.cc:485
#6  0x0000555555c8d603 in CreateSnapshotDataBlob () at ../../v8/src/api.cc:594
#7  0x0000555555c805bf in main () at ../../v8/src/snapshot/mksnapshot.cc:164

What happens here is three acts story:

1. BuildWithCodeStubAssemblerCS creates CodeStubAssembler assembler(isolate,
&zone, descriptor, flags, name) and calls generator(&assembler):
https://cs.chromium.org/chromium/src/v8/src/builtins/builtins.cc?q=v8/src/bui...

2. (this change is in this CL) KeyedStoreICMegamorphic calls
KeyedStoreGenericGenerator::Generate(assembler, &p, mode);
https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-handler.cc?q=sr...

3. KeyedStoreGenericGenerator::Generate assumes the assembler is
KeyedStoreGenericAssembler and makes a cast followed by a vcall:
auto assm = reinterpret_cast<KeyedStoreGenericAssembler*>(assembler);
assm->KeyedStoreGeneric(p, language_mode);

Then UBSan Vptr rightfully complains about this.
I am not sure what would be the right call here. May be revert the CL and
properly implement it, may be just fix it.

[krasin1, 2016-11-11 22:32:03]

I have filed https://bugs.chromium.org/p/v8/issues/detail?id=5628. Please, take
a look on Monday.

[hans, 2016-11-14 22:16:00]

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

[hans, 2016-11-14 22:16:01]

https://codereview.chromium.org/2444353002/diff/140001/src/ic/keyed-store-gen...
File src/ic/keyed-store-generic.cc (right):

https://codereview.chromium.org/2444353002/diff/140001/src/ic/keyed-store-gen...
src/ic/keyed-store-generic.cc:71: auto assm =
reinterpret_cast<KeyedStoreGenericAssembler*>(assembler);
This looks like a hack, and the reason for the UBSan error.

The code is trying to extend the CodeStubAssembler class with some methods for
use in this file, by creating a subclass and down-casting to it. I'm pretty sure
this isn't valid C++.

[Igor Sheludko, 2016-11-14 23:14:19]

On 2016/11/14 22:16:01, hans wrote:
>
https://codereview.chromium.org/2444353002/diff/140001/src/ic/keyed-store-gen...
> File src/ic/keyed-store-generic.cc (right):
> 
>
https://codereview.chromium.org/2444353002/diff/140001/src/ic/keyed-store-gen...
> src/ic/keyed-store-generic.cc:71: auto assm =
> reinterpret_cast<KeyedStoreGenericAssembler*>(assembler);
> This looks like a hack, and the reason for the UBSan error.
> 
> The code is trying to extend the CodeStubAssembler class with some methods for
> use in this file, by creating a subclass and down-casting to it. I'm pretty
sure
> this isn't valid C++.

Jakob is working on fixing this now.

[commit-bot: I haz the power, 2016-11-17 22:29:21]

Description was changed from

==========
[stubs] Port KeyedStoreIC_Megamorphic stub to Turbofan

BUG=v8:5269,v8:5561
==========

to

==========
[stubs] Port KeyedStoreIC_Megamorphic stub to Turbofan

BUG=v8:5269,v8:5561

Committed: https://crrev.com/5c03cb79228e4312e9722e46c681c0632eb55f2b
Cr-Commit-Position: refs/heads/master@{#40896}
==========

[commit-bot: I haz the power, 2016-11-17 22:29:22]

Patchset 8 (id:??) landed as
https://crrev.com/5c03cb79228e4312e9722e46c681c0632eb55f2b
Cr-Commit-Position: refs/heads/master@{#40896}