Skip to content

Commit aac2f8c

Browse files
schuayCommit Bot
authored and
Commit Bot
committedOct 10, 2018
[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}
1 parent 60d3ce7 commit aac2f8c

File tree

2 files changed

+82
-1
lines changed

2 files changed

+82
-1
lines changed
 

‎src/debug/debug-coverage.cc

+39
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,12 @@ class CoverageBlockIterator final {
171171
return function_->blocks[read_index_ + 1];
172172
}
173173

174+
CoverageBlock& GetPreviousBlock() {
175+
DCHECK(IsActive());
176+
DCHECK_GT(read_index_, 0);
177+
return function_->blocks[read_index_ - 1];
178+
}
179+
174180
CoverageBlock& GetParent() {
175181
DCHECK(IsActive());
176182
return nesting_stack_.back();
@@ -325,6 +331,30 @@ void MergeNestedRanges(CoverageFunction* function) {
325331
}
326332
}
327333

334+
void FilterAliasedSingletons(CoverageFunction* function) {
335+
CoverageBlockIterator iter(function);
336+
337+
iter.Next(); // Advance once since we reference the previous block later.
338+
339+
while (iter.Next()) {
340+
CoverageBlock& previous_block = iter.GetPreviousBlock();
341+
CoverageBlock& block = iter.GetBlock();
342+
343+
bool is_singleton = block.end == kNoSourcePosition;
344+
bool aliases_start = block.start == previous_block.start;
345+
346+
if (is_singleton && aliases_start) {
347+
// The previous block must have a full range since duplicate singletons
348+
// have already been merged.
349+
DCHECK_NE(previous_block.end, kNoSourcePosition);
350+
// Likewise, the next block must have another start position since
351+
// singletons are sorted to the end.
352+
DCHECK_IMPLIES(iter.HasNext(), iter.GetNextBlock().start != block.start);
353+
iter.DeleteBlock();
354+
}
355+
}
356+
}
357+
328358
void FilterUncoveredRanges(CoverageFunction* function) {
329359
CoverageBlockIterator iter(function);
330360

@@ -397,6 +427,15 @@ void CollectBlockCoverage(CoverageFunction* function, SharedFunctionInfo* info,
397427
// Remove duplicate singleton ranges, keeping the max count.
398428
MergeDuplicateSingletons(function);
399429

430+
// Remove singleton ranges with the same start position as a full range and
431+
// throw away their counts.
432+
// Singleton ranges are only intended to split existing full ranges and should
433+
// never expand into a full range. Consider 'if (cond) { ... } else { ... }'
434+
// as a problematic example; if the then-block produces a continuation
435+
// singleton, it would incorrectly expand into the else range.
436+
// For more context, see https://crbug.com/v8/8237.
437+
FilterAliasedSingletons(function);
438+
400439
// Rewrite all singletons (created e.g. by continuations and unconditional
401440
// control flow) to ranges.
402441
RewritePositionSingletonsToRanges(function);

‎test/mjsunit/code-coverage-block.js

+43-1
Original file line numberDiff line numberDiff line change
@@ -847,7 +847,49 @@ Util.escape("foo.bar"); // 0400
847847
[{"start":0,"end":449,"count":1},
848848
{"start":64,"end":351,"count":1},
849849
{"start":112,"end":203,"count":0},
850-
{"start":303,"end":350,"count":0}]
850+
{"start":268,"end":350,"count":0}]
851+
);
852+
853+
TestCoverage(
854+
"https://crbug.com/v8/8237",
855+
`
856+
!function() { // 0000
857+
if (true) // 0050
858+
while (false) return; else nop(); // 0100
859+
}(); // 0150
860+
!function() { // 0200
861+
if (true) l0: { break l0; } else // 0250
862+
if (nop()) { } // 0300
863+
}(); // 0350
864+
!function() { // 0400
865+
if (true) { if (false) { return; } // 0450
866+
} else if (nop()) { } }(); // 0500
867+
!function(){ // 0550
868+
if(true)while(false)return;else nop() // 0600
869+
}(); // 0650
870+
!function(){ // 0700
871+
if(true) l0:{break l0}else if (nop()){} // 0750
872+
}(); // 0800
873+
!function(){ // 0850
874+
if(true){if(false){return}}else // 0900
875+
if(nop()){} // 0950
876+
}(); // 1000
877+
`,
878+
[{"start":0,"end":1049,"count":1},
879+
{"start":1,"end":151,"count":1},
880+
{"start":118,"end":137,"count":0},
881+
{"start":201,"end":351,"count":1},
882+
{"start":277,"end":318,"count":0},
883+
{"start":401,"end":525,"count":1},
884+
{"start":475,"end":486,"count":0},
885+
{"start":503,"end":523,"count":0},
886+
{"start":551,"end":651,"count":1},
887+
{"start":622,"end":639,"count":0},
888+
{"start":701,"end":801,"count":1},
889+
{"start":773,"end":791,"count":0},
890+
{"start":851,"end":1001,"count":1},
891+
{"start":920,"end":928,"count":0},
892+
{"start":929,"end":965,"count":0}]
851893
);
852894

853895
%DebugToggleBlockCoverage(false);

0 commit comments

Comments
 (0)