[generators] Implement %GeneratorGetSourcePosition.

[generators] Implement %GeneratorGetSourcePosition.

This runtime function now also works for Ignition generators. It returns the
source position of the yield at which a suspended generator got suspended.  This
works by storing the current bytecode offset at suspension and using an existing
mechanism to map it back to a source position.

TBR=littledan@chromium.org
BUG=

Committed: https://crrev.com/7c57ffc1dfaffe0cf9fa32e6d02b3667aa0e954a
Cr-Commit-Position: refs/heads/master@{#37140}

[neis, 2016-06-17 14:17:15]

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

[commit-bot: I haz the power, 2016-06-17 14:17:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2079613003/1

[commit-bot: I haz the power, 2016-06-17 14:44:41]

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

[commit-bot: I haz the power, 2016-06-17 14:44:42]

Dry run: This issue passed the CQ dry run.

[neis, 2016-06-17 15:11:20]

Description was changed from

==========
[generators] Implement %GeneratorGetSourcePosition.

This runtime function now also works for Ignition generators. It returns the
source position of the yield at which a suspended generator got suspended.  This
works by storing the current bytecode offset at suspension and using an existing
mechanism to map it back to a source position.

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

to

==========
[generators] Implement %GeneratorGetSourcePosition.

This runtime function now also works for Ignition generators. It returns the
source position of the yield at which a suspended generator got suspended.  This
works by storing the current bytecode offset at suspension and using an existing
mechanism to map it back to a source position.

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

[neis, 2016-06-17 15:11:20]

neis@chromium.org changed reviewers:
+ jarin@chromium.org, rmcilroy@chromium.org, yangguo@chromium.org

[neis, 2016-06-17 15:15:11]

ptal

[neis, 2016-06-17 15:17:22]

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

[commit-bot: I haz the power, 2016-06-17 15:17:24]

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

[commit-bot: I haz the power, 2016-06-17 15:39:11]

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

[commit-bot: I haz the power, 2016-06-17 15:39:12]

Dry run: This issue passed the CQ dry run.

[Jarin, 2016-06-20 07:56:50]

lgtm

https://codereview.chromium.org/2079613003/diff/20001/src/compiler/js-typed-l...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/2079613003/diff/20001/src/compiler/js-typed-l...
src/compiler/js-typed-lowering.cc:1740: FieldAccess input_field =
AccessBuilder::ForJSGeneratorObjectInput();
Please, add some explanation why it is ok to hijack the input field for offset.
Maybe we should consider to rename the field to reflect the dual purpose of the
field.

[rmcilroy, 2016-06-20 11:11:26]

LGTM with one suggestion.

https://codereview.chromium.org/2079613003/diff/20001/src/compiler/js-typed-l...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/2079613003/diff/20001/src/compiler/js-typed-l...
src/compiler/js-typed-lowering.cc:1740: FieldAccess input_field =
AccessBuilder::ForJSGeneratorObjectInput();
On 2016/06/20 07:56:49, Jarin wrote:
> Please, add some explanation why it is ok to hijack the input field for
offset.
> Maybe we should consider to rename the field to reflect the dual purpose of
the
> field.

+1

https://codereview.chromium.org/2079613003/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2079613003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1825: __ StoreObjectField(generator,
JSGeneratorObject::kInputOffset, offset);
Could we just store the non-subtracted value in the generator object here and do
the subtraction in JSGeneratorObject::source_position? This would be consistent
with the approach we use to push the bytecode offset on the stack used for stack
traces (where the subtraction happens in InterpretedFrame::GetBytecodeOffset())

[Yang, 2016-06-21 06:34:43]

On 2016/06/20 11:11:26, rmcilroy wrote:
> LGTM with one suggestion.
> 
>
https://codereview.chromium.org/2079613003/diff/20001/src/compiler/js-typed-l...
> File src/compiler/js-typed-lowering.cc (right):
> 
>
https://codereview.chromium.org/2079613003/diff/20001/src/compiler/js-typed-l...
> src/compiler/js-typed-lowering.cc:1740: FieldAccess input_field =
> AccessBuilder::ForJSGeneratorObjectInput();
> On 2016/06/20 07:56:49, Jarin wrote:
> > Please, add some explanation why it is ok to hijack the input field for
> offset.
> > Maybe we should consider to rename the field to reflect the dual purpose of
> the
> > field.
> 
> +1
> 
>
https://codereview.chromium.org/2079613003/diff/20001/src/interpreter/interpr...
> File src/interpreter/interpreter.cc (right):
> 
>
https://codereview.chromium.org/2079613003/diff/20001/src/interpreter/interpr...
> src/interpreter/interpreter.cc:1825: __ StoreObjectField(generator,
> JSGeneratorObject::kInputOffset, offset);
> Could we just store the non-subtracted value in the generator object here and
do
> the subtraction in JSGeneratorObject::source_position? This would be
consistent
> with the approach we use to push the bytecode offset on the stack used for
stack
> traces (where the subtraction happens in
InterpretedFrame::GetBytecodeOffset())

lgtm.

[neis, 2016-06-21 07:36:06]

On 2016/06/20 11:11:26, rmcilroy wrote:
>
https://codereview.chromium.org/2079613003/diff/20001/src/interpreter/interpr...
> src/interpreter/interpreter.cc:1825: __ StoreObjectField(generator,
> JSGeneratorObject::kInputOffset, offset);
> Could we just store the non-subtracted value in the generator object here and
do
> the subtraction in JSGeneratorObject::source_position? This would be
consistent
> with the approach we use to push the bytecode offset on the stack used for
stack
> traces (where the subtraction happens in
InterpretedFrame::GetBytecodeOffset())

That's what I wanted to do, but on the Turbofan side (in the bytecode graph
builder) I have the other kind of offset, so I would have to artificially add
something there just to subtract it again later.

