making heap verification more aggressive

[runtime] making heap verification more aggressive

- check that packed elements do not contain the_hole (with fix)
- verify argument objects with elements kind
- use JSObjectVerifiy in all JSObject "subclasses"
- change initialization order for ArrayLiteralBoilerplate to simplify verification

BUG=v8:5188
Committed: https://crrev.com/599aa2e106ca8ab79f5cc489d1b93b6a26b19714
Cr-Commit-Position: refs/heads/master@{#37680}

[Camillo Bruni, 2016-07-07 11:53:52]

Description was changed from

==========
making heap verification more aggressive

BUG=
==========

to

==========
[runtime] making heap verification more aggressive

- check that packed elements do not contain the_hole (with fix)
- verify argument objects with elements kind
- use JSObjectVerifiy in all JSObject "subclasses"
- change initialization order for ArrayLiteralBoilerplate to simplify
verification

BUG=
==========

[Camillo Bruni, 2016-07-07 11:53:57]

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

[commit-bot: I haz the power, 2016-07-07 11:54:08]

Dry run: 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-07-07 12:16:31]

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

[commit-bot: I haz the power, 2016-07-07 12:16:32]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)

[Camillo Bruni, 2016-07-07 12:40:57]

Description was changed from

==========
[runtime] making heap verification more aggressive

- check that packed elements do not contain the_hole (with fix)
- verify argument objects with elements kind
- use JSObjectVerifiy in all JSObject "subclasses"
- change initialization order for ArrayLiteralBoilerplate to simplify
verification

BUG=
==========

to

==========
[runtime] making heap verification more aggressive

- check that packed elements do not contain the_hole (with fix)
- verify argument objects with elements kind
- use JSObjectVerifiy in all JSObject "subclasses"
- change initialization order for ArrayLiteralBoilerplate to simplify
verification

BUG=v8:5188
==========

[Camillo Bruni, 2016-07-07 15:52:57]

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

[commit-bot: I haz the power, 2016-07-07 15:53:08]

Dry run: 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-07-07 16:21:50]

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

[commit-bot: I haz the power, 2016-07-07 16:21:50]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2016-07-07 17:49:54]

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

[Camillo Bruni, 2016-07-07 17:49:54]

ulan@ PTAL heap.*
ishell@ PTAL the rest

There will be follow-up CLs once I manage to deal with the 3 issues I found

[ulan, 2016-07-08 06:19:40]

heap lgtm

https://codereview.chromium.org/2126613002/diff/20001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2126613002/diff/20001/src/factory.cc#newcode1655
src/factory.cc:1655: default:
case INITIALIZE_ARRAY_ELEMENTS_WITH_UNDEFINED: UNREACHABLE();

With enums it is better to avoid the default case. That way the compiler can
give warnings if some case is missing.

[Igor Sheludko, 2016-07-08 08:38:23]

https://codereview.chromium.org/2126613002/diff/20001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2126613002/diff/20001/src/factory.cc#newcode1668
src/factory.cc:1668: elms = NewFixedArray(capacity);
I think it's better to have breaks even in the last cases.

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:270: namespace {
Add a blank line after this line.

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:325: } else {
Instead of VerifySlowProperties:

} else if (obj->IsJSGlobalObject()) {
  VerifyDictionary(JSObject::cast(obj)->global_dictionary());
} else {
  VerifyDictionary(obj->property_dictionary());
}

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:356: CHECK(map->has_fast_object_elements() ==
obj->HasFastObjectElements());
CHECK_EQ?

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:358: // If a GC was caused while constructing this object,
the elements
I think this comment belongs only to ElementAreSafeToExamine().

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:369:
VerifyDictionary(SeededNumberDictionary::cast(elements));
I think this check also applies to SLOW_SLOPPY_ARGUMENTS_ELEMENTS and
SLOW_STRING_WRAPPER_ELEMENTS.

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:407: CHECK(FIRST_TYPE <= instance_type());
CHECK_LE?

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:523: Object* length = *Object::GetProperty(handle(this,
isolate),
This is scary. I guess heap verification should not cause any side effects at
all. Can you ensure somehow that you get the length without side effects?

In any case, if this call cause any allocations then when you return back,
"this" could not be a valid pointer anymore therefore all accesses to "this"
below should be done via handle.

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:529: PrintF(".");
Surprise! :)

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:543: void JSDate::JSDateVerify() {
JSDate and all other JSObject builtins should also be JSObjectVerify()-ed.

https://codereview.chromium.org/2126613002/diff/20001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-undefined-nan.js (right):

https://codereview.chromium.org/2126613002/diff/20001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-undefined-nan.js:24: assertTrue(isNaN(f_view [0]));
I can't lgtm a CL that contains a space between "f_view" and "[0]"! Never!

[Camillo Bruni, 2016-07-11 11:46:39]

ishell@ PTAL again, I changed the arguments validation quite a bit in the
meantime...

https://codereview.chromium.org/2126613002/diff/20001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2126613002/diff/20001/src/factory.cc#newcode1655
src/factory.cc:1655: default:
On 2016/07/08 at 06:19:40, ulan wrote:
> case INITIALIZE_ARRAY_ELEMENTS_WITH_UNDEFINED: UNREACHABLE();
> 
> With enums it is better to avoid the default case. That way the compiler can
give warnings if some case is missing.

done.

https://codereview.chromium.org/2126613002/diff/20001/src/factory.cc#newcode1668
src/factory.cc:1668: elms = NewFixedArray(capacity);
On 2016/07/08 at 08:38:22, Igor Sheludko wrote:
> I think it's better to have breaks even in the last cases.

done.

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:270: namespace {
On 2016/07/08 at 08:38:22, Igor Sheludko wrote:
> Add a blank line after this line.

done

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:325: } else {
On 2016/07/08 at 08:38:22, Igor Sheludko wrote:
> Instead of VerifySlowProperties:
> 
> } else if (obj->IsJSGlobalObject()) {
>   VerifyDictionary(JSObject::cast(obj)->global_dictionary());
> } else {
>   VerifyDictionary(obj->property_dictionary());
> }

Kept the separate method to do further checks but getting the dictionary the
suggested way.

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:356: CHECK(map->has_fast_object_elements() ==
obj->HasFastObjectElements());
On 2016/07/08 at 08:38:22, Igor Sheludko wrote:
> CHECK_EQ?

copy pasta

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:358: // If a GC was caused while constructing this object,
the elements
On 2016/07/08 at 08:38:22, Igor Sheludko wrote:
> I think this comment belongs only to ElementAreSafeToExamine().

right.

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:369:
VerifyDictionary(SeededNumberDictionary::cast(elements));
On 2016/07/08 at 08:38:22, Igor Sheludko wrote:
> I think this check also applies to SLOW_SLOPPY_ARGUMENTS_ELEMENTS and
SLOW_STRING_WRAPPER_ELEMENTS.

eventually I will have to put a switch statement here over all elements kind.
Will do in a follow-up CL...

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:407: CHECK(FIRST_TYPE <= instance_type());
On 2016/07/08 at 08:38:22, Igor Sheludko wrote:
> CHECK_LE?

done.

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:523: Object* length = *Object::GetProperty(handle(this,
isolate),
On 2016/07/08 at 08:38:22, Igor Sheludko wrote:
> This is scary. I guess heap verification should not cause any side effects at
all. Can you ensure somehow that you get the length without side effects?
> 
> In any case, if this call cause any allocations then when you return back,
"this" could not be a valid pointer anymore therefore all accesses to "this"
below should be done via handle.

right, also I just noticed that this can be easily overwritten from js side...

https://codereview.chromium.org/2126613002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:529: PrintF(".");
On 2016/07/08 at 08:38:22, Igor Sheludko wrote:
> Surprise! :)

... as good as unreachable.

https://codereview.chromium.org/2126613002/diff/20001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-undefined-nan.js (right):

https://codereview.chromium.org/2126613002/diff/20001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-undefined-nan.js:24: assertTrue(isNaN(f_view [0]));
On 2016/07/08 at 08:38:22, Igor Sheludko wrote:
> I can't lgtm a CL that contains a space between "f_view" and "[0]"! Never!

dammit! :P

[Igor Sheludko, 2016-07-11 15:37:10]

lgtm with nits:

https://codereview.chromium.org/2126613002/diff/80001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2126613002/diff/80001/src/factory.cc#newcode1658
src/factory.cc:1658: UNREACHABLE();
break;

https://codereview.chromium.org/2126613002/diff/80001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2126613002/diff/80001/src/objects-debug.cc#ne...
src/objects-debug.cc:534: CHECK(!(context->IsUndefined(isolate) &&
!arguments->IsUndefined(isolate)));
CHECK_IMPLIES?

https://codereview.chromium.org/2126613002/diff/80001/src/objects-debug.cc#ne...
src/objects-debug.cc:590: }
Either add JSObjectVerify(); here or even replace the above code with
JSValueVerify();

https://codereview.chromium.org/2126613002/diff/80001/src/objects-debug.cc#ne...
src/objects-debug.cc:642: VerifyObjectField(kStackFramesOffset);
JSObjectVerify();

[Camillo Bruni, 2016-07-12 12:38:12]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-07-12 12:38:12]

The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org,
ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2126613002/#ps120001
(title: "adding additional validation")

[commit-bot: I haz the power, 2016-07-12 12:38:20]

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

[Camillo Bruni, 2016-07-12 12:38:47]

The CQ bit was unchecked by cbruni@chromium.org

[Camillo Bruni, 2016-07-12 13:27:46]

The CQ bit was checked by cbruni@chromium.org

[commit-bot: I haz the power, 2016-07-12 13:27:55]

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-07-12 13:30:42]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-12 13:30:44]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-12 13:32:07]

Description was changed from

==========
[runtime] making heap verification more aggressive

- check that packed elements do not contain the_hole (with fix)
- verify argument objects with elements kind
- use JSObjectVerifiy in all JSObject "subclasses"
- change initialization order for ArrayLiteralBoilerplate to simplify
verification

BUG=v8:5188
==========

to

==========
[runtime] making heap verification more aggressive

- check that packed elements do not contain the_hole (with fix)
- verify argument objects with elements kind
- use JSObjectVerifiy in all JSObject "subclasses"
- change initialization order for ArrayLiteralBoilerplate to simplify
verification

BUG=v8:5188

Committed: https://crrev.com/599aa2e106ca8ab79f5cc489d1b93b6a26b19714
Cr-Commit-Position: refs/heads/master@{#37680}
==========

[commit-bot: I haz the power, 2016-07-12 13:32:08]

Patchset 7 (id:??) landed as
https://crrev.com/599aa2e106ca8ab79f5cc489d1b93b6a26b19714
Cr-Commit-Position: refs/heads/master@{#37680}

[Camillo Bruni, 2016-07-12 14:04:04]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2140163002/ by cbruni@chromium.org.

The reason for reverting is: failing gc stress tests:
https://build.chromium.org/p/client.v8/builders/V8%20Mac%20GC%20Stress/builds....