[interpreter] add source positions for call and call-new.

[interpreter] add source positions for call and call-new.

This change adds the basic infrastructure to record source
positions for bytecode.

R=rmcilroy@chromium.org, vogelheim@chromium.org
BUG=v8:4960
LOG=N

Committed: https://crrev.com/876d35e22815faebb8b9015329a7591e01e8b9e8
Cr-Commit-Position: refs/heads/master@{#33726}

[Yang, 2016-02-03 07:09:21]

Description was changed from

==========
[interpreter] add source positions for call and call-new.

R=rmcilroy@chromium.org, vogelheim@chromium.org
BUG=v8:4960
LOG=N
==========

to

==========
[interpreter] add source positions for call and call-new.

This change adds the basic infrastructure to record source
positions for bytecode.

R=rmcilroy@chromium.org, vogelheim@chromium.org
BUG=v8:4960
LOG=N
==========

[vogelheim, 2016-02-03 11:42:09]

lgtm. Nice!

https://codereview.chromium.org/1662983002/diff/1/src/interpreter/source-posi...
File src/interpreter/source-position-table.h (right):

https://codereview.chromium.org/1662983002/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:22: class SourcePositionTableBase {
I find this an odd use of inheritance. Why not have a class
SourcePositionAndIsStatement [well, that's arguably an awkward name, but
whatever] that has a uint32_t member and int + bool accessors, and that does the
mapping using BitField<...> internally?

Btw.. this is totally a matter of personal preferences, so please proceed as you
see fit. :)

https://codereview.chromium.org/1662983002/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:78: int is_statement() const { return
is_statement_; }
bool?

is_statement_ is bool, so this is an implicit promtion.

[rmcilroy, 2016-02-03 12:05:24]

lgtm, thanks.

https://codereview.chromium.org/1662983002/diff/1/src/interpreter/source-posi...
File src/interpreter/source-position-table.h (right):

https://codereview.chromium.org/1662983002/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:22: class SourcePositionTableBase {
On 2016/02/03 11:42:09, vogelheim wrote:
> I find this an odd use of inheritance. Why not have a class
> SourcePositionAndIsStatement [well, that's arguably an awkward name, but
> whatever] that has a uint32_t member and int + bool accessors, and that does
the
> mapping using BitField<...> internally?
> 
> Btw.. this is totally a matter of personal preferences, so please proceed as
you
> see fit. :)

+1

https://codereview.chromium.org/1662983002/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:41:
EncodeSourcePositionAndType(source_position, true)});
I think these should really be in the cc file (I think the general advice is
that only setters / getters should be implmented in headers, although that may
be chromium advice that isn't followed in V8).

https://codereview.chromium.org/1662983002/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:59: DCHECK_IMPLIES(entries_.size() > 0,
DHECK_IMPLIES got ripped out of chromium's base library for being confusing. I'd
suggest we should probably avoid using it for new things in V8 and possibly rip
it out of V8 in the future too for consistency.

https://codereview.chromium.org/1662983002/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:75: bool Next();
nit - most other v8 iterators seem to use Advance() and done() (e.g,
StackFrameIterator, BytecodeArrayIterator). I personally don't mind though -
feel free to keep it as is if you prefer this.

[Yang, 2016-02-03 14:16:20]

addressed comments.

https://codereview.chromium.org/1662983002/diff/1/src/interpreter/source-posi...
File src/interpreter/source-position-table.h (right):

https://codereview.chromium.org/1662983002/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:22: class SourcePositionTableBase {
On 2016/02/03 12:05:23, rmcilroy wrote:
> On 2016/02/03 11:42:09, vogelheim wrote:
> > I find this an odd use of inheritance. Why not have a class
> > SourcePositionAndIsStatement [well, that's arguably an awkward name, but
> > whatever] that has a uint32_t member and int + bool accessors, and that does
> the
> > mapping using BitField<...> internally?
> > 
> > Btw.. this is totally a matter of personal preferences, so please proceed as
> you
> > see fit. :)
> 
> +1

Done.

