Reland "[coverage] add coverage for binary expressions"

This is a reland of 4d3bc552
Original change's description:
> [coverage] add coverage for binary expressions
> 
> Adds block-level coverage tracking for binary && and ||
> expressions. Introduces a BinaryOperation source-range
> for tracking the operations themselves and an Expression
> source-range, used for tracking NaryLogical expressions.
> 
> This builds on work by jgruber@chromium.org in
> the issue.
> 
> TBR=marja@chromium.org
> R=jgruber@chromium.org, rmcilroy@chromium.org
> 
> Bug: v8:6660
> Change-Id: I83a81f13a3514a734c06948b2d3e91138fb00e18
> Reviewed-on: https://chromium-review.googlesource.com/754564
> Commit-Queue: Jakob Gruber <jgruber@chromium.org>
> Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
> Reviewed-by: Jakob Gruber <jgruber@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#49304}

Bug: v8:6660
Change-Id: I1c8571660d6c501d526886867bd841c49d5c44fd
Reviewed-on: https://chromium-review.googlesource.com/778288Reviewed-by: Marja Hölttä <marja@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49613}

AUTHORS

@@ -46,6 +46,7 @@ Andrew Paprocki <andrew@ishiboo.com>
 Andrei Kashcha <anvaka@gmail.com>
 Anna Henningsen <anna@addaleax.net>
 Bangfu Tao <bangfu.tao@samsung.com>
+Ben Coe <ben@npmjs.com>
 Ben Noordhuis <info@bnoordhuis.nl>
 Benjamin Tan <demoneaux@gmail.com>
 Bert Belder <bertbelder@gmail.com>

src/ast/ast-source-ranges.h

