[coverage] Filter out singleton ranges that alias full ranges

Block coverage is based on a system of ranges that can either have both a start and end position, or only a start position (so-called singleton ranges). When formatting coverage information, singletons are expanded until the end of the immediate full parent range. E.g. in: {0, 10} // Full range. {5, -1} // Singleton range. the singleton range is expanded to {5, 10}. Singletons are produced mostly for continuation counters that track whether we execute past a specific language construct. Unfortunately, continuation counters can turn up in spots that confuse our post-processing. For example: if (true) { ... block1 ... } else { ... block2 ... } If block1 produces a continuation counter, it could end up with the same start position as the else-branch counter. Since we merge identical blocks, the else-branch could incorrectly end up with an execution count of one. We need to avoid merging such cases. A full range should always take precedence over a singleton range; a singleton range should never expand to completely fill a full range. An additional post-processing pass ensures this. Bug: v8:8237 Change-Id: Idb3ec7b2feddc0585313810b9c8be1e9f4ec64bf Reviewed-on: https://chromium-review.googlesource.com/c/1273095 Reviewed-by: Georg Neis <neis@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/master@{#56531}
v8 · Oct 10, 2018 · aac2f8c · aac2f8c
1 parent 60d3ce7
commit aac2f8c
Show file tree

Hide file tree

Showing 2 changed files with 82 additions and 1 deletion.
diff --git a/src/debug/debug-coverage.cc b/src/debug/debug-coverage.cc
@@ -171,6 +171,12 @@ class CoverageBlockIterator final {
     return function_->blocks[read_index_ + 1];
   }
 
+  CoverageBlock& GetPreviousBlock() {
+    DCHECK(IsActive());
+    DCHECK_GT(read_index_, 0);
+    return function_->blocks[read_index_ - 1];
+  }
+
   CoverageBlock& GetParent() {
     DCHECK(IsActive());
     return nesting_stack_.back();
@@ -325,6 +331,30 @@ void MergeNestedRanges(CoverageFunction* function) {
   }
 }
 
+void FilterAliasedSingletons(CoverageFunction* function) {
+  CoverageBlockIterator iter(function);
+
+  iter.Next();  // Advance once since we reference the previous block later.
+
+  while (iter.Next()) {
+    CoverageBlock& previous_block = iter.GetPreviousBlock();
+    CoverageBlock& block = iter.GetBlock();
+
+    bool is_singleton = block.end == kNoSourcePosition;
+    bool aliases_start = block.start == previous_block.start;
+
+    if (is_singleton && aliases_start) {
+      // The previous block must have a full range since duplicate singletons
+      // have already been merged.
+      DCHECK_NE(previous_block.end, kNoSourcePosition);
+      // Likewise, the next block must have another start position since
+      // singletons are sorted to the end.
+      DCHECK_IMPLIES(iter.HasNext(), iter.GetNextBlock().start != block.start);
+      iter.DeleteBlock();
+    }
+  }
+}
+
 void FilterUncoveredRanges(CoverageFunction* function) {
   CoverageBlockIterator iter(function);
 
@@ -397,6 +427,15 @@ void CollectBlockCoverage(CoverageFunction* function, SharedFunctionInfo* info,
   // Remove duplicate singleton ranges, keeping the max count.
   MergeDuplicateSingletons(function);
 
+  // Remove singleton ranges with the same start position as a full range and
+  // throw away their counts.
+  // Singleton ranges are only intended to split existing full ranges and should
+  // never expand into a full range. Consider 'if (cond) { ... } else { ... }'
+  // as a problematic example; if the then-block produces a continuation
+  // singleton, it would incorrectly expand into the else range.
+  // For more context, see https://crbug.com/v8/8237.
+  FilterAliasedSingletons(function);
+
   // Rewrite all singletons (created e.g. by continuations and unconditional
   // control flow) to ranges.
   RewritePositionSingletonsToRanges(function);

diff --git a/test/mjsunit/code-coverage-block.js b/test/mjsunit/code-coverage-block.js
@@ -847,7 +847,49 @@ Util.escape("foo.bar");                   // 0400
 [{"start":0,"end":449,"count":1},
  {"start":64,"end":351,"count":1},
  {"start":112,"end":203,"count":0},
- {"start":303,"end":350,"count":0}]
+ {"start":268,"end":350,"count":0}]
+);
+
+TestCoverage(
+"https://crbug.com/v8/8237",
+`
+!function() {                             // 0000
+  if (true)                               // 0050
+    while (false) return; else nop();     // 0100
+}();                                      // 0150
+!function() {                             // 0200
+  if (true) l0: { break l0; } else        // 0250
+    if (nop()) { }                        // 0300
+}();                                      // 0350
+!function() {                             // 0400
+  if (true) { if (false) { return; }      // 0450
+  } else if (nop()) { } }();              // 0500
+!function(){                              // 0550
+  if(true)while(false)return;else nop()   // 0600
+}();                                      // 0650
+!function(){                              // 0700
+  if(true) l0:{break l0}else if (nop()){} // 0750
+}();                                      // 0800
+!function(){                              // 0850
+  if(true){if(false){return}}else         // 0900
+    if(nop()){}                           // 0950
+}();                                      // 1000
+`,
+[{"start":0,"end":1049,"count":1},
+ {"start":1,"end":151,"count":1},
+ {"start":118,"end":137,"count":0},
+ {"start":201,"end":351,"count":1},
+ {"start":277,"end":318,"count":0},
+ {"start":401,"end":525,"count":1},
+ {"start":475,"end":486,"count":0},
+ {"start":503,"end":523,"count":0},
+ {"start":551,"end":651,"count":1},
+ {"start":622,"end":639,"count":0},
+ {"start":701,"end":801,"count":1},
+ {"start":773,"end":791,"count":0},
+ {"start":851,"end":1001,"count":1},
+ {"start":920,"end":928,"count":0},
+ {"start":929,"end":965,"count":0}]
 );
 
 %DebugToggleBlockCoverage(false);