[coverage] Unify slot handling in BreakableControlFlowBuilders

Move block coverage slot creation into BreakableControlFlowBuilder for
Switch/Loop/Block constructs.

Bug: v8:6000
Change-Id: I4fa7fdb2ffbb56fd1016c22741458c103b42219c
Reviewed-on: https://chromium-review.googlesource.com/571808
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46808}

src/interpreter/bytecode-generator.cc

@@ -1037,7 +1037,6 @@ void BytecodeGenerator::VisitBlockDeclarationsAndStatements(Block* stmt) {
     VisitDeclarations(stmt->scope()->declarations());
   }
   VisitStatements(stmt->statements());
-  block_builder.EndBlock();
 }
 
 void BytecodeGenerator::VisitVariableDeclaration(VariableDeclaration* decl) {
@@ -1285,7 +1284,8 @@ void BytecodeGenerator::VisitSwitchStatement(SwitchStatement* stmt) {
   // We need this scope because we visit for register values. We have to
   // maintain a execution result scope where registers can be allocated.
   ZoneList<CaseClause*>* clauses = stmt->cases();
-  SwitchBuilder switch_builder(builder(), clauses->length());
+  SwitchBuilder switch_builder(builder(), block_coverage_builder_, stmt,
+                               clauses->length());
   ControlScopeForBreakable scope(this, stmt, &switch_builder);
   int default_index = -1;
 
@@ -1324,13 +1324,9 @@ void BytecodeGenerator::VisitSwitchStatement(SwitchStatement* stmt) {
   // Iterate over all cases and create the case bodies.
   for (int i = 0; i < clauses->length(); i++) {
     CaseClause* clause = clauses->at(i);
-    switch_builder.SetCaseTarget(i);
-    BuildIncrementBlockCoverageCounterIfEnabled(clause, SourceRangeKind::kBody);
+    switch_builder.SetCaseTarget(i, clause);
     VisitStatements(clause->statements());
   }
-  switch_builder.BindBreakTarget();
-  BuildIncrementBlockCoverageCounterIfEnabled(stmt,
-                                              SourceRangeKind::kContinuation);
 }
 
 void BytecodeGenerator::VisitCaseClause(CaseClause* clause) {

src/interpreter/control-flow-builders.cc

@@ -11,7 +11,12 @@ namespace interpreter {
 
 
 BreakableControlFlowBuilder::~BreakableControlFlowBuilder() {
+  BindBreakTarget();
   DCHECK(break_labels_.empty() || break_labels_.is_bound());
+  if (block_coverage_builder_ != nullptr && needs_continuation_counter()) {
+    block_coverage_builder_->IncrementBlockCounter(
+        node_, SourceRangeKind::kContinuation);
+  }
 }
 
 void BreakableControlFlowBuilder::BindBreakTarget() {
@@ -40,29 +45,12 @@ void BreakableControlFlowBuilder::EmitJumpIfNull(BytecodeLabels* sites) {
   builder()->JumpIfNull(sites->New());
 }
 
-void BlockBuilder::EndBlock() {
-  if (statement_->labels() != nullptr) {
-    builder()->Bind(&block_end_);
-    BindBreakTarget();
-  }
-  if (block_coverage_builder_ != nullptr && needs_continuation_counter_) {
-    block_coverage_builder_->IncrementBlockCounter(
-        statement_, SourceRangeKind::kContinuation);
-  }
-}
-
 LoopBuilder::~LoopBuilder() {
   DCHECK(continue_labels_.empty() || continue_labels_.is_bound());
-  BindBreakTarget();
   // Restore the parent jump table.
   if (generator_jump_table_location_ != nullptr) {
     *generator_jump_table_location_ = parent_generator_jump_table_;
   }
-  // Generate block coverage counter for the continuation.
-  if (block_coverage_builder_ != nullptr) {
-    block_coverage_builder_->IncrementBlockCounter(
-        block_coverage_continuation_slot_);
-  }
 }
 
 void LoopBuilder::LoopHeader() {
@@ -120,10 +108,13 @@ SwitchBuilder::~SwitchBuilder() {
 #endif
 }
 
-
-void SwitchBuilder::SetCaseTarget(int index) {
+void SwitchBuilder::SetCaseTarget(int index, CaseClause* clause) {
   BytecodeLabel& site = case_sites_.at(index);
   builder()->Bind(&site);
+  if (block_coverage_builder_) {
+    block_coverage_builder_->IncrementBlockCounter(clause,
+                                                   SourceRangeKind::kBody);
+  }
 }
 
 

src/interpreter/control-flow-builders.h

@@ -34,14 +34,15 @@ class V8_EXPORT_PRIVATE ControlFlowBuilder BASE_EMBEDDED {
 class V8_EXPORT_PRIVATE BreakableControlFlowBuilder
     : public ControlFlowBuilder {
  public:
-  explicit BreakableControlFlowBuilder(BytecodeArrayBuilder* builder)
-      : ControlFlowBuilder(builder), break_labels_(builder->zone()) {}
+  BreakableControlFlowBuilder(BytecodeArrayBuilder* builder,
+                              BlockCoverageBuilder* block_coverage_builder,
+                              AstNode* node)
+      : ControlFlowBuilder(builder),
+        break_labels_(builder->zone()),
+        node_(node),
+        block_coverage_builder_(block_coverage_builder) {}
   virtual ~BreakableControlFlowBuilder();
 
-  // This method should be called by the control flow owner before
-  // destruction to update sites that emit jumps for break.
-  void BindBreakTarget();
-
   // This method is called when visiting break statements in the AST.
   // Inserts a jump to an unbound label that is patched when the corresponding
   // BindBreakTarget is called.
@@ -58,6 +59,9 @@ class V8_EXPORT_PRIVATE BreakableControlFlowBuilder
   BytecodeLabels* break_labels() { return &break_labels_; }
 
   void set_needs_continuation_counter() { needs_continuation_counter_ = true; }
+  bool needs_continuation_counter() const {
+    return needs_continuation_counter_;
+  }
 
  protected:
   void EmitJump(BytecodeLabels* labels);
@@ -68,12 +72,17 @@ class V8_EXPORT_PRIVATE BreakableControlFlowBuilder
   void EmitJumpIfUndefined(BytecodeLabels* labels);
   void EmitJumpIfNull(BytecodeLabels* labels);
 
+  // Called from the destructor to update sites that emit jumps for break.
+  void BindBreakTarget();
+
   // Unbound labels that identify jumps for break statements in the code.
   BytecodeLabels break_labels_;
 
   // A continuation counter (for block coverage) is needed e.g. when
   // encountering a break statement.
+  AstNode* node_;
   bool needs_continuation_counter_ = false;
+  BlockCoverageBuilder* block_coverage_builder_;
 };
 
 
@@ -84,16 +93,8 @@ class V8_EXPORT_PRIVATE BlockBuilder final
   BlockBuilder(BytecodeArrayBuilder* builder,
                BlockCoverageBuilder* block_coverage_builder,
                BreakableStatement* statement)
-      : BreakableControlFlowBuilder(builder),
-        block_coverage_builder_(block_coverage_builder),
-        statement_(statement) {}
-
-  void EndBlock();
-
- private:
-  BytecodeLabel block_end_;
-  BlockCoverageBuilder* block_coverage_builder_;
-  BreakableStatement* statement_;
+      : BreakableControlFlowBuilder(builder, block_coverage_builder,
+                                    statement) {}
 };
 
 
@@ -103,18 +104,15 @@ class V8_EXPORT_PRIVATE LoopBuilder final : public BreakableControlFlowBuilder {
  public:
   LoopBuilder(BytecodeArrayBuilder* builder,
               BlockCoverageBuilder* block_coverage_builder, AstNode* node)
-      : BreakableControlFlowBuilder(builder),
+      : BreakableControlFlowBuilder(builder, block_coverage_builder, node),
         continue_labels_(builder->zone()),
         generator_jump_table_location_(nullptr),
-        parent_generator_jump_table_(nullptr),
-        block_coverage_builder_(block_coverage_builder) {
+        parent_generator_jump_table_(nullptr) {
     if (block_coverage_builder_ != nullptr) {
+      set_needs_continuation_counter();
       block_coverage_body_slot_ =
           block_coverage_builder_->AllocateBlockCoverageSlot(
               node, SourceRangeKind::kBody);
-      block_coverage_continuation_slot_ =
-          block_coverage_builder_->AllocateBlockCoverageSlot(
-              node, SourceRangeKind::kContinuation);
     }
   }
   ~LoopBuilder();
@@ -148,8 +146,6 @@ class V8_EXPORT_PRIVATE LoopBuilder final : public BreakableControlFlowBuilder {
   BytecodeJumpTable* parent_generator_jump_table_;
 
   int block_coverage_body_slot_;
-  int block_coverage_continuation_slot_;
-  BlockCoverageBuilder* block_coverage_builder_;
 };
 
 
@@ -157,8 +153,10 @@ class V8_EXPORT_PRIVATE LoopBuilder final : public BreakableControlFlowBuilder {
 class V8_EXPORT_PRIVATE SwitchBuilder final
     : public BreakableControlFlowBuilder {
  public:
-  explicit SwitchBuilder(BytecodeArrayBuilder* builder, int number_of_cases)
-      : BreakableControlFlowBuilder(builder),
+  SwitchBuilder(BytecodeArrayBuilder* builder,
+                BlockCoverageBuilder* block_coverage_builder,
+                SwitchStatement* statement, int number_of_cases)
+      : BreakableControlFlowBuilder(builder, block_coverage_builder, statement),
         case_sites_(builder->zone()) {
     case_sites_.resize(number_of_cases);
   }
@@ -166,7 +164,7 @@ class V8_EXPORT_PRIVATE SwitchBuilder final
 
   // This method should be called by the SwitchBuilder owner when the case
   // statement with |index| is emitted to update the case jump site.
-  void SetCaseTarget(int index);
+  void SetCaseTarget(int index, CaseClause* clause);
 
   // This method is called when visiting case comparison operation for |index|.
   // Inserts a JumpIfTrue with ToBooleanMode |mode| to a unbound label that is
@@ -189,8 +187,8 @@ class V8_EXPORT_PRIVATE SwitchBuilder final
 // A class to help with co-ordinating control flow in try-catch statements.
 class V8_EXPORT_PRIVATE TryCatchBuilder final : public ControlFlowBuilder {
  public:
-  explicit TryCatchBuilder(BytecodeArrayBuilder* builder,
-                           HandlerTable::CatchPrediction catch_prediction)
+  TryCatchBuilder(BytecodeArrayBuilder* builder,
+                  HandlerTable::CatchPrediction catch_prediction)
       : ControlFlowBuilder(builder),
         handler_id_(builder->NewHandlerEntry()),
         catch_prediction_(catch_prediction) {}
@@ -210,8 +208,8 @@ class V8_EXPORT_PRIVATE TryCatchBuilder final : public ControlFlowBuilder {
 // A class to help with co-ordinating control flow in try-finally statements.
 class V8_EXPORT_PRIVATE TryFinallyBuilder final : public ControlFlowBuilder {
  public:
-  explicit TryFinallyBuilder(BytecodeArrayBuilder* builder,
-                             HandlerTable::CatchPrediction catch_prediction)
+  TryFinallyBuilder(BytecodeArrayBuilder* builder,
+                    HandlerTable::CatchPrediction catch_prediction)
       : ControlFlowBuilder(builder),
         handler_id_(builder->NewHandlerEntry()),
         catch_prediction_(catch_prediction),