From aac2f8c933f82064f37e218a00572092ac6dfe37 Mon Sep 17 00:00:00 2001 From: Jakob Gruber Date: Wed, 10 Oct 2018 14:44:11 +0200 Subject: [PATCH] [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 Reviewed-by: Yang Guo Commit-Queue: Jakob Gruber Cr-Commit-Position: refs/heads/master@{#56531} --- src/debug/debug-coverage.cc | 39 +++++++++++++++++++++++++ test/mjsunit/code-coverage-block.js | 44 ++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/src/debug/debug-coverage.cc b/src/debug/debug-coverage.cc index a71c9e572b..f8b716f7c9 100644 --- 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 index 2ecd1b0d1e..8cbb2969f7 100644 --- 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);