[coverage] Block coverage with support for IfStatements

src/interpreter/bytecode-generator.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/interpreter/bytecode-generator.h"
 
 #include "src/ast/compile-time-value.h"
 #include "src/ast/scopes.h"
 #include "src/builtins/builtins-constructor.h"
 #include "src/code-stubs.h"
@@ skipping 719 matching lines @@
     const AstRawString* name;
     FeedbackSlot slot;
     FeedbackSlot literal_slot;
     FunctionLiteral* func;
   };
   ZoneVector<Declaration> declarations_;
   size_t constant_pool_entry_;
   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)
       : generator_(generator), outer_scope_(generator->current_scope()) {
     if (scope != nullptr) {
       generator_->set_current_scope(scope);
     }
   }
   ~CurrentScope() {
     if (outer_scope_ != generator_->current_scope()) {
@@ skipping 10 matching lines @@
     : zone_(info->zone()),
       builder_(new (zone()) BytecodeArrayBuilder(
           info->isolate(), info->zone(), info->num_parameters_including_this(),
           info->scope()->num_stack_slots(), info->literal(),
           info->SourcePositionRecordingMode())),
       info_(info),
       ast_string_constants_(info->isolate()->ast_string_constants()),
       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()),
       object_literals_(0, info->zone()),
       array_literals_(0, info->zone()),
       execution_control_(nullptr),
       execution_context_(nullptr),
       execution_result_(nullptr),
       generator_jump_table_(nullptr),
       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);
 }
 
 void BytecodeGenerator::AllocateDeferredConstants(Isolate* isolate) {
   // Build global declaration pair arrays.
   for (GlobalDeclarationsBuilder* globals_builder : global_declarations_) {
     Handle<FixedArray> declarations =
         globals_builder->AllocateDeclarations(info());
     if (declarations.is_null()) return SetStackOverflow();
@@ skipping 381 matching lines @@
 void BytecodeGenerator::VisitExpressionStatement(ExpressionStatement* stmt) {
   builder()->SetStatementPosition(stmt);
   VisitForEffect(stmt->expression());
 }
 
 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 {
     // TODO(oth): If then statement is BreakStatement or
     // ContinueStatement we can reduce number of generated
     // jump/jump_ifs here. See BasicLoops test.
     BytecodeLabel end_label;
     BytecodeLabels then_labels(zone()), else_labels(zone());
     VisitForTest(stmt->condition(), &then_labels, &else_labels,
                  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());
     }
     builder()->Bind(&end_label);
   }
 }
 
 void BytecodeGenerator::VisitSloppyBlockFunctionStatement(
     SloppyBlockFunctionStatement* stmt) {
@@ skipping 2672 matching lines @@
 }
 
 LanguageMode BytecodeGenerator::language_mode() const {
   return current_scope()->language_mode();
 }
 
 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;
 }
 
 Runtime::FunctionId BytecodeGenerator::StoreKeyedToSuperRuntimeId() {
   return is_strict(language_mode()) ? Runtime::kStoreKeyedToSuper_Strict
                                     : Runtime::kStoreKeyedToSuper_Sloppy;
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8