https://codereview.chromium.org/1662983002/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:41:
EncodeSourcePositionAndType(source_position, true)});
On 2016/02/03 12:05:23, rmcilroy wrote:
> I think these should really be in the cc file (I think the general advice is
> that only setters / getters should be implmented in headers, although that may
> be chromium advice that isn't followed in V8).

Done.

https://codereview.chromium.org/1662983002/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:59: DCHECK_IMPLIES(entries_.size() > 0,
On 2016/02/03 12:05:23, rmcilroy wrote:
> DHECK_IMPLIES got ripped out of chromium's base library for being confusing.
I'd
> suggest we should probably avoid using it for new things in V8 and possibly
rip
> it out of V8 in the future too for consistency.

Done.

https://codereview.chromium.org/1662983002/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:75: bool Next();
On 2016/02/03 12:05:24, rmcilroy wrote:
> nit - most other v8 iterators seem to use Advance() and done() (e.g,
> StackFrameIterator, BytecodeArrayIterator). I personally don't mind though -
> feel free to keep it as is if you prefer this.

Done.

https://codereview.chromium.org/1662983002/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:78: int is_statement() const { return
is_statement_; }
On 2016/02/03 11:42:09, vogelheim wrote:
> bool?
> 
> is_statement_ is bool, so this is an implicit promtion.

Done. Nice catch. Thanks.

[Yang, 2016-02-03 14:16:37]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-03 14:16:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/1662983002/#ps20001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-02-03 14:16:43]

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

[commit-bot: I haz the power, 2016-02-03 14:19:21]

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

[commit-bot: I haz the power, 2016-02-03 14:19:22]

Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/866)
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)

[Yang, 2016-02-04 06:11:57]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-04 06:11:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/1662983002/#ps40001
(title: "fix build")

[commit-bot: I haz the power, 2016-02-04 06:12:13]

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

[commit-bot: I haz the power, 2016-02-04 06:16:28]

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

[commit-bot: I haz the power, 2016-02-04 06:16:30]

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

[Yang, 2016-02-04 06:57:59]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-04 06:57:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/1662983002/#ps60001
(title: "fix build")

[commit-bot: I haz the power, 2016-02-04 06:58:04]

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

[commit-bot: I haz the power, 2016-02-04 07:11:22]

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

[commit-bot: I haz the power, 2016-02-04 07:11:23]

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

[Yang, 2016-02-04 08:05:50]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-04 08:05:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/1662983002/#ps80001
(title: "fix test expectation")

[commit-bot: I haz the power, 2016-02-04 08:05:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1662983002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1662983002/80001

[commit-bot: I haz the power, 2016-02-04 08:21:23]

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

[commit-bot: I haz the power, 2016-02-04 08:21:24]

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

[Yang, 2016-02-04 10:09:48]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-04 10:09:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/1662983002/#ps100001
(title: "fix test expectations")

[commit-bot: I haz the power, 2016-02-04 10:09:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1662983002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1662983002/100001

[commit-bot: I haz the power, 2016-02-04 10:50:05]

Description was changed from

==========
[interpreter] add source positions for call and call-new.

This change adds the basic infrastructure to record source
positions for bytecode.

R=rmcilroy@chromium.org, vogelheim@chromium.org
BUG=v8:4960
LOG=N
==========

to

==========
[interpreter] add source positions for call and call-new.

This change adds the basic infrastructure to record source
positions for bytecode.

R=rmcilroy@chromium.org, vogelheim@chromium.org
BUG=v8:4960
LOG=N
==========

[commit-bot: I haz the power, 2016-02-04 10:50:06]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-04 10:50:27]

Description was changed from

==========
[interpreter] add source positions for call and call-new.

This change adds the basic infrastructure to record source
positions for bytecode.

R=rmcilroy@chromium.org, vogelheim@chromium.org
BUG=v8:4960
LOG=N
==========

to

==========
[interpreter] add source positions for call and call-new.

This change adds the basic infrastructure to record source
positions for bytecode.

R=rmcilroy@chromium.org, vogelheim@chromium.org
BUG=v8:4960
LOG=N

Committed: https://crrev.com/876d35e22815faebb8b9015329a7591e01e8b9e8
Cr-Commit-Position: refs/heads/master@{#33726}
==========

[commit-bot: I haz the power, 2016-02-04 10:50:28]

Patchset 6 (id:??) landed as
https://crrev.com/876d35e22815faebb8b9015329a7591e01e8b9e8
Cr-Commit-Position: refs/heads/master@{#33726}