[rmcilroy, 2016-06-21 08:38:46]

On 2016/06/21 07:36:06, neis wrote:
> On 2016/06/20 11:11:26, rmcilroy wrote:
> >
>
https://codereview.chromium.org/2079613003/diff/20001/src/interpreter/interpr...
> > src/interpreter/interpreter.cc:1825: __ StoreObjectField(generator,
> > JSGeneratorObject::kInputOffset, offset);
> > Could we just store the non-subtracted value in the generator object here
and
> do
> > the subtraction in JSGeneratorObject::source_position? This would be
> consistent
> > with the approach we use to push the bytecode offset on the stack used for
> stack
> > traces (where the subtraction happens in
> InterpretedFrame::GetBytecodeOffset())
> 
> That's what I wanted to do, but on the Turbofan side (in the bytecode graph
> builder) I have the other kind of offset, so I would have to artificially add
> something there just to subtract it again later.

Sure, but then you would do that once at compile time rather than at every
generator yield in the interpreter.

[neis, 2016-06-21 11:17:26]

On 2016/06/21 08:38:46, rmcilroy wrote:
> On 2016/06/21 07:36:06, neis wrote:
> > On 2016/06/20 11:11:26, rmcilroy wrote:
> > >
> >
>
https://codereview.chromium.org/2079613003/diff/20001/src/interpreter/interpr...
> > > src/interpreter/interpreter.cc:1825: __ StoreObjectField(generator,
> > > JSGeneratorObject::kInputOffset, offset);
> > > Could we just store the non-subtracted value in the generator object here
> and
> > do
> > > the subtraction in JSGeneratorObject::source_position? This would be
> > consistent
> > > with the approach we use to push the bytecode offset on the stack used for
> > stack
> > > traces (where the subtraction happens in
> > InterpretedFrame::GetBytecodeOffset())
> > 
> > That's what I wanted to do, but on the Turbofan side (in the bytecode graph
> > builder) I have the other kind of offset, so I would have to artificially
add
> > something there just to subtract it again later.
> 
> Sure, but then you would do that once at compile time rather than at every
> generator yield in the interpreter.

Alright, changed it.

[neis, 2016-06-21 11:17:41]

The CQ bit was checked by neis@chromium.org

[neis, 2016-06-21 11:17:42]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org,
rmcilroy@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2079613003/#ps60001
(title: "rename again, and move subtraction to runtime")

[commit-bot: I haz the power, 2016-06-21 11:17:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2079613003/60001

[commit-bot: I haz the power, 2016-06-21 11:22:04]

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

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

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/17651)

[neis, 2016-06-21 11:23:04]

Description was changed from

==========
[generators] Implement %GeneratorGetSourcePosition.

This runtime function now also works for Ignition generators. It returns the
source position of the yield at which a suspended generator got suspended.  This
works by storing the current bytecode offset at suspension and using an existing
mechanism to map it back to a source position.

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

to

==========
[generators] Implement %GeneratorGetSourcePosition.

This runtime function now also works for Ignition generators. It returns the
source position of the yield at which a suspended generator got suspended.  This
works by storing the current bytecode offset at suspension and using an existing
mechanism to map it back to a source position.

TBR=littledan@chromium.org
BUG=
==========

[neis, 2016-06-21 11:43:24]

The CQ bit was checked by neis@chromium.org

[commit-bot: I haz the power, 2016-06-21 11:43:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2079613003/60001

[commit-bot: I haz the power, 2016-06-21 12:12:57]

Description was changed from

==========
[generators] Implement %GeneratorGetSourcePosition.

This runtime function now also works for Ignition generators. It returns the
source position of the yield at which a suspended generator got suspended.  This
works by storing the current bytecode offset at suspension and using an existing
mechanism to map it back to a source position.

TBR=littledan@chromium.org
BUG=
==========

to

==========
[generators] Implement %GeneratorGetSourcePosition.

This runtime function now also works for Ignition generators. It returns the
source position of the yield at which a suspended generator got suspended.  This
works by storing the current bytecode offset at suspension and using an existing
mechanism to map it back to a source position.

TBR=littledan@chromium.org
BUG=
==========

[commit-bot: I haz the power, 2016-06-21 12:12:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-21 12:13:55]

Description was changed from

==========
[generators] Implement %GeneratorGetSourcePosition.

This runtime function now also works for Ignition generators. It returns the
source position of the yield at which a suspended generator got suspended.  This
works by storing the current bytecode offset at suspension and using an existing
mechanism to map it back to a source position.

TBR=littledan@chromium.org
BUG=
==========

to

==========
[generators] Implement %GeneratorGetSourcePosition.

This runtime function now also works for Ignition generators. It returns the
source position of the yield at which a suspended generator got suspended.  This
works by storing the current bytecode offset at suspension and using an existing
mechanism to map it back to a source position.

TBR=littledan@chromium.org
BUG=

Committed: https://crrev.com/7c57ffc1dfaffe0cf9fa32e6d02b3667aa0e954a
Cr-Commit-Position: refs/heads/master@{#37140}
==========

[commit-bot: I haz the power, 2016-06-21 12:13:57]

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