[coverage] Block coverage with support for IfStatements

src/interpreter/bytecode-generator.cc

Index: src/interpreter/bytecode-generator.cc
diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc
index b754897e759335c90ea8c4a94555f3d22d3e6a9f..9a49b849d3b38bf56c99c1b354dfe25533c2f722 100644
--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -737,6 +737,36 @@ class BytecodeGenerator::GlobalDeclarationsBuilder final : public ZoneObject {
   bool has_constant_pool_entry_;
 };
 
+// Used to generate IncBlockCounter bytecodes and the {source range, slot}
+// mapping for block coverage.
+class BytecodeGenerator::BlockCoverageBuilder final : public ZoneObject {
+ public:
+  explicit BlockCoverageBuilder(Zone* zone, BytecodeArrayBuilder* builder)
+      : slots_(0, zone), builder_(builder) {}
+
+  static const int kNoCoverageArraySlot = -1;
+
+  int AllocateBlockCoverageSlot(SourceRange range) {
+    if (range.IsEmpty()) return kNoCoverageArraySlot;
+    const int slot = static_cast<int>(slots_.size());
+    slots_.emplace_back(range);
+    return slot;
+  }
+
+  void IncBlockCounter(int coverage_array_slot) {

[rmcilroy, 2017/06/02 09:35:59]

nit - IncrementBlockCounter

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

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

+    if (coverage_array_slot == kNoCoverageArraySlot) return;
+    builder_->IncBlockCounter(coverage_array_slot);
+  }
+
+  const ZoneVector<SourceRange>& slots() const { return slots_; }
+
+ private:
+  // Contains source range information for allocated block coverage counter
+  // slots. Slot i covers range slots_[i].
+  ZoneVector<SourceRange> slots_;
+  BytecodeArrayBuilder* builder_;
+};
+
 class BytecodeGenerator::CurrentScope final {
  public:
   CurrentScope(BytecodeGenerator* generator, Scope* scope)
@@ -767,6 +797,7 @@ BytecodeGenerator::BytecodeGenerator(CompilationInfo* info)
       closure_scope_(info->scope()),
       current_scope_(info->scope()),
       globals_builder_(new (zone()) GlobalDeclarationsBuilder(info->zone())),
+      block_coverage_builder_(nullptr),
       global_declarations_(0, info->zone()),
       function_literals_(0, info->zone()),
       native_function_literals_(0, info->zone()),
@@ -779,10 +810,23 @@ BytecodeGenerator::BytecodeGenerator(CompilationInfo* info)
       generator_state_(),
       loop_depth_(0) {
   DCHECK_EQ(closure_scope(), closure_scope()->GetClosureScope());
+  if (info->is_block_coverage_enabled()) {
+    DCHECK(FLAG_block_coverage);
+    block_coverage_builder_ =
+        new (zone()) BlockCoverageBuilder(zone(), builder());
+  }
 }
 
 Handle<BytecodeArray> BytecodeGenerator::FinalizeBytecode(Isolate* isolate) {
+  DCHECK(ThreadId::Current().Equals(isolate->thread_id()));
+
   AllocateDeferredConstants(isolate);
+
+  if (info()->is_block_coverage_enabled()) {
+    info()->set_coverage_info(
+        isolate->factory()->NewCoverageInfo(block_coverage_builder_->slots()));
+  }
+
   if (HasStackOverflow()) return Handle<BytecodeArray>();
   return builder()->ToBytecodeArray(isolate);
 }
@@ -1184,12 +1228,18 @@ void BytecodeGenerator::VisitEmptyStatement(EmptyStatement* stmt) {
 
 void BytecodeGenerator::VisitIfStatement(IfStatement* stmt) {
   builder()->SetStatementPosition(stmt);
+
+  int then_slot = AllocateBlockCoverageSlot(stmt->then_range());
+  int else_slot = AllocateBlockCoverageSlot(stmt->else_range());

[rmcilroy, 2017/06/02 09:35:59]

Any reason you need two seperate functions here? Could you just do:

IncrementBlockCounterIfCoverageEnabled(stmt->then_range()) 

and do the allocation in BlockCoverageBuilder::IncBlockCounter?

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

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.

[rmcilroy, 2017/06/02 12:52:49]

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]

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).

+
   if (stmt->condition()->ToBooleanIsTrue()) {
     // Generate then block unconditionally as always true.
+    IncBlockCounter(then_slot);
     Visit(stmt->then_statement());
   } else if (stmt->condition()->ToBooleanIsFalse()) {
     // Generate else block unconditionally if it exists.
     if (stmt->HasElseStatement()) {
+      IncBlockCounter(else_slot);
       Visit(stmt->else_statement());
     }
   } else {
@@ -1202,11 +1252,13 @@ void BytecodeGenerator::VisitIfStatement(IfStatement* stmt) {
                  TestFallthrough::kThen);
 
     then_labels.Bind(builder());
+    IncBlockCounter(then_slot);
     Visit(stmt->then_statement());
 
     if (stmt->HasElseStatement()) {
       builder()->Jump(&end_label);
       else_labels.Bind(builder());
+      IncBlockCounter(else_slot);
       Visit(stmt->else_statement());
     } else {
       else_labels.Bind(builder());
@@ -3899,6 +3951,18 @@ int BytecodeGenerator::feedback_index(FeedbackSlot slot) const {
   return FeedbackVector::GetIndex(slot);
 }
 
+int BytecodeGenerator::AllocateBlockCoverageSlot(SourceRange range) {
+  return (block_coverage_builder_ == nullptr)
+             ? BlockCoverageBuilder::kNoCoverageArraySlot
+             : block_coverage_builder_->AllocateBlockCoverageSlot(range);
+}
+
+void BytecodeGenerator::IncBlockCounter(int coverage_array_slot) {

[rmcilroy, 2017/06/02 09:35:59]

nit - BuildIncrementBlockCoverageCounterIfEnabled. Also please move above
language_mode / feedback_index getters.

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

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

Done. Also renamed AllocateBlockCoverageSlotIfEnabled.

+  if (block_coverage_builder_ != nullptr) {
+    block_coverage_builder_->IncBlockCounter(coverage_array_slot);
+  }
+}
+
 Runtime::FunctionId BytecodeGenerator::StoreToSuperRuntimeId() {
   return is_strict(language_mode()) ? Runtime::kStoreToSuper_Strict
                                     : Runtime::kStoreToSuper_Sloppy;