[interpreter] Add checks for source position to test-bytecode-generator.

[interpreter] Add checks for source position to test-bytecode-generator.

Prints source position information alongside bytecode.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/52600c6b1cc84c87d6f92f4b05e6f97af9143e60
Cr-Commit-Position: refs/heads/master@{#36171}

[oth, 2016-05-09 13:10:00]

oth@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[oth, 2016-05-09 13:10:01]

Ross, PTAL.

Stefano, FYI. Comments welcome.

Thanks.

[oth, 2016-05-09 13:12:04]

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

[commit-bot: I haz the power, 2016-05-09 13:12:11]

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

[commit-bot: I haz the power, 2016-05-09 13:38:03]

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

[commit-bot: I haz the power, 2016-05-09 13:38:04]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-05-09 15:36:52]

LGTM, thanks.

+Yang who was interested in adding source positions to the bytecode generator
tests.

https://codereview.chromium.org/1963663002/diff/1/test/cctest/interpreter/tes...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1963663002/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:112: source_code += '\n';
Does this get hit in the current code? Is there a corresponding implicit newline
in the generate_bytecode_expectations?

[Stefano Sanfilippo, 2016-05-09 15:45:46]

Code looks good, I don't particularly fancy the output format with the
surrounding /* */

[oth, 2016-05-09 16:14:44]

On 2016/05/09 15:45:46, Stefano Sanfilippo wrote:
> Code looks good, I don't particularly fancy the output format with the
> surrounding /* */

The format puts the source position on the same line as the bytecode it pertains
to and doesn't pack extra "bytes" into the bytecode array. The output is similar
to BytecodeArray::Print(). It'd be hard for people to reason about source
position changes if they are not on the same line.

Suggestions for alternatives welcome.

[Stefano Sanfilippo, 2016-05-09 16:25:39]

On 2016/05/09 16:14:44, oth wrote:
> On 2016/05/09 15:45:46, Stefano Sanfilippo wrote:
> > Code looks good, I don't particularly fancy the output format with the
> > surrounding /* */
> 
> The format puts the source position on the same line as the bytecode it
pertains
> to and doesn't pack extra "bytes" into the bytecode array. The output is
similar
> to BytecodeArray::Print(). It'd be hard for people to reason about source
> position changes if they are not on the same line.
> 
> Suggestions for alternatives welcome.

What about dropping the /* */? Like:

    54 S> B(LdaConstant), U8(0),
    54 E> B(Throw),
    B(LdaUndefined),
    72 S> B(Return),

WDYT?

[oth, 2016-05-10 06:03:24]

On 2016/05/09 16:25:39, Stefano Sanfilippo wrote:
> On 2016/05/09 16:14:44, oth wrote:
> > On 2016/05/09 15:45:46, Stefano Sanfilippo wrote:
> > > Code looks good, I don't particularly fancy the output format with the
> > > surrounding /* */
> > 
> > The format puts the source position on the same line as the bytecode it
> pertains
> > to and doesn't pack extra "bytes" into the bytecode array. The output is
> similar
> > to BytecodeArray::Print(). It'd be hard for people to reason about source
> > position changes if they are not on the same line.
> > 
> > Suggestions for alternatives welcome.
> 
> What about dropping the /* */? Like:
> 
>     54 S> B(LdaConstant), U8(0),
>     54 E> B(Throw),
>     B(LdaUndefined),
>     72 S> B(Return),
> 
> WDYT?

The suggestion drops the illusion of being a cut-n-pastable bag of C/C++ bytes
and has varying indentation which makes diffs harder to read. Changes in flight
are moving source positions around, it's easier to process these if the
bytecodes are aligned.

I experimented with putting the source positions in S(123) and E(412) "macros"
and also putting source and position info on separate lines. Neither of these
aided in reading diffs, hence the fixed column width in the proposed output.

WDYT about taking the output for the bytecode section from BytecodeArray::Print,
normalize the addresses, and put that in the golden files?

[Yang, 2016-05-10 09:25:14]

On 2016/05/10 06:03:24, oth wrote:
> On 2016/05/09 16:25:39, Stefano Sanfilippo wrote:
> > On 2016/05/09 16:14:44, oth wrote:
> > > On 2016/05/09 15:45:46, Stefano Sanfilippo wrote:
> > > > Code looks good, I don't particularly fancy the output format with the
> > > > surrounding /* */
> > > 
> > > The format puts the source position on the same line as the bytecode it
> > pertains
> > > to and doesn't pack extra "bytes" into the bytecode array. The output is
> > similar
> > > to BytecodeArray::Print(). It'd be hard for people to reason about source
> > > position changes if they are not on the same line.
> > > 
> > > Suggestions for alternatives welcome.
> > 
> > What about dropping the /* */? Like:
> > 
> >     54 S> B(LdaConstant), U8(0),
> >     54 E> B(Throw),
> >     B(LdaUndefined),
> >     72 S> B(Return),
> > 
> > WDYT?
> 
> The suggestion drops the illusion of being a cut-n-pastable bag of C/C++ bytes
> and has varying indentation which makes diffs harder to read. Changes in
flight
> are moving source positions around, it's easier to process these if the
> bytecodes are aligned.
> 
> I experimented with putting the source positions in S(123) and E(412) "macros"
> and also putting source and position info on separate lines. Neither of these
> aided in reading diffs, hence the fixed column width in the proposed output.
> 
> WDYT about taking the output for the bytecode section from
BytecodeArray::Print,
> normalize the addresses, and put that in the golden files?

LGTM. Thanks!

[oth, 2016-05-11 10:59:41]

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

[commit-bot: I haz the power, 2016-05-11 10:59:51]

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

[commit-bot: I haz the power, 2016-05-11 11:01:26]

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

[commit-bot: I haz the power, 2016-05-11 11:01:27]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/5584)
  v8_linux_chromium_gn_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_mac_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/1469)

[oth, 2016-05-11 11:08:56]

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

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

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

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

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

[commit-bot: I haz the power, 2016-05-11 11:11:53]

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

[oth, 2016-05-11 11:45:29]

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

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

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

[commit-bot: I haz the power, 2016-05-11 12:14:44]

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

[commit-bot: I haz the power, 2016-05-11 12:14:46]

Dry run: This issue passed the CQ dry run.

[oth, 2016-05-11 12:19:07]

The CQ bit was checked by oth@chromium.org

[oth, 2016-05-11 12:19:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1963663002/#ps40001
(title: "Fix bad implicit cast.")

[commit-bot: I haz the power, 2016-05-11 12:19:13]

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

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

Committed patchset #5 (id:40001)

[commit-bot: I haz the power, 2016-05-11 12:22:22]

Description was changed from

==========
[interpreter] Add checks for source position to test-bytecode-generator.

Prints source position information alongside bytecode.

BUG=v8:4280
LOG=N
==========

to

==========
[interpreter] Add checks for source position to test-bytecode-generator.

Prints source position information alongside bytecode.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/52600c6b1cc84c87d6f92f4b05e6f97af9143e60
Cr-Commit-Position: refs/heads/master@{#36171}
==========

[commit-bot: I haz the power, 2016-05-11 12:22:24]

Patchset 5 (id:??) landed as
https://crrev.com/52600c6b1cc84c87d6f92f4b05e6f97af9143e60
Cr-Commit-Position: refs/heads/master@{#36171}