[coverage] Block coverage with support for IfStatements

[coverage] Block coverage with support for IfStatements

This CL implements general infrastructure for block coverage together with
initial support for if-statements.

Coverage output can be generated in lcov format by d8 as follows: 

$ d8 --block-coverage --lcov=$(echo ~/simple-if.lcov) ~/simple-if.js
$ genhtml ~/simple-if.lcov -o ~/simple-if
$ chrome ~/simple-if/index.html

A high level overview of the implementation follows:

The parser now collects source ranges unconditionally for relevant AST nodes.
Memory overhead is very low and this seemed like the cleanest and simplest
alternative.

Bytecode generation uses these ranges to allocate coverage slots and insert
IncBlockCounter instructions (e.g. at the beginning of then- and else blocks
for if-statements). The slot-range mapping is generated here and passed on
through CompilationInfo, and is later accessible through the
SharedFunctionInfo.

The IncBlockCounter bytecode fetches the slot-range mapping (called
CoverageInfo) from the shared function info and simply increments the counter.
We don't collect native-context-specific counts as they are irrelevant to our
use-cases.

Coverage information is finally generated on-demand through Coverage::Collect.
The only current consumer is a d8 front-end with lcov-style output, but the
short-term goal is to expose this through the inspector protocol.

BUG=v8:6000
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng

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

[jgruber, 2017-05-15 08:26:08]

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

[commit-bot: I haz the power, 2017-05-15 08:26:12]

Dry run: 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-05-15 08:34:17]

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

[commit-bot: I haz the power, 2017-05-15 08:34:18]

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

[jgruber, 2017-05-17 14:11:27]

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

[commit-bot: I haz the power, 2017-05-17 14:11:32]

Dry run: CQ is trying da patch.

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

[jgruber, 2017-05-17 14:17:21]

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

[commit-bot: I haz the power, 2017-05-17 14:17:32]

Dry run: 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-05-17 15:32:18]

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

[commit-bot: I haz the power, 2017-05-17 15:32:19]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-05-17 16:03:35]

Description was changed from

==========
[coverage] Block coverage prototype

Initial prototype that implements block coverage for if statements.

Includes plenty of hacky solutions where we need to figure out something better
before landing this permanently.

Usage:

out/debug/d8 --block-coverage --lcov=$(echo ~/simple-if.lcov) ~/simple-if.js
genhtml ~/simple-if.lcov -o ~/simple-if
google-chrome ~/simple-if/index.html

BUG=v8:6000
==========

to

==========
[coverage] Block coverage prototype

Initial prototype that implements block coverage for if statements.

Usage:

out/debug/d8 --block-coverage --lcov=$(echo ~/simple-if.lcov) ~/simple-if.js
genhtml ~/simple-if.lcov -o ~/simple-if
google-chrome ~/simple-if/index.html

BUG=v8:6000
==========

[jgruber, 2017-05-17 16:14:52]

jgruber@chromium.org changed reviewers:
+ marja@chromium.org, rmcilroy@chromium.org

[jgruber, 2017-05-17 16:14:53]

Hi Marja, Ross,

the block coverage prototype is getting closer in shape to how I can imagine
landing it, and it'd be great to get your feedback on ast/ & parser/ (Marja) and
interpreter/ (Ross).

Note that this only handles if-statements, so expect plenty more code to handle
other relevant AstNodes in ast/, parser/ (collect and store source range
information), and interpreter/bytecode-generator (generate IncBlockCounter
bytecodes). 

