[coverage] Improve source range precision

This CL improves reported source range precision in a couple of ways:

Source ranges are now standardized to consist of an inclusive start
index and an exclusive end index (similar to what's reported for
functions). For example:

0123456789  // Offset.
{ f(); }    // Block represented as range {0,8}.

Duplicate singleton ranges (i.e. same start and end offsets) are now
merged (this only becomes relevant once jump statement coverage is
added). For example:

for (.) break;  // Break- and loop continuation have same positions.

SourceRangeScope incorrectly collected starting position
(unconditionally) and end position (when no semi-colon was present).

01234567890123  // Offset.
for (.) break   // Loop body range is {8,13}, was {6,9}.

Bug: v8:6000
Change-Id: I62e7c70cc894a20f318330a2fbbcedc47da2b5db
Reviewed-on: https://chromium-review.googlesource.com/541358
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46095}

src/ast/ast.h

@@ -181,13 +181,15 @@ class AstProperties final BASE_EMBEDDED {
 
 DEFINE_OPERATORS_FOR_FLAGS(AstProperties::Flags)
 
+// Specifies a range within the source code. {start} is 0-based and inclusive,
+// {end} is 0-based and exclusive.
 struct SourceRange {
   SourceRange() : SourceRange(kNoSourcePosition, kNoSourcePosition) {}
   SourceRange(int start, int end) : start(start), end(end) {}
   bool IsEmpty() const { return start == kNoSourcePosition; }
   static SourceRange ContinuationOf(const SourceRange& that) {
     return that.IsEmpty() ? SourceRange()
-                          : SourceRange(that.end + 1, kNoSourcePosition);
+                          : SourceRange(that.end, kNoSourcePosition);
   }
   int32_t start, end;
 };

src/d8.cc

@@ -1933,7 +1933,7 @@ void Shell::WriteLcovData(v8::Isolate* isolate, const char* file) {
       for (size_t k = 0; k < function_data.BlockCount(); k++) {
         debug::Coverage::BlockData block_data = function_data.GetBlockData(k);
         int start_line = LineFromOffset(script, block_data.StartOffset());
-        int end_line = LineFromOffset(script, block_data.EndOffset());
+        int end_line = LineFromOffset(script, block_data.EndOffset() - 1);
         uint32_t count = block_data.Count();
         WriteLcovDataForRange(lines, start_line, end_line, count);
       }

src/debug/debug-coverage.cc

@@ -66,16 +66,50 @@ bool CompareCoverageBlock(const CoverageBlock& a, const CoverageBlock& b) {
   return a.start < b.start;
 }
 
+bool HaveSameSourceRange(const CoverageBlock& lhs, const CoverageBlock& rhs) {
+  return lhs.start == rhs.start && lhs.end == rhs.end;
+}
+
+void MergeDuplicateSingletons(std::vector<CoverageBlock>& blocks) {
+  int from = 1;
+  int to = 1;
+  while (from < static_cast<int>(blocks.size())) {
+    CoverageBlock& prev_block = blocks[to - 1];
+    CoverageBlock& this_block = blocks[from];
+
+    // Identical ranges should only occur through singleton ranges. Consider the
+    // ranges for `for (.) break;`: continuation ranges for both the `break` and
+    // `for` statements begin after the trailing semicolon.
+    // Such ranges are merged and keep the maximal execution count.
+    if (HaveSameSourceRange(prev_block, this_block)) {
+      DCHECK_EQ(kNoSourcePosition, this_block.end);  // Singleton range.
+      prev_block.count = std::max(prev_block.count, this_block.count);
+      from++;  // Do not advance {to} cursor.
+      continue;
+    }
+
+    DCHECK(!HaveSameSourceRange(prev_block, this_block));
+
+    // Copy if necessary.
+    if (from != to) blocks[to] = blocks[from];
+
+    from++;
+    to++;
+  }
+  blocks.resize(to);
+}
+
 std::vector<CoverageBlock> GetSortedBlockData(Isolate* isolate,
                                               SharedFunctionInfo* shared) {
   DCHECK(FLAG_block_coverage);
   DCHECK(shared->HasCoverageInfo());
 
-  std::vector<CoverageBlock> result;
-
   CoverageInfo* coverage_info =
       CoverageInfo::cast(shared->GetDebugInfo()->coverage_info());
 
+  std::vector<CoverageBlock> result;
+  if (coverage_info->SlotCount() == 0) return result;
+
   for (int i = 0; i < coverage_info->SlotCount(); i++) {
     const int start_pos = coverage_info->StartSourcePosition(i);
     const int until_pos = coverage_info->EndSourcePosition(i);
@@ -88,6 +122,12 @@ std::vector<CoverageBlock> GetSortedBlockData(Isolate* isolate,
   // Sort according to the block nesting structure.
   std::sort(result.begin(), result.end(), CompareCoverageBlock);
 
+  // Remove duplicate singleton ranges, keeping the max count.
+  MergeDuplicateSingletons(result);
+
+  // TODO(jgruber): Merge consecutive ranges with identical counts, remove empty
+  // ranges.
+
   return result;
 }
 
@@ -103,19 +143,21 @@ void RewritePositionSingletonsToRanges(CoverageFunction* function) {
   for (int i = 0; i < blocks_count; i++) {
     CoverageBlock& block = function->blocks[i];
 
-    while (nesting_stack.back().end < block.start) {
+    while (nesting_stack.back().end <= block.start) {
       nesting_stack.pop_back();
     }
 
     const SourceRange& parent_range = nesting_stack.back();
 
-    DCHECK(block.start != kNoSourcePosition);
+    DCHECK_NE(block.start, kNoSourcePosition);
+    DCHECK_LE(block.end, parent_range.end);
+
     if (block.end == kNoSourcePosition) {
       // The current block ends at the next sibling block (if it exists) or the
       // end of the parent block otherwise.
       if (i < blocks_count - 1 &&
-          function->blocks[i + 1].start <= parent_range.end) {
-        block.end = function->blocks[i + 1].start - 1;
+          function->blocks[i + 1].start < parent_range.end) {
+        block.end = function->blocks[i + 1].start;
       } else {
         block.end = parent_range.end;
       }

src/debug/debug-coverage.h

@@ -18,6 +18,7 @@ class Isolate;
 
 struct CoverageBlock {
   CoverageBlock(int s, int e, uint32_t c) : start(s), end(e), count(c) {}
+  CoverageBlock() : CoverageBlock(kNoSourcePosition, kNoSourcePosition, 0) {}
   int start;
   int end;
   uint32_t count;

src/parsing/parser-base.h

@@ -87,13 +87,15 @@ struct FormalParametersBase {
 class SourceRangeScope final {
  public:
   enum PositionKind {
-    POSITION,
-    PEEK_POS,
+    POSITION_BEG,
+    POSITION_END,
+    PEEK_POSITION_BEG,
+    PEEK_POSITION_END,
   };
 
   SourceRangeScope(Scanner* scanner, SourceRange* range,
-                   PositionKind pre_kind = PEEK_POS,
-                   PositionKind post_kind = POSITION)
+                   PositionKind pre_kind = PEEK_POSITION_BEG,
+                   PositionKind post_kind = POSITION_END)
       : scanner_(scanner), range_(range), post_kind_(post_kind) {
     range_->start = GetPosition(pre_kind);
     DCHECK_NE(range_->start, kNoSourcePosition);
@@ -106,11 +108,15 @@ class SourceRangeScope final {
 
  private:
   int32_t GetPosition(PositionKind kind) {
-    switch (post_kind_) {
-      case POSITION:
+    switch (kind) {
+      case POSITION_BEG:
         return scanner_->location().beg_pos;
-      case PEEK_POS:
+      case POSITION_END:
+        return scanner_->location().end_pos;
+      case PEEK_POSITION_BEG:
         return scanner_->peek_location().beg_pos;
+      case PEEK_POSITION_END:
+        return scanner_->peek_location().end_pos;
       default:
         UNREACHABLE();
     }