@@ -30,12 +30,14 @@ struct SourceRange {
 // The list of ast node kinds that have associated source ranges. Note that this
 // macro is not undefined at the end of this file.
 #define AST_SOURCE_RANGE_LIST(V) \
+  V(BinaryOperation)             \
   V(Block)                       \
   V(CaseClause)                  \
   V(Conditional)                 \
   V(IfStatement)                 \
   V(IterationStatement)          \
   V(JumpStatement)               \
+  V(NaryOperation)               \
   V(Suspend)                     \
   V(SwitchStatement)             \
   V(Throw)                       \
@@ -48,6 +50,7 @@ enum class SourceRangeKind {
   kContinuation,
   kElse,
   kFinally,
+  kRight,
   kThen,
 };
 
@@ -57,6 +60,20 @@ class AstNodeSourceRanges : public ZoneObject {
   virtual SourceRange GetRange(SourceRangeKind kind) = 0;
 };
 
+class BinaryOperationSourceRanges final : public AstNodeSourceRanges {
+ public:
+  explicit BinaryOperationSourceRanges(const SourceRange& right_range)
+      : right_range_(right_range) {}
+
+  SourceRange GetRange(SourceRangeKind kind) {
+    DCHECK_EQ(kind, SourceRangeKind::kRight);
+    return right_range_;
+  }
+
+ private:
+  SourceRange right_range_;
+};
+
 class ContinuationSourceRanges : public AstNodeSourceRanges {
  public:
   explicit ContinuationSourceRanges(int32_t continuation_position)
@@ -166,6 +183,27 @@ class JumpStatementSourceRanges final : public ContinuationSourceRanges {
       : ContinuationSourceRanges(continuation_position) {}
 };
 
+class NaryOperationSourceRanges final : public AstNodeSourceRanges {
+ public:
+  NaryOperationSourceRanges(Zone* zone, const SourceRange& range)
+      : ranges_(zone) {
+    AddRange(range);
+  }
+
+  SourceRange GetRangeAtIndex(size_t index) {
+    DCHECK(index < ranges_.size());
+    return ranges_[index];
+  }
+
+  void AddRange(const SourceRange& range) { ranges_.push_back(range); }
+  size_t RangeCount() const { return ranges_.size(); }
+
+  SourceRange GetRange(SourceRangeKind kind) { UNREACHABLE(); }
+
+ private:
+  ZoneVector<SourceRange> ranges_;
+};
+
 class SuspendSourceRanges final : public ContinuationSourceRanges {
  public:
   explicit SuspendSourceRanges(int32_t continuation_position)

src/interpreter/block-coverage-builder.h

@@ -41,6 +41,19 @@ class BlockCoverageBuilder final : public ZoneObject {
     return slot;
   }
 
+  int AllocateNaryBlockCoverageSlot(NaryOperation* node, size_t index) {
+    NaryOperationSourceRanges* ranges =
+        static_cast<NaryOperationSourceRanges*>(source_range_map_->Find(node));
+    if (ranges == nullptr) return kNoCoverageArraySlot;
+
+    SourceRange range = ranges->GetRangeAtIndex(index);
+    if (range.IsEmpty()) return kNoCoverageArraySlot;
+
+    const int slot = static_cast<int>(slots_.size());
+    slots_.emplace_back(range);
+    return slot;
+  }
+
   void IncrementBlockCounter(int coverage_array_slot) {
     if (coverage_array_slot == kNoCoverageArraySlot) return;
     builder_->IncBlockCounter(coverage_array_slot);

src/interpreter/bytecode-generator.h

@@ -56,6 +56,7 @@ class BytecodeGenerator final : public AstVisitor<BytecodeGenerator> {
   class EffectResultScope;
   class FeedbackSlotCache;
   class GlobalDeclarationsBuilder;
+  class NaryCodeCoverageSlots;
   class RegisterAllocationScope;
   class TestResultScope;
   class ValueResultScope;
@@ -171,20 +172,23 @@ class BytecodeGenerator final : public AstVisitor<BytecodeGenerator> {
 
   // Visit a logical OR/AND within a test context, rewiring the jumps based
   // on the expression values.
-  void VisitLogicalTest(Token::Value token, Expression* left,
-                        Expression* right);
-  void VisitNaryLogicalTest(Token::Value token, NaryOperation* expr);
+  void VisitLogicalTest(Token::Value token, Expression* left, Expression* right,
+                        int right_coverage_slot);
+  void VisitNaryLogicalTest(Token::Value token, NaryOperation* expr,
+                            const NaryCodeCoverageSlots* coverage_slots);
   // Visit a (non-RHS) test for a logical op, which falls through if the test
   // fails or jumps to the appropriate labels if it succeeds.
   void VisitLogicalTestSubExpression(Token::Value token, Expression* expr,
                                      BytecodeLabels* then_labels,
-                                     BytecodeLabels* else_labels);
+                                     BytecodeLabels* else_labels,
+                                     int coverage_slot);
 
   // Helpers for binary and nary logical op value expressions.
-  bool VisitLogicalOrSubExpression(Expression* expr,
-                                   BytecodeLabels* end_labels);
+  bool VisitLogicalOrSubExpression(Expression* expr, BytecodeLabels* end_labels,
+                                   int coverage_slot);
   bool VisitLogicalAndSubExpression(Expression* expr,
-                                    BytecodeLabels* end_labels);
+                                    BytecodeLabels* end_labels,
+                                    int coverage_slot);
 
   // Visit the header/body of a loop iteration.
   void VisitIterationHeader(IterationStatement* stmt,
@@ -201,6 +205,8 @@ class BytecodeGenerator final : public AstVisitor<BytecodeGenerator> {
   void BuildLoadPropertyKey(LiteralProperty* property, Register out_reg);
 
   int AllocateBlockCoverageSlotIfEnabled(AstNode* node, SourceRangeKind kind);
+  int AllocateNaryBlockCoverageSlotIfEnabled(NaryOperation* node, size_t index);
+
   void BuildIncrementBlockCoverageCounterIfEnabled(AstNode* node,
                                                    SourceRangeKind kind);
   void BuildIncrementBlockCoverageCounterIfEnabled(int coverage_array_slot);

src/parsing/parser-base.h

@@ -1980,7 +1980,8 @@ ParserBase<Impl>::ParseExpressionCoverGrammar(bool accept_IN, bool* ok) {
       // First time through the loop.
       result = right;
     } else if (impl()->CollapseNaryExpression(&result, right, Token::COMMA,
-                                              comma_pos)) {
+                                              comma_pos,
+                                              SourceRange::Empty())) {
       // Do nothing, "result" is already updated.
     } else {
       result =
@@ -3081,6 +3082,7 @@ template <typename Impl>
 typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParseBinaryExpression(
     int prec, bool accept_IN, bool* ok) {
   DCHECK_GE(prec, 4);
+  SourceRange right_range;
   ExpressionT x = ParseUnaryExpression(CHECK_OK);
   for (int prec1 = Precedence(peek(), accept_IN); prec1 >= prec; prec1--) {
     // prec1 >= 4
@@ -3088,12 +3090,15 @@ typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParseBinaryExpression(
       impl()->RewriteNonPattern(CHECK_OK);
       BindingPatternUnexpectedToken();
       ArrowFormalParametersUnexpectedToken();
+
+      SourceRangeScope right_range_scope(scanner(), &right_range);
       Token::Value op = Next();
       int pos = position();
 
       const bool is_right_associative = op == Token::EXP;
       const int next_prec = is_right_associative ? prec1 : prec1 + 1;
       ExpressionT y = ParseBinaryExpression(next_prec, accept_IN, CHECK_OK);
+      right_range_scope.Finalize();
       impl()->RewriteNonPattern(CHECK_OK);
 
       if (impl()->ShortcutNumericLiteralBinaryExpression(&x, y, op, pos)) {
@@ -3118,11 +3123,14 @@ typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParseBinaryExpression(
         }
       } else if (op == Token::EXP) {
         x = impl()->RewriteExponentiation(x, y, pos);
-      } else if (impl()->CollapseNaryExpression(&x, y, op, pos)) {
+      } else if (impl()->CollapseNaryExpression(&x, y, op, pos, right_range)) {
         continue;
       } else {
         // We have a "normal" binary operation.
         x = factory()->NewBinaryOperation(op, x, y, pos);
+        if (op == Token::OR || op == Token::AND) {
+          impl()->RecordBinaryOperationSourceRange(x, right_range);
+        }
       }
     }
   }

src/parsing/parser.cc

@@ -307,7 +307,8 @@ bool Parser::ShortcutNumericLiteralBinaryExpression(Expression** x,
 }
 
 bool Parser::CollapseNaryExpression(Expression** x, Expression* y,
-                                    Token::Value op, int pos) {
+                                    Token::Value op, int pos,
+                                    const SourceRange& range) {
   // Filter out unsupported ops.
   if (!Token::IsBinaryOp(op) || op == Token::EXP) return false;
 
@@ -320,6 +321,7 @@ bool Parser::CollapseNaryExpression(Expression** x, Expression* y,
 
     nary = factory()->NewNaryOperation(op, binop->left(), 2);
     nary->AddSubsequent(binop->right(), binop->position());
+    ConvertBinaryToNaryOperationSourceRange(binop, nary);
     *x = nary;
   } else if ((*x)->IsNaryOperation()) {
     nary = (*x)->AsNaryOperation();
@@ -332,6 +334,8 @@ bool Parser::CollapseNaryExpression(Expression** x, Expression* y,
   // TODO(leszeks): Do some literal collapsing here if we're appending Smi or
   // String literals.
   nary->AddSubsequent(y, pos);
+  AppendNaryOperationSourceRange(nary, range);
+
   return true;
 }
 

src/parsing/parser.h

@@ -765,7 +765,7 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   // into a single n-ary expression. In that case, *x will be changed to an
   // n-ary expression.
   bool CollapseNaryExpression(Expression** x, Expression* y, Token::Value op,
-                              int pos);
+                              int pos, const SourceRange& range);
 
   // Rewrites the following types of unary expressions:
   // not <literal> -> true / false
@@ -1011,6 +1011,32 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
     return parameters_end_pos_ != kNoSourcePosition;
   }
 
+  V8_INLINE void ConvertBinaryToNaryOperationSourceRange(
+      BinaryOperation* binary_op, NaryOperation* nary_op) {
+    if (source_range_map_ == nullptr) return;
+    DCHECK_NULL(source_range_map_->Find(nary_op));
+
+    BinaryOperationSourceRanges* ranges =
+        static_cast<BinaryOperationSourceRanges*>(
+            source_range_map_->Find(binary_op));
+    if (ranges == nullptr) return;
+
+    SourceRange range = ranges->GetRange(SourceRangeKind::kRight);
+    source_range_map_->Insert(
+        nary_op, new (zone()) NaryOperationSourceRanges(zone(), range));
+  }
+
+  V8_INLINE void AppendNaryOperationSourceRange(NaryOperation* node,
+                                                const SourceRange& range) {
+    if (source_range_map_ == nullptr) return;
+    NaryOperationSourceRanges* ranges =
+        static_cast<NaryOperationSourceRanges*>(source_range_map_->Find(node));
+    if (ranges == nullptr) return;
+
+    ranges->AddRange(range);
+    DCHECK_EQ(node->subsequent_length(), ranges->RangeCount());
+  }
+
   V8_INLINE void RecordBlockSourceRange(Block* node,
                                         int32_t continuation_position) {
     if (source_range_map_ == nullptr) return;
@@ -1034,6 +1060,14 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
         new (zone()) ConditionalSourceRanges(then_range, else_range));
   }
 
+  V8_INLINE void RecordBinaryOperationSourceRange(
+      Expression* node, const SourceRange& right_range) {
+    if (source_range_map_ == nullptr) return;
+    source_range_map_->Insert(node->AsBinaryOperation(),
+                              new (zone())
+                                  BinaryOperationSourceRanges(right_range));
+  }
+
   V8_INLINE void RecordJumpStatementSourceRange(Statement* node,
                                                 int32_t continuation_position) {
     if (source_range_map_ == nullptr) return;

src/parsing/preparser.h

@@ -1306,10 +1306,11 @@ class PreParser : public ParserBase<PreParser> {
     return false;
   }
 
-  V8_INLINE bool CollapseNaryExpression(PreParserExpression* x,
-                                        PreParserExpression y, Token::Value op,
-                                        int pos) {
-    return false;
+  V8_INLINE NaryOperation* CollapseNaryExpression(PreParserExpression* x,
+                                                  PreParserExpression y,
+                                                  Token::Value op, int pos,
+                                                  const SourceRange& range) {
+    return nullptr;
   }
 
   V8_INLINE PreParserExpression BuildUnaryExpression(

test/mjsunit/code-coverage-block.js

@@ -666,4 +666,166 @@ f();                                      // 0200
  {"start":61,"end":150,"count":1}]
 );
 
+TestCoverage(
+"LogicalOrExpression assignment",
+`
+const a = true || 99                      // 0000
+function b () {                           // 0050
+  const b = a || 2                        // 0100
+}                                         // 0150
+b()                                       // 0200
+b()                                       // 0250
+`,
+[{"start":0,"end":299,"count":1},
+ {"start":15,"end":20,"count":0},
+ {"start":50,"end":151,"count":2},
+ {"start":114,"end":118,"count":0}]);
+
+TestCoverage(
+"LogicalOrExpression IsTest()",
+`
+true || false                             // 0000
+const a = 99                              // 0050
+a || 50                                   // 0100
+const b = false                           // 0150
+if (b || true) {}                         // 0200
+`,
+[{"start":0,"end":249,"count":1},
+ {"start":5,"end":13,"count":0},
+ {"start":102,"end":107,"count":0}]);
+
+TestCoverage(
+"LogicalAndExpression assignment",
+`
+const a = false && 99                     // 0000
+function b () {                           // 0050
+  const b = a && 2                        // 0100
+}                                         // 0150
+b()                                       // 0200
+b()                                       // 0250
+const c = true && 50                      // 0300
+`,
+[{"start":0,"end":349,"count":1},
+ {"start":16,"end":21,"count":0},
+ {"start":50,"end":151,"count":2},
+ {"start":114,"end":118,"count":0}]);
+
+TestCoverage(
+"LogicalAndExpression IsTest()",
+`
+false && true                             // 0000
+const a = 0                               // 0050
+a && 50                                   // 0100
+const b = true                            // 0150
+if (b && true) {}                         // 0200
+true && true                              // 0250
+`,
+[{"start":0,"end":299,"count":1},
+ {"start":6,"end":13,"count":0},
+ {"start":102,"end":107,"count":0}]);
+
+TestCoverage(
+"NaryLogicalOr assignment",
+`
+const a = true                            // 0000
+const b = false                           // 0050
+const c = false || false || 99            // 0100
+const d = false || true || 99             // 0150
+const e = true || true || 99              // 0200
+const f = b || b || 99                    // 0250
+const g = b || a || 99                    // 0300
+const h = a || a || 99                    // 0350
+const i = a || (b || c) || d              // 0400
+`,
+[{"start":0,"end":449,"count":1},
+ {"start":174,"end":179,"count":0},
+ {"start":215,"end":222,"count":0},
+ {"start":223,"end":228,"count":0},
+ {"start":317,"end":322,"count":0},
+ {"start":362,"end":366,"count":0},
+ {"start":367,"end":372,"count":0},
+ {"start":412,"end":423,"count":0},
+ {"start":424,"end":428,"count":0}]);
+
+TestCoverage(
+"NaryLogicalOr IsTest()",
+`
+const a = true                            // 0000
+const b = false                           // 0050
+false || false || 99                      // 0100
+false || true || 99                       // 0150
+true || true || 99                        // 0200
+b || b || 99                              // 0250
+b || a || 99                              // 0300
+a || a || 99                              // 0350
+`,
+[{"start":0,"end":399,"count":1},
+ {"start":164,"end":169,"count":0},
+ {"start":205,"end":212,"count":0},
+ {"start":213,"end":218,"count":0},
+ {"start":307,"end":312,"count":0},
+ {"start":352,"end":356,"count":0},
+ {"start":357,"end":362,"count":0}]);
+
+TestCoverage(
+"NaryLogicalAnd assignment",
+`
+const a = true                            // 0000
+const b = false                           // 0050
+const c = false && false && 99            // 0100
+const d = false && true && 99             // 0150
+const e = true && true && 99              // 0200
+const f = true && false || true           // 0250
+const g = true || false && true           // 0300
+`,
+[{"start":0,"end":349,"count":1},
+ {"start":116,"end":124,"count":0},
+ {"start":125,"end":130,"count":0},
+ {"start":166,"end":173,"count":0},
+ {"start":174,"end":179,"count":0},
+ {"start":315,"end":331,"count":0}
+]);
+
+TestCoverage(
+"NaryLogicalAnd IsTest()",
+`
+const a = true                            // 0000
+const b = false                           // 0050
+false && false && 99                      // 0100
+false && true && 99                       // 0150
+true && true && 99                        // 0200
+true && false || true                     // 0250
+true || false && true                     // 0300
+false || false || 99 || 55                // 0350
+`,
+[{"start":0,"end":399,"count":1},
+ {"start":106,"end":114,"count":0},
+ {"start":115,"end":120,"count":0},
+ {"start":156,"end":163,"count":0},
+ {"start":164,"end":169,"count":0},
+ {"start":305,"end":321,"count":0},
+ {"start":371,"end":376,"count":0}]);
+
+// see regression: https://bugs.chromium.org/p/chromium/issues/detail?id=785778
+TestCoverage(
+"logical expressions + conditional expressions",
+`
+const a = true                            // 0000
+const b = 99                              // 0050
+const c = false                           // 0100
+const d = ''                              // 0150
+const e = a && (b ? 'left' : 'right')     // 0200
+const f = a || (b ? 'left' : 'right')     // 0250
+const g = c || d ? 'left' : 'right'       // 0300
+const h = a && b && (b ? 'left' : 'right')// 0350
+const i = d || c || (c ? 'left' : 'right')// 0400
+`,
+[{"start":0,"end":449,"count":1},
+ {"start":227,"end":236,"count":0},
+ {"start":262,"end":287,"count":0},
+ {"start":317,"end":325,"count":0},
+ {"start":382,"end":391,"count":0},
+ {"start":423,"end":431,"count":0}
+]);
+
 %DebugToggleBlockCoverage(false);