The block coverage feature is currently enabled through FLAG_block_coverage, but
this will still change (I guess we'll pass in a flag through ParseInfo).

PTAL and let me know if you have concerns about the current approach.

[marja, 2017-05-18 11:30:17]

ast/* and parsing/* lgtm with comments

https://codereview.chromium.org/2882973002/diff/60001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2882973002/diff/60001/src/ast/ast.h#newcode187
src/ast/ast.h:187: int32_t from, to;
In Scope, FunctionLiteral etc. there's already start_position and end_position,
I think it makes sense to have consistent naming there. I.e., these should be
start_position and end_position instead of from and to.

https://codereview.chromium.org/2882973002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2882973002/diff/60001/src/objects.h#newcode10066
src/objects.h:10066: class CoverageInfo : public FixedArray {
Please put this in a different file than objects.h. Somewhere in src/objects/ .
Maybe you want to combine with other related classes.

[rmcilroy, 2017-05-18 14:18:00]

Just looked at the interpreter bits. Overall looks reasonable, but a couple of
suggestions. Also, are you going to do this for for/while loops as well (since
they might never loop)?

https://codereview.chromium.org/2882973002/diff/60001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2882973002/diff/60001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:2223: PrepareEagerCheckpoint();
I don't think you would need a Checkpoint for this bytecode since it can't call
into JS - just add the runtime call into the whitelist in
Linkage::NeedsFrameStateInput.

https://codereview.chromium.org/2882973002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2882973002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:665:
info()->shared_info()->set_coverage_info(*coverage_info);
We don't set things in the shared_info in the bytecode generator and I would
rather we didn't do this here. Can we instead set something on the CompileInfo
(maybe even just the slot count for allocation at a later point).

https://codereview.chromium.org/2882973002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/2882973002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:234: }
How about factoring this out into a small helper class (similar to
GlobalDeclarationBuilder) and just create the builder in the BytecodeGenerator's
constructor if:
  FLAG_block_coverage && info()->parse_info()->script()->IsUserJavaScript()

then just check for:
  if (block_coverage_builder())
block_coverage_builder()->IncrementBlock(SourceRange range);

in the if / else visitor, then you don't need a is_block_coverage() function,
and we don't have the overhead of an empty ZoneVector when block coverage is
turned off. WDYT?

https://codereview.chromium.org/2882973002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/2882973002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:336: V(IncBlockCounter, AccumulatorUse::kNone,
OperandType::kImm)                 \
use kIdx if these are unsigned (will give more range in non-wide bytecodes and
no need to check DCHECK_GE(slot, 0); in the bytecode array-builder as this will
be done when converting to an operand.

https://codereview.chromium.org/2882973002/diff/60001/src/interpreter/interpr...
File src/interpreter/interpreter-generator.cc (right):

https://codereview.chromium.org/2882973002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-generator.cc:3152: Node* slot =
BytecodeOperandImmSmi(0);
nit - slot -> coverage_array_slot (to avoid confusion with feedback_vector
slot).

https://codereview.chromium.org/2882973002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2882973002/diff/60001/src/objects.h#newcode1580
src/objects.h:1580: static inline int ToInt(Object* o);
Is this worth adding rather than just Smi::cast(o)->value() ?

https://codereview.chromium.org/2882973002/diff/60001/src/objects.h#newcode6279
src/objects.h:6279: static const int kCoverageInfoOffset = kDebugInfoOffset +
kPointerSize;
I don't think we want to add this as a field to SFI, that will be a reasonable
memory overhead even when block coverage is turned off. Can it be added to
DebugInfo or somewhere else which is allocated on-demand?

https://codereview.chromium.org/2882973002/diff/60001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/2882973002/diff/60001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:1979: CONVERT_ARG_HANDLE_CHECKED(FeedbackVector,
vector, 0);
Unused?

https://codereview.chromium.org/2882973002/diff/60001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:1980: CONVERT_SMI_ARG_CHECKED(slot_index, 1);
coverage_array_slot

[jgruber, 2017-05-22 09:43:32]

Thanks for the look, answers inline.

> Also, are you going to do this for for/while loops as well (since
> they might never loop)?

Yes, this will be extended to loops & iteration, conditional & unconditional
control flow, short-circuiting boolean ops, exceptions, and similar.

The prototype only handles if-statements, but I think it should be possible to
land something close to the current form and then extend support iteratively.

Short-term plan: Move CoverageInfo to SharedFunctionInfo::debug_info, then begin
to land things.

https://codereview.chromium.org/2882973002/diff/60001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2882973002/diff/60001/src/ast/ast.h#newcode187
src/ast/ast.h:187: int32_t from, to;
On 2017/05/18 11:30:17, marja wrote:
> In Scope, FunctionLiteral etc. there's already start_position and
end_position,
> I think it makes sense to have consistent naming there. I.e., these should be
> start_position and end_position instead of from and to.

No very strong feelings on this from my side, but if possible I'd prefer keeping
the names short. What about {start, end}?

Maybe we could also change Scope, FunctionLiteral etc. to contain a SourceRange
at some point.

https://codereview.chromium.org/2882973002/diff/60001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2882973002/diff/60001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:2223: PrepareEagerCheckpoint();
On 2017/05/18 14:17:59, rmcilroy wrote:
> I don't think you would need a Checkpoint for this bytecode since it can't
call
> into JS - just add the runtime call into the whitelist in
> Linkage::NeedsFrameStateInput.

Done.

https://codereview.chromium.org/2882973002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2882973002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:665:
info()->shared_info()->set_coverage_info(*coverage_info);
On 2017/05/18 14:17:59, rmcilroy wrote:
> We don't set things in the shared_info in the bytecode generator and I would
> rather we didn't do this here. Can we instead set something on the CompileInfo
> (maybe even just the slot count for allocation at a later point).

I changed this to store the CoverageInfo on CompilationInfo, and move it to the
SFI in FinalizeUnoptimizedCompilationJob.

(The mapping between coverage slot and source range needs to happen before
bytecode generation so we can't pass only the slot count.)

https://codereview.chromium.org/2882973002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/2882973002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:336: V(IncBlockCounter, AccumulatorUse::kNone,
OperandType::kImm)                 \
On 2017/05/18 14:17:59, rmcilroy wrote:
> use kIdx if these are unsigned (will give more range in non-wide bytecodes and
> no need to check DCHECK_GE(slot, 0); in the bytecode array-builder as this
will
> be done when converting to an operand.

Done.

https://codereview.chromium.org/2882973002/diff/60001/src/interpreter/interpr...
File src/interpreter/interpreter-generator.cc (right):

https://codereview.chromium.org/2882973002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-generator.cc:3152: Node* slot =
BytecodeOperandImmSmi(0);
On 2017/05/18 14:17:59, rmcilroy wrote:
> nit - slot -> coverage_array_slot (to avoid confusion with feedback_vector
> slot).

Done.

https://codereview.chromium.org/2882973002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2882973002/diff/60001/src/objects.h#newcode1580
src/objects.h:1580: static inline int ToInt(Object* o);
On 2017/05/18 14:17:59, rmcilroy wrote:
> Is this worth adding rather than just Smi::cast(o)->value() ?

Smi::cast(o)->value()
Smi::ToInt(o)

I think the latter is way more readable (especially if the argument is not just
'o'), and I was planning on updating our entire code-base at some point in a
clean-up CL. Also a Smi::TryToInt() for the other very common pattern.

It's orthogonal to this CL though and I don't mind splitting it out.

https://codereview.chromium.org/2882973002/diff/60001/src/objects.h#newcode6279
src/objects.h:6279: static const int kCoverageInfoOffset = kDebugInfoOffset +
kPointerSize;
On 2017/05/18 14:17:59, rmcilroy wrote:
> I don't think we want to add this as a field to SFI, that will be a reasonable
> memory overhead even when block coverage is turned off. Can it be added to
> DebugInfo or somewhere else which is allocated on-demand?

Ack. I discussed this with Benedikt a while ago, and he was also in favor of
moving to DebugInfo. There's still the issue that the existence of DebugInfo
currently implies the existence of breakpoints, so this assumption will need to
change.

https://codereview.chromium.org/2882973002/diff/60001/src/objects.h#newcode10066
src/objects.h:10066: class CoverageInfo : public FixedArray {
On 2017/05/18 11:30:17, marja wrote:
> Please put this in a different file than objects.h. Somewhere in src/objects/
.
> Maybe you want to combine with other related classes.

Ack, thought so myself after uploading this CL. I'll move to a separate file in
objects/.

https://codereview.chromium.org/2882973002/diff/60001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/2882973002/diff/60001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:1979: CONVERT_ARG_HANDLE_CHECKED(FeedbackVector,
vector, 0);
On 2017/05/18 14:17:59, rmcilroy wrote:
> Unused?

The vector is needed to get to the SFI below.

https://codereview.chromium.org/2882973002/diff/60001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:1980: CONVERT_SMI_ARG_CHECKED(slot_index, 1);
On 2017/05/18 14:17:59, rmcilroy wrote:
> coverage_array_slot

Done.

[rmcilroy, 2017-05-22 11:08:46]

https://codereview.chromium.org/2882973002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2882973002/diff/60001/src/objects.h#newcode1580
src/objects.h:1580: static inline int ToInt(Object* o);
On 2017/05/22 09:43:32, jgruber wrote:
> On 2017/05/18 14:17:59, rmcilroy wrote:
> > Is this worth adding rather than just Smi::cast(o)->value() ?
> 
> Smi::cast(o)->value()
> Smi::ToInt(o)
> 
> I think the latter is way more readable (especially if the argument is not
just
> 'o'), and I was planning on updating our entire code-base at some point in a
> clean-up CL. Also a Smi::TryToInt() for the other very common pattern.
> 
> It's orthogonal to this CL though and I don't mind splitting it out.

Acknowledged.

https://codereview.chromium.org/2882973002/diff/60001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/2882973002/diff/60001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:1979: CONVERT_ARG_HANDLE_CHECKED(FeedbackVector,
vector, 0);
On 2017/05/22 09:43:32, jgruber wrote:
> On 2017/05/18 14:17:59, rmcilroy wrote:
> > Unused?
> 
> The vector is needed to get to the SFI below.

Sorry missed this. It's more normal to get the SFI from the JSFunction. Could
you do this instead (you can get it in interpreter-generator with
LoadRegister(Register::function_closure())

[jgruber, 2017-05-22 11:10:08]

https://codereview.chromium.org/2882973002/diff/60001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/2882973002/diff/60001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:1979: CONVERT_ARG_HANDLE_CHECKED(FeedbackVector,
vector, 0);
On 2017/05/22 11:08:46, rmcilroy wrote:
> On 2017/05/22 09:43:32, jgruber wrote:
> > On 2017/05/18 14:17:59, rmcilroy wrote:
> > > Unused?
> > 
> > The vector is needed to get to the SFI below.
> 
> Sorry missed this. It's more normal to get the SFI from the JSFunction. Could
> you do this instead (you can get it in interpreter-generator with
> LoadRegister(Register::function_closure())

Sure, will do.

[marja, 2017-05-22 12:55:37]

https://codereview.chromium.org/2882973002/diff/60001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2882973002/diff/60001/src/ast/ast.h#newcode187
src/ast/ast.h:187: int32_t from, to;
On 2017/05/22 09:43:32, jgruber wrote:
> On 2017/05/18 11:30:17, marja wrote:
> > In Scope, FunctionLiteral etc. there's already start_position and
> end_position,
> > I think it makes sense to have consistent naming there. I.e., these should
be
> > start_position and end_position instead of from and to.
> 
> No very strong feelings on this from my side, but if possible I'd prefer
keeping
> the names short. What about {start, end}?
> 
> Maybe we could also change Scope, FunctionLiteral etc. to contain a
SourceRange
> at some point.

start + end sgtm

[jgruber, 2017-05-31 08:45:43]

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

[commit-bot: I haz the power, 2017-05-31 08:45:51]

Dry run: CQ is trying da patch.

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

[jgruber, 2017-05-31 09:02:15]

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

[commit-bot: I haz the power, 2017-05-31 09:02:23]

Dry run: 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-05-31 09:33:33]

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

[commit-bot: I haz the power, 2017-05-31 09:33:34]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-05-31 11:04:01]

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

[commit-bot: I haz the power, 2017-05-31 11:04:12]

Dry run: CQ is trying da patch.

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

[jgruber, 2017-05-31 11:15:11]

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

[commit-bot: I haz the power, 2017-05-31 11:15:16]

Dry run: 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-05-31 11:47:46]

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

[commit-bot: I haz the power, 2017-05-31 11:47:46]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-05-31 16:22:04]

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

[jgruber, 2017-05-31 16:22:05]

Description was changed from

==========
[coverage] Block coverage prototype

Initial prototype that implements block coverage for if statements.

Usage:

out/debug/d8 --block-coverage --lcov=$(echo ~/simple-if.lcov) ~/simple-if.js
genhtml ~/simple-if.lcov -o ~/simple-if
google-chrome ~/simple-if/index.html

BUG=v8:6000
==========

to

==========
[coverage] Block coverage prototype

Initial prototype that implements block coverage for if statements.

Usage:

out/debug/d8 --block-coverage --lcov=$(echo ~/simple-if.lcov) ~/simple-if.js
genhtml ~/simple-if.lcov -o ~/simple-if
google-chrome ~/simple-if/index.html

BUG=v8:6000
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng
==========

[commit-bot: I haz the power, 2017-05-31 16:22:13]

Dry run: 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-05-31 16:40:17]

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

[commit-bot: I haz the power, 2017-05-31 16:40:18]

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

[jgruber, 2017-06-01 09:13:40]

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

[commit-bot: I haz the power, 2017-06-01 09:13:48]

Dry run: CQ is trying da patch.

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

[jgruber, 2017-06-01 09:46:10]

Description was changed from

==========
[coverage] Block coverage prototype

Initial prototype that implements block coverage for if statements.

Usage:

out/debug/d8 --block-coverage --lcov=$(echo ~/simple-if.lcov) ~/simple-if.js
genhtml ~/simple-if.lcov -o ~/simple-if
google-chrome ~/simple-if/index.html

BUG=v8:6000
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng
==========

to

==========
[coverage] Block coverage with support for IfStatements

This CL implements general infrastructure for block coverage together with
initial support for if-statements.

Coverage output can be generated in lcov format by d8 as follows: 

$ d8 --block-coverage --lcov=$(echo ~/simple-if.lcov) ~/simple-if.js
$ genhtml ~/simple-if.lcov -o ~/simple-if
$ chrome ~/simple-if/index.html

A high level overview of the implementation follows:

The parser now collects source ranges unconditionally for relevant AST nodes.
Memory overhead is very low and this seemed like the cleanest and simplest
alternative.

Bytecode generation uses these ranges to allocate coverage slots and insert
IncBlockCounter instructions (e.g. at the beginning of then- and else blocks
for if-statements). The slot-range mapping is generated here and passed on
through CompilationInfo, and is later accessible through the
SharedFunctionInfo.

The IncBlockCounter bytecode fetches the slot-range mapping (called
CoverageInfo) from the shared function info and simply increments the counter.
We don't collect native-context-specific counts as they are irrelevant to our
use-cases.

Coverage information is finally generated on-demand through Coverage::Collect.
The only current consumer is a d8 front-end with lcov-style output, but the
short-term goal is to expose this through the inspector protocol.

BUG=v8:6000
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng
==========

[commit-bot: I haz the power, 2017-06-01 09:54:08]

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

[commit-bot: I haz the power, 2017-06-01 09:54:09]

Dry run: Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/26721)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[jgruber, 2017-06-01 11:31:13]

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

[commit-bot: I haz the power, 2017-06-01 11:31:21]

Dry run: CQ is trying da patch.

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

[jgruber, 2017-06-01 11:56:26]

jgruber@chromium.org changed reviewers:
+ mstarzinger@chromium.org, yangguo@chromium.org

[jgruber, 2017-06-01 11:56:27]

Now that DebugInfo changes have landed, I think this is ready for review.

There's a high-level summary of changes in the commit message.

A brief reviewers guide:

src/ast
src/parsing: gather and store source ranges

src/interpreter/bytecode-generator: generation of coverage slots &
IncBlockCounter bytecodes

src/compilation-info
src/compiler
src/interpreter
src/runtime: IncBlockCounter bytecode impl and general shepherding of data
through the compilation pipeline

src/objects/: CoverageInfo implementation - this is stored on SFI::DebugInfo and
contains both the slot-source range mapping and the actual execution counters.

src/api
src/d8
src/debug/: data structures & generation of coverage infos

Please take a look!

https://codereview.chromium.org/2882973002/diff/160001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2882973002/diff/160001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:2236:
environment()->RecordAfterState(node, Environment::kAttachFrameState);
Ross: Is RecordAfterState needed here?

[commit-bot: I haz the power, 2017-06-01 12:31:41]

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

[commit-bot: I haz the power, 2017-06-01 12:31:42]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2017-06-01 14:01:50]

Thanks for moving to DebugInfo. A couple more comments.

https://codereview.chromium.org/2882973002/diff/200001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2882973002/diff/200001/src/compiler.cc#newcod...
src/compiler.cc:466: }
nit - maybe do this in InstallUnoptimizedCode similar to how
InstallSharedScopeInfo is done ?

https://codereview.chromium.org/2882973002/diff/200001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/2882973002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-generator.h:253: }
I still have this comment here which was unanswered on the previous patchset:

How about factoring this out into a small helper class (similar to
GlobalDeclarationBuilder) and just create the builder in the BytecodeGenerator's
constructor if:
  FLAG_block_coverage && info()->parse_info()->script()->IsUserJavaScript()

then just check for:
  if (block_coverage_builder())
block_coverage_builder()->IncrementBlock(SourceRange range);

in the if / else visitor, then you don't need a is_block_coverage() function,
and we don't have the overhead of an empty ZoneVector when block coverage is
turned off. WDYT?

https://codereview.chromium.org/2882973002/diff/200001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/2882973002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecodes.h:342: V(IncBlockCounter, AccumulatorUse::kNone,
OperandType::kIdx)                 \
Any reason we couldn't just emit the runtime call rather than being a bytecode
given that Ignition and TurboFan just do a runtime call here anyway?

https://codereview.chromium.org/2882973002/diff/200001/test/unittests/interpr...
File test/unittests/interpreter/bytecode-array-builder-unittest.cc (right):

https://codereview.chromium.org/2882973002/diff/200001/test/unittests/interpr...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:375:
FLAG_block_coverage = true;  // Satisfy DCHECKs.
This will unconditionally set the flag for all following unittests if they are
run in the same process. Please use the SaveFlags to save and restore the flags
at the start and end of this test.

[jgruber, 2017-06-02 06:34:45]

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

[commit-bot: I haz the power, 2017-06-02 06:34:52]

Dry run: CQ is trying da patch.

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

[jgruber, 2017-06-02 06:39:27]

Addressed comments & removed Smi::ToInt from this CL to reduce noise.

https://codereview.chromium.org/2882973002/diff/200001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2882973002/diff/200001/src/compiler.cc#newcod...
src/compiler.cc:466: }
On 2017/06/01 14:01:50, rmcilroy wrote:
> nit - maybe do this in InstallUnoptimizedCode similar to how
> InstallSharedScopeInfo is done ?

Done.

https://codereview.chromium.org/2882973002/diff/200001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/2882973002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-generator.h:253: }
On 2017/06/01 14:01:50, rmcilroy wrote:
> I still have this comment here which was unanswered on the previous patchset:
> 
> How about factoring this out into a small helper class (similar to
> GlobalDeclarationBuilder) and just create the builder in the
BytecodeGenerator's
> constructor if:
>   FLAG_block_coverage && info()->parse_info()->script()->IsUserJavaScript()
> 
> then just check for:
>   if (block_coverage_builder())
> block_coverage_builder()->IncrementBlock(SourceRange range);
> 
> in the if / else visitor, then you don't need a is_block_coverage() function,
> and we don't have the overhead of an empty ZoneVector when block coverage is
> turned off. WDYT?

Sorry I missed that one. I'm happy to factor out into a helper class, but I'd
like to avoid the

if (block_coverage_builder())
  block_coverage_builder()->IncrementBlock(...);

pattern. How do you like the solution the current PS (helper class + 2 helper
methods on BytecodeGenerator)?

https://codereview.chromium.org/2882973002/diff/200001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/2882973002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecodes.h:342: V(IncBlockCounter, AccumulatorUse::kNone,
OperandType::kIdx)                 \
On 2017/06/01 14:01:50, rmcilroy wrote:
> Any reason we couldn't just emit the runtime call rather than being a bytecode
> given that Ignition and TurboFan just do a runtime call here anyway?

The plan is to remove the runtime call soon with a more efficient implementation
that just loads the CoverageInfo and increments the correct slot. I just haven't
gotten around to it yet.

If you agree, I'd like to land the bytecode in this CL to make the upcoming
transition from the runtime call easier.

https://codereview.chromium.org/2882973002/diff/200001/test/unittests/interpr...
File test/unittests/interpreter/bytecode-array-builder-unittest.cc (right):

https://codereview.chromium.org/2882973002/diff/200001/test/unittests/interpr...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:375:
FLAG_block_coverage = true;  // Satisfy DCHECKs.
On 2017/06/01 14:01:50, rmcilroy wrote:
> This will unconditionally set the flag for all following unittests if they are
> run in the same process. Please use the SaveFlags to save and restore the
flags
> at the start and end of this test.

Done.

[jgruber, 2017-06-02 06:41:11]

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

[commit-bot: I haz the power, 2017-06-02 06:41:20]

Dry run: CQ is trying da patch.

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

[marja, 2017-06-02 07:01:23]

parsing & ast still lgtm

https://codereview.chromium.org/2882973002/diff/240001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2882973002/diff/240001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:822: 
Since you're touching this, can you add a DCHECK here that we're in the main
thread?

https://codereview.chromium.org/2882973002/diff/240001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2882973002/diff/240001/src/isolate.h#newcode1020
src/isolate.h:1020: bool is_block_count_code_coverage() const {
This function name is a bit odd... is code coverage what?
 Maybe block_count_code_coverage_enabled would be clearer?

https://codereview.chromium.org/2882973002/diff/240001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2882973002/diff/240001/src/objects-inl.h#newc...
src/objects-inl.h:78: TYPE_CHECKER(CoverageInfo, FIXED_ARRAY_TYPE)
Why is this here and not in debug-objects-inl.h?

[jgruber, 2017-06-02 07:18:20]

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

[commit-bot: I haz the power, 2017-06-02 07:18:23]

Dry run: CQ is trying da patch.

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

[jgruber, 2017-06-02 07:18:32]

https://codereview.chromium.org/2882973002/diff/240001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2882973002/diff/240001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:822: 
On 2017/06/02 07:01:23, marja wrote:
> Since you're touching this, can you add a DCHECK here that we're in the main
> thread?

Done.

https://codereview.chromium.org/2882973002/diff/240001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2882973002/diff/240001/src/isolate.h#newcode1020
src/isolate.h:1020: bool is_block_count_code_coverage() const {
On 2017/06/02 07:01:23, marja wrote:
> This function name is a bit odd... is code coverage what?
>  Maybe block_count_code_coverage_enabled would be clearer? 

I agree it's a bit odd, but the naming is consistent with the other coverage
mode accessors above (such as 'is_precise_count_code_coverage').

https://codereview.chromium.org/2882973002/diff/240001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2882973002/diff/240001/src/objects-inl.h#newc...
src/objects-inl.h:78: TYPE_CHECKER(CoverageInfo, FIXED_ARRAY_TYPE)
On 2017/06/02 07:01:23, marja wrote:
> Why is this here and not in debug-objects-inl.h?

TYPE_CHECKER generates methods on HeapObject, and objects.h has dependencies on
those methods; e.g. through

HEAP_OBJECT_TYPE_LIST(IS_TYPE_FUNCTION_DECL)

[commit-bot: I haz the power, 2017-06-02 08:01:43]

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

[commit-bot: I haz the power, 2017-06-02 08:01:44]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-06-02 08:56:22]

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

[commit-bot: I haz the power, 2017-06-02 08:56:30]

Dry run: CQ is trying da patch.

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

[rmcilroy, 2017-06-02 09:36:00]

A couple more comments, but LGTM once they are addressed, thanks.

https://codereview.chromium.org/2882973002/diff/200001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/2882973002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecodes.h:342: V(IncBlockCounter, AccumulatorUse::kNone,
OperandType::kIdx)                 \
On 2017/06/02 06:39:27, jgruber wrote:
> On 2017/06/01 14:01:50, rmcilroy wrote:
> > Any reason we couldn't just emit the runtime call rather than being a
bytecode
> > given that Ignition and TurboFan just do a runtime call here anyway?
> 
> The plan is to remove the runtime call soon with a more efficient
implementation
> that just loads the CoverageInfo and increments the correct slot. I just
haven't
> gotten around to it yet.
> 
> If you agree, I'd like to land the bytecode in this CL to make the upcoming
> transition from the runtime call easier.

Sounds good.

https://codereview.chromium.org/2882973002/diff/280001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2882973002/diff/280001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:2237:
environment()->RecordAfterState(node, Environment::kAttachFrameState);
I don't think you need this (since the runtime call doesn't call into JS and so
can't cause a lazy deopt).

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:756: void IncBlockCounter(int
coverage_array_slot) {
nit - IncrementBlockCounter

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1233: int else_slot =
AllocateBlockCoverageSlot(stmt->else_range());
Any reason you need two seperate functions here? Could you just do:

IncrementBlockCounterIfCoverageEnabled(stmt->then_range()) 

and do the allocation in BlockCoverageBuilder::IncBlockCounter?

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:3960: void
BytecodeGenerator::IncBlockCounter(int coverage_array_slot) {
nit - BuildIncrementBlockCoverageCounterIfEnabled. Also please move above
language_mode / feedback_index getters.

(we prepend function names with Build or Visit if they emit bytecode)

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.h:241: void IncBlockCounter(int
coverage_array_slot);
nit - move above "getters" (maybe below BuildLoadPropertyKey)

[commit-bot: I haz the power, 2017-06-02 09:38:54]

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

[commit-bot: I haz the power, 2017-06-02 09:38:55]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-06-02 11:37:41]

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

[commit-bot: I haz the power, 2017-06-02 11:37:50]

Dry run: CQ is trying da patch.

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

[jgruber, 2017-06-02 11:38:49]

https://codereview.chromium.org/2882973002/diff/280001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2882973002/diff/280001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:2237:
environment()->RecordAfterState(node, Environment::kAttachFrameState);
On 2017/06/02 09:35:59, rmcilroy wrote:
> I don't think you need this (since the runtime call doesn't call into JS and
so
> can't cause a lazy deopt).

Thanks, I thought so (asked this in an comment that must've gotten lost in an
older patch set).

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:756: void IncBlockCounter(int
coverage_array_slot) {
On 2017/06/02 09:35:59, rmcilroy wrote:
> nit - IncrementBlockCounter

Done. I assume you meant only this function and not the bytecode name itself.

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1233: int else_slot =
AllocateBlockCoverageSlot(stmt->else_range());
On 2017/06/02 09:35:59, rmcilroy wrote:
> Any reason you need two seperate functions here? Could you just do:
> 
> IncrementBlockCounterIfCoverageEnabled(stmt->then_range()) 
> 
> and do the allocation in BlockCoverageBuilder::IncBlockCounter?

I originally  had it like that, but this turned out to be cleaner due to the
optimizations we do around constant false/true conditions. In those cases, we
still need to allocate the counter (to track that the block is not executed),
but we don't generate an IncBlockCounter bytecode.

The alternative would be to sprinkle explicit AllocateBlockCoverageSlot calls in
code paths that skip then/else block bytecode generation.

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:3960: void
BytecodeGenerator::IncBlockCounter(int coverage_array_slot) {
On 2017/06/02 09:35:59, rmcilroy wrote:
> nit - BuildIncrementBlockCoverageCounterIfEnabled. Also please move above
> language_mode / feedback_index getters.
> 
> (we prepend function names with Build or Visit if they emit bytecode)

Done. Also renamed AllocateBlockCoverageSlotIfEnabled.

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.h:241: void IncBlockCounter(int
coverage_array_slot);
On 2017/06/02 09:35:59, rmcilroy wrote:
> nit - move above "getters" (maybe below BuildLoadPropertyKey)

Done.

[commit-bot: I haz the power, 2017-06-02 12:18:37]

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

[commit-bot: I haz the power, 2017-06-02 12:18:38]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2017-06-02 12:52:50]

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1233: int else_slot =
AllocateBlockCoverageSlot(stmt->else_range());
On 2017/06/02 11:38:49, jgruber wrote:
> On 2017/06/02 09:35:59, rmcilroy wrote:
> > Any reason you need two seperate functions here? Could you just do:
> > 
> > IncrementBlockCounterIfCoverageEnabled(stmt->then_range()) 
> > 
> > and do the allocation in BlockCoverageBuilder::IncBlockCounter?
> 
> I originally  had it like that, but this turned out to be cleaner due to the
> optimizations we do around constant false/true conditions. In those cases, we
> still need to allocate the counter (to track that the block is not executed),
> but we don't generate an IncBlockCounter bytecode.
> 
> The alternative would be to sprinkle explicit AllocateBlockCoverageSlot calls
in
> code paths that skip then/else block bytecode generation.

Ahh I see. Can we not just infer that a block has never been executed if we
never allocated a coverage slot for the range?

[jgruber, 2017-06-02 13:01:54]

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2882973002/diff/280001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1233: int else_slot =
AllocateBlockCoverageSlot(stmt->else_range());
On 2017/06/02 12:52:49, rmcilroy wrote:
> On 2017/06/02 11:38:49, jgruber wrote:
> > On 2017/06/02 09:35:59, rmcilroy wrote:
> > > Any reason you need two seperate functions here? Could you just do:
> > > 
> > > IncrementBlockCounterIfCoverageEnabled(stmt->then_range()) 
> > > 
> > > and do the allocation in BlockCoverageBuilder::IncBlockCounter?
> > 
> > I originally  had it like that, but this turned out to be cleaner due to the
> > optimizations we do around constant false/true conditions. In those cases,
we
> > still need to allocate the counter (to track that the block is not
executed),
> > but we don't generate an IncBlockCounter bytecode.
> > 
> > The alternative would be to sprinkle explicit AllocateBlockCoverageSlot
calls
> in
> > code paths that skip then/else block bytecode generation.
> 
> Ahh I see. Can we not just infer that a block has never been executed if we
> never allocated a coverage slot for the range?

No, if the slot is not allocated then we do not know that a relevant source
range exists at this point.

A coverage slot contains both the execution count and the relevant source range
(and we don't store these ranges anywhere else).

[rmcilroy, 2017-06-02 14:27:54]

LGTM

[Michael Starzinger, 2017-06-06 11:49:33]

LGTM if comments are addressed.

https://codereview.chromium.org/2882973002/diff/300001/src/debug/debug-interf...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2882973002/diff/300001/src/debug/debug-interf...
src/debug/debug-interface.h:251: // lower granularity. See
go/v8-designdoc-block-code-coverage.
The "go" short-links are not available to people outside of Google. Is this
document public? If so could we turn this into a link usable to everyone?
Otherwise I'd just drop the link completely.

https://codereview.chromium.org/2882973002/diff/300001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2882973002/diff/300001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:394: //
(go/v8-designdoc-block-code-coverage).
nit: Likewise to my previous comment, no "go" links please.

https://codereview.chromium.org/2882973002/diff/300001/src/objects/debug-obje...
File src/objects/debug-objects.h (right):

https://codereview.chromium.org/2882973002/diff/300001/src/objects/debug-obje...
src/objects/debug-objects.h:147: // More information at
go/v8-designdoc-block-code-coverage.
nit: Likewise, no "go" links.

https://codereview.chromium.org/2882973002/diff/300001/src/runtime/runtime-de...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/2882973002/diff/300001/src/runtime/runtime-de...
src/runtime/runtime-debug.cc:2010: HandleScope scope(isolate);
nit: Seems like this function is not using handles. Can we use SealHandleScope
instead to ensure it? Especially since this function is also makred as not
requiring a FrameState it would be super useful to ensure it stays
un-handlified.

https://codereview.chromium.org/2882973002/diff/300001/test/mjsunit/code-cove...
File test/mjsunit/code-coverage-block.js (right):

https://codereview.chromium.org/2882973002/diff/300001/test/mjsunit/code-cove...
test/mjsunit/code-coverage-block.js:5: // Flags: --allow-natives-syntax
--no-always-opt --block-coverage
question: Why --no-always-opt here? Do the expected counts start to mismatch in
presence of deopt? Because that would be something worth keeping track of or
commenting on.

https://codereview.chromium.org/2882973002/diff/300001/test/unittests/interpr...
File test/unittests/interpreter/bytecode-array-builder-unittest.cc (right):

https://codereview.chromium.org/2882973002/diff/300001/test/unittests/interpr...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:377:
FLAG_block_coverage = true;  // Satisfy DCHECKs.
suggestion: We could also just remove the DCHECKs in the BytecodeArrayBuilder. I
think those we could live without.

[rmcilroy, 2017-06-06 13:02:32]

https://codereview.chromium.org/2882973002/diff/300001/test/unittests/interpr...
File test/unittests/interpreter/bytecode-array-builder-unittest.cc (right):

https://codereview.chromium.org/2882973002/diff/300001/test/unittests/interpr...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:377:
FLAG_block_coverage = true;  // Satisfy DCHECKs.
On 2017/06/06 11:49:32, Michael Starzinger wrote:
> suggestion: We could also just remove the DCHECKs in the BytecodeArrayBuilder.
I
> think those we could live without.

+1

[jgruber, 2017-06-06 14:52:46]

https://codereview.chromium.org/2882973002/diff/300001/src/debug/debug-interf...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2882973002/diff/300001/src/debug/debug-interf...
src/debug/debug-interface.h:251: // lower granularity. See
go/v8-designdoc-block-code-coverage.
On 2017/06/06 11:49:32, Michael Starzinger wrote:
> The "go" short-links are not available to people outside of Google. Is this
> document public? If so could we turn this into a link usable to everyone?
> Otherwise I'd just drop the link completely.

Done, changed to goo.gl shortened link and dropped other occurrences.

https://codereview.chromium.org/2882973002/diff/300001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2882973002/diff/300001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:394: //
(go/v8-designdoc-block-code-coverage).
On 2017/06/06 11:49:32, Michael Starzinger wrote:
> nit: Likewise to my previous comment, no "go" links please.

Done.

https://codereview.chromium.org/2882973002/diff/300001/src/objects/debug-obje...
File src/objects/debug-objects.h (right):

https://codereview.chromium.org/2882973002/diff/300001/src/objects/debug-obje...
src/objects/debug-objects.h:147: // More information at
go/v8-designdoc-block-code-coverage.
On 2017/06/06 11:49:32, Michael Starzinger wrote:
> nit: Likewise, no "go" links.

Done.

https://codereview.chromium.org/2882973002/diff/300001/src/runtime/runtime-de...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/2882973002/diff/300001/src/runtime/runtime-de...
src/runtime/runtime-debug.cc:2010: HandleScope scope(isolate);
On 2017/06/06 11:49:32, Michael Starzinger wrote:
> nit: Seems like this function is not using handles. Can we use SealHandleScope
> instead to ensure it? Especially since this function is also makred as not
> requiring a FrameState it would be super useful to ensure it stays
> un-handlified.

Done.

https://codereview.chromium.org/2882973002/diff/300001/test/mjsunit/code-cove...
File test/mjsunit/code-coverage-block.js (right):

https://codereview.chromium.org/2882973002/diff/300001/test/mjsunit/code-cove...
test/mjsunit/code-coverage-block.js:5: // Flags: --allow-natives-syntax
--no-always-opt --block-coverage
On 2017/06/06 11:49:32, Michael Starzinger wrote:
> question: Why --no-always-opt here? Do the expected counts start to mismatch
in
> presence of deopt? Because that would be something worth keeping track of or
> commenting on.

--no-always-opt is inherited from the ad-hoc/precise coverage mode tests
(code-coverage-{ad-hoc,precise}). Passing --always-opt makes counts for the
top-level function disappear (see below).

I'm not sure why that happens.. Maybe there's some sort of special handling for
FeedbackVectors of top-level functions?

https://codereview.chromium.org/2882973002/diff/300001/test/mjsunit/code-cove...
test/mjsunit/code-coverage-block.js:33:
[{"start":0,"end":20,"count":1},{"start":1,"end":16,"count":1}]
With --always-opt, the result is [{"start":1,"end":16,"count":1}] instead.

https://codereview.chromium.org/2882973002/diff/300001/test/unittests/interpr...
File test/unittests/interpreter/bytecode-array-builder-unittest.cc (right):

https://codereview.chromium.org/2882973002/diff/300001/test/unittests/interpr...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:377:
FLAG_block_coverage = true;  // Satisfy DCHECKs.
On 2017/06/06 11:49:32, Michael Starzinger wrote:
> suggestion: We could also just remove the DCHECKs in the BytecodeArrayBuilder.
I
> think those we could live without.

Done.

[jgruber, 2017-06-06 14:54:04]

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

[commit-bot: I haz the power, 2017-06-06 14:54:07]

Dry run: CQ is trying da patch.

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

[jgruber, 2017-06-06 15:06:33]

https://codereview.chromium.org/2882973002/diff/300001/test/mjsunit/code-cove...
File test/mjsunit/code-coverage-block.js (right):

https://codereview.chromium.org/2882973002/diff/300001/test/mjsunit/code-cove...
test/mjsunit/code-coverage-block.js:5: // Flags: --allow-natives-syntax
--no-always-opt --block-coverage
On 2017/06/06 14:52:46, jgruber wrote:
> On 2017/06/06 11:49:32, Michael Starzinger wrote:
> > question: Why --no-always-opt here? Do the expected counts start to mismatch
> in
> > presence of deopt? Because that would be something worth keeping track of or
> > commenting on.
> 
> --no-always-opt is inherited from the ad-hoc/precise coverage mode tests
> (code-coverage-{ad-hoc,precise}). Passing --always-opt makes counts for the
> top-level function disappear (see below).
> 
> I'm not sure why that happens.. Maybe there's some sort of special handling
for
> FeedbackVectors of top-level functions?

Brief investigation: The top-level FeedbackVector exists, but its invocation
count is 0. As discussed offline, I'll land this as-is for now. Opened
http://crbug.com/v8/6466 for investigation.

[jgruber, 2017-06-06 15:07:59]

The CQ bit was unchecked by jgruber@chromium.org

[jgruber, 2017-06-06 15:08:08]

The CQ bit was checked by jgruber@chromium.org

[jgruber, 2017-06-06 15:08:08]

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

[commit-bot: I haz the power, 2017-06-06 15:08:12]

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-06 15:44:56]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1496761688504830,
"parent_rev": "0532e964980b3cc72e2e4c2ba066125e2fe6d217", "commit_rev":
"b42415402f0d251ab879bb168b386bcd71fdc35d"}

[commit-bot: I haz the power, 2017-06-06 15:45:05]

Description was changed from

==========
[coverage] Block coverage with support for IfStatements

This CL implements general infrastructure for block coverage together with
initial support for if-statements.

Coverage output can be generated in lcov format by d8 as follows: 

$ d8 --block-coverage --lcov=$(echo ~/simple-if.lcov) ~/simple-if.js
$ genhtml ~/simple-if.lcov -o ~/simple-if
$ chrome ~/simple-if/index.html

A high level overview of the implementation follows:

The parser now collects source ranges unconditionally for relevant AST nodes.
Memory overhead is very low and this seemed like the cleanest and simplest
alternative.

Bytecode generation uses these ranges to allocate coverage slots and insert
IncBlockCounter instructions (e.g. at the beginning of then- and else blocks
for if-statements). The slot-range mapping is generated here and passed on
through CompilationInfo, and is later accessible through the
SharedFunctionInfo.

The IncBlockCounter bytecode fetches the slot-range mapping (called
CoverageInfo) from the shared function info and simply increments the counter.
We don't collect native-context-specific counts as they are irrelevant to our
use-cases.

Coverage information is finally generated on-demand through Coverage::Collect.
The only current consumer is a d8 front-end with lcov-style output, but the
short-term goal is to expose this through the inspector protocol.

BUG=v8:6000
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng
==========

to

==========
[coverage] Block coverage with support for IfStatements

This CL implements general infrastructure for block coverage together with
initial support for if-statements.

Coverage output can be generated in lcov format by d8 as follows: 

$ d8 --block-coverage --lcov=$(echo ~/simple-if.lcov) ~/simple-if.js
$ genhtml ~/simple-if.lcov -o ~/simple-if
$ chrome ~/simple-if/index.html

A high level overview of the implementation follows:

The parser now collects source ranges unconditionally for relevant AST nodes.
Memory overhead is very low and this seemed like the cleanest and simplest
alternative.

Bytecode generation uses these ranges to allocate coverage slots and insert
IncBlockCounter instructions (e.g. at the beginning of then- and else blocks
for if-statements). The slot-range mapping is generated here and passed on
through CompilationInfo, and is later accessible through the
SharedFunctionInfo.

The IncBlockCounter bytecode fetches the slot-range mapping (called
CoverageInfo) from the shared function info and simply increments the counter.
We don't collect native-context-specific counts as they are irrelevant to our
use-cases.

Coverage information is finally generated on-demand through Coverage::Collect.
The only current consumer is a d8 front-end with lcov-style output, but the
short-term goal is to expose this through the inspector protocol.

BUG=v8:6000
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng

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

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

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/v8/v8/+/b42415402f0d251ab879bb168b386bcd71f...