Commit aac2f8c9 authored by Jakob Gruber's avatar Jakob Gruber Committed by Commit Bot

[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/1273095Reviewed-by: 's avatarGeorg Neis <neis@chromium.org>
Reviewed-by: 's avatarYang Guo <yangguo@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#56531}
parent 60d3ce72
......@@ -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);
......
......@@ -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);
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment