[objects] Relax JSBoundFunction verification.

[objects] Relax JSBoundFunction verification.

The heap verifier does certain invariant checks on JSBoundFunction
objects, i.e. it assumes that the bound_target_function is a proper
JSReceiver. The Deoptimizer cannot maintain this invariant, because it
first allocates the JSBoundFunction in an invalid state and only
afterwards fix up the state. But the GC (and thus the heap verifier)
can observe this invalid state why materializing field values, so
we need to relax the verification slightly.

BUG=chromium:729573, chromium:732176
R=mstarzinger@chromium.org

Review-Url: https://codereview.chromium.org/2933283002
Cr-Commit-Position: refs/heads/master@{#45988}
Committed: https://chromium.googlesource.com/v8/v8/+/a9b9c7ab8ca8c0ae749eb119418fb4cc037a93de

[Benedikt Meurer, 2017-06-13 06:57:28]

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

[commit-bot: I haz the power, 2017-06-13 06:57:38]

Dry run: CQ is trying da patch.

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

[Benedikt Meurer, 2017-06-13 06:58:36]

Hey Michi,

Here's the fix for the Deoptimizer unhappiness with my JSBoundFunction patch. I
didn't add a regression test, since that is highly sensitive to triggering GC at
the right time and thus not very useful to me.
Please take a look.

Thanks,
Benedikt

[commit-bot: I haz the power, 2017-06-13 07:22:56]

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

[commit-bot: I haz the power, 2017-06-13 07:22:56]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2017-06-13 07:57:32]

https://codereview.chromium.org/2933283002/diff/1/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/2933283002/diff/1/src/deoptimizer.cc#newcode4436
src/deoptimizer.cc:4436: slot->value_ = object;
This only works because escape analysis currently doesn't support cycles. As
soon as cycles are supported it might be that the bound function references
itself in the properties or elements backing store above. Please add a big TODO
here in that regard, explaining that this should be move up again.

[Michael Starzinger, 2017-06-13 07:58:27]

LGTM once comment is addressed.

[Benedikt Meurer, 2017-06-13 08:21:08]

Hm, this sounds dangerous. Should we relax the object verifier instead?

[Michael Starzinger, 2017-06-13 09:13:16]

mstarzinger@chromium.org changed reviewers:
+ jarin@chromium.org

[Michael Starzinger, 2017-06-13 09:13:16]

Jaro and I just had a brief chat about this as well. He also suggested relaxing
the verifier. I would be totally fine with that, it might turn out to be less
dangerous in the long run. Also note that we already have relaxed verifiers in
other places (and for other reasons), see for example
JSGeneratorObject::JSGeneratorObjectVerify and the comment in that function.

[Jarin, 2017-06-13 09:28:53]

On 2017/06/13 09:13:16, Michael Starzinger wrote:
> Jaro and I just had a brief chat about this as well. He also suggested
relaxing
> the verifier. I would be totally fine with that, it might turn out to be less
> dangerous in the long run. Also note that we already have relaxed verifiers in
> other places (and for other reasons), see for example
> JSGeneratorObject::JSGeneratorObjectVerify and the comment in that function.

Indeed, we already relaxed the verifier for other cases. Creating the object
late is inconsistent with all the other cases (also, it is a landmine waiting 
for someone).

[Benedikt Meurer, 2017-06-19 06:39:58]

Description was changed from

==========
[deoptimizer] Avoid JSBoundFunction invalid state.

The heap verifier does certain invariant checks on JSBoundFunction
objects, i.e. it assumes that the bound_target_function is a proper
JSReceiver. The Deoptimizer didn't maintain this invariant, because it
would first allocate the JSBoundFunction in an invalid state and only
afterwards fix up the state. But the GC (and thus the heap verifier)
could observer this invalid state why materializing field values.

BUG=chromium:729573,chromium:732176
R=mstarzinger@chromium.org
==========

to

==========
[objects] Relax JSBoundFunction verification.

The heap verifier does certain invariant checks on JSBoundFunction
objects, i.e. it assumes that the bound_target_function is a proper
JSReceiver. The Deoptimizer cannot maintain this invariant, because it
first allocates the JSBoundFunction in an invalid state and only
afterwards fix up the state. But the GC (and thus the heap verifier)
can observe this invalid state why materializing field values, so
we need to relax the verification slightly.

BUG=chromium:729573,chromium:732176
R=mstarzinger@chromium.org
==========

[Benedikt Meurer, 2017-06-19 06:40:26]

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

[Benedikt Meurer, 2017-06-19 06:40:33]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2017-06-19 06:40:36]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2017-06-19 06:40:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2933283002/#ps20001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-06-19 06:40:46]

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, 2017-06-19 07:09:07]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497854436505810,
"parent_rev": "8a32788f39e1472ecf7190e1b837973688518dd9", "commit_rev":
"a9b9c7ab8ca8c0ae749eb119418fb4cc037a93de"}

[commit-bot: I haz the power, 2017-06-19 07:09:15]

Description was changed from

==========
[objects] Relax JSBoundFunction verification.

The heap verifier does certain invariant checks on JSBoundFunction
objects, i.e. it assumes that the bound_target_function is a proper
JSReceiver. The Deoptimizer cannot maintain this invariant, because it
first allocates the JSBoundFunction in an invalid state and only
afterwards fix up the state. But the GC (and thus the heap verifier)
can observe this invalid state why materializing field values, so
we need to relax the verification slightly.

BUG=chromium:729573,chromium:732176
R=mstarzinger@chromium.org
==========

to

==========
[objects] Relax JSBoundFunction verification.

The heap verifier does certain invariant checks on JSBoundFunction
objects, i.e. it assumes that the bound_target_function is a proper
JSReceiver. The Deoptimizer cannot maintain this invariant, because it
first allocates the JSBoundFunction in an invalid state and only
afterwards fix up the state. But the GC (and thus the heap verifier)
can observe this invalid state why materializing field values, so
we need to relax the verification slightly.

BUG=chromium:729573,chromium:732176
R=mstarzinger@chromium.org

Review-Url: https://codereview.chromium.org/2933283002
Cr-Commit-Position: refs/heads/master@{#45988}
Committed:
https://chromium.googlesource.com/v8/v8/+/a9b9c7ab8ca8c0ae749eb119418fb4cc037...
==========

[commit-bot: I haz the power, 2017-06-19 07:09:16]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/a9b9c7ab8ca8c0ae749eb119418fb4cc037...