Remove unused 'receiver' field from generators

Remove unused 'receiver' field from generators

V8 previously handled the receiver ('this') specially with respect to
scoping, but it now handles it similarly to other local variables in
most contexts.

Generators had some code in them to restore the receiver on generator
resume, but this support is redundant with the way that contexts already
will restore the receiver. Further, async arrow functions cannot have
redundant reads and writes of the receiver as it would trigger a TDZ
error in some cases.

This patch eliminates the unnecesary saving of the receiver. To preserve
the calling convention, the hole value is restored as the receiver in
ResumeGeneratorTrampoline, similarly to how arguments are restored as
the hole, however it is expected that this value will never be seen by
user code because of the forced context allocation, which impacts the
receiver just like the arguments.

This patch fixes an async/await test in the unsubmitted draft code which
failed previously due to an unnecessary read of 'this' in a subclass
constructor when it was in TDZ.

R=neis,bmeurer
BUG=v8:4483

[Dan Ehrenberg, 2016-05-11 23:27:53]

Description was changed from

==========
Remove unused 'receiver' field from generators

V8 previously handled the receiver ('this') specially with respect to
scoping, but it now handles it similarly to other local variables in
most contexts.

Generators had some code in them to restore the receiver on generator
resume, but this support is redundant with the way that contexts already
will restore the receiver. Further, async arrow functions cannot have
redundant reads and writes of the receiver as it would trigger a TDZ
error in some cases.

This patch eliminates the unnecesary saving of the receiver. To preserve
the calling convention, the hole value is restored as the receiver in
ResumeGeneratorTrampoline, similarly to how arguments are restored as
the hole, however it is expected that this value will never be seen by
user code because of the forced context allocation, which impacts the
receiver just like the arguments.

This patch fixes an async/await test in the unsubmitted draft code which
failed previously due to an unnecessary read of 'this' in a subclass
constructor when it was in TDZ.

R=neis
BUG=v8:4483
==========

to

==========
Remove unused 'receiver' field from generators

V8 previously handled the receiver ('this') specially with respect to
scoping, but it now handles it similarly to other local variables in
most contexts.

Generators had some code in them to restore the receiver on generator
resume, but this support is redundant with the way that contexts already
will restore the receiver. Further, async arrow functions cannot have
redundant reads and writes of the receiver as it would trigger a TDZ
error in some cases.

This patch eliminates the unnecesary saving of the receiver. To preserve
the calling convention, the hole value is restored as the receiver in
ResumeGeneratorTrampoline, similarly to how arguments are restored as
the hole, however it is expected that this value will never be seen by
user code because of the forced context allocation, which impacts the
receiver just like the arguments.

This patch fixes an async/await test in the unsubmitted draft code which
failed previously due to an unnecessary read of 'this' in a subclass
constructor when it was in TDZ.

R=neis,bmeurer
BUG=v8:4483
==========

[Dan Ehrenberg, 2016-05-11 23:27:53]

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

[Dan Ehrenberg, 2016-05-11 23:32:42]

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

[commit-bot: I haz the power, 2016-05-11 23:32:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1968263002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1968263002/20001

[commit-bot: I haz the power, 2016-05-11 23:52:02]

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

[commit-bot: I haz the power, 2016-05-11 23:52:03]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[Dan Ehrenberg, 2016-05-11 23:54:44]

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

[commit-bot: I haz the power, 2016-05-11 23:54:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1968263002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1968263002/40001

[commit-bot: I haz the power, 2016-05-12 00:13:24]

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

[commit-bot: I haz the power, 2016-05-12 00:13:27]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[Dan Ehrenberg, 2016-05-12 00:29:09]

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

[commit-bot: I haz the power, 2016-05-12 00:29:13]

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

[commit-bot: I haz the power, 2016-05-12 00:48:25]

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

[commit-bot: I haz the power, 2016-05-12 00:48:26]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_arm_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)

[Benedikt Meurer, 2016-05-12 04:04:46]

I don't think this will work. This will break the FrameSummary for resumed
generator frames, and by that it'll probably break a lot of users of the
FrameSummary, because now the receiver in the FrameSummary is the_hole.

[Dan Ehrenberg, 2016-05-12 04:47:12]

On 2016/05/12 at 04:04:46, bmeurer wrote:
> I don't think this will work. This will break the FrameSummary for resumed
generator frames, and by that it'll probably break a lot of users of the
FrameSummary, because now the receiver in the FrameSummary is the_hole.

For async arrow functions to be able to call super (obscure, I know), the real
receiver needs to be context-allocated, and we cannot have an accurate receiver
copied into the generator. How should we handle this?

[Benedikt Meurer, 2016-05-12 17:50:19]

On 2016/05/12 04:47:12, Dan Ehrenberg wrote:
> On 2016/05/12 at 04:04:46, bmeurer wrote:
> > I don't think this will work. This will break the FrameSummary for resumed
> generator frames, and by that it'll probably break a lot of users of the
> FrameSummary, because now the receiver in the FrameSummary is the_hole.
> 
> For async arrow functions to be able to call super (obscure, I know), the real
> receiver needs to be context-allocated, and we cannot have an accurate
receiver
> copied into the generator. How should we handle this?

I doesn't matter what you copy into the generator, that's completely orthogonal
to the issue at hand. I talked briefly with Andreas and Yang today, and we
should focus on fixing the issue that makes the test fail. Optimizing the
generator suspend/resume by not saving the receiver slot is an optimization that
we can discuss afterwards (there are some additional constraints due to
FrameSummary and some debugger interaction, which are unrelated to
super/async/generators).

So, if I understand correctly, the bug is due to the multiple ThisExpression ast
nodes being emitted for yields?

[Benedikt Meurer, 2016-05-12 17:50:39]

bmeurer@chromium.org changed reviewers:
+ rossberg@chromium.org, yangguo@chromium.org

[Dan Ehrenberg, 2016-05-12 17:56:57]

On 2016/05/12 at 17:50:19, bmeurer wrote:
> On 2016/05/12 04:47:12, Dan Ehrenberg wrote:
> > On 2016/05/12 at 04:04:46, bmeurer wrote:
> > > I don't think this will work. This will break the FrameSummary for resumed
> > generator frames, and by that it'll probably break a lot of users of the
> > FrameSummary, because now the receiver in the FrameSummary is the_hole.
> > 
> > For async arrow functions to be able to call super (obscure, I know), the
real
> > receiver needs to be context-allocated, and we cannot have an accurate
receiver
> > copied into the generator. How should we handle this?
> 
> I doesn't matter what you copy into the generator, that's completely
orthogonal to the issue at hand. I talked briefly with Andreas and Yang today,
and we should focus on fixing the issue that makes the test fail. Optimizing the
generator suspend/resume by not saving the receiver slot is an optimization that
we can discuss afterwards (there are some additional constraints due to
FrameSummary and some debugger interaction, which are unrelated to
super/async/generators).
> 
> So, if I understand correctly, the bug is due to the multiple ThisExpression
ast nodes being emitted for yields?

The issue is the read of 'this' to be passed into CreateJSGeneratorObject, which
might be in the TDZ in case it's in a subclass constructor. I can simply replace
this with 'undefined' when it's an async arrow function, if that's how arrows
generally work with the receiver in the stack frame.