Skip to content

Commit e8d3d1a

Browse files
committed
[Profiler] Fix ternary error handling coverage
Previously the branches of a ternary would cut off the coverage after a throwing expression, since they have their own region, and we pop child regions when leaving. Update the logic to use the existing counter adjustment logic when we leave the scope, ensuring that we push a new counter to reflect the exit count of the scope.
1 parent 98e65d0 commit e8d3d1a

File tree

3 files changed

+184
-55
lines changed

3 files changed

+184
-55
lines changed

lib/SIL/IR/SILProfiler.cpp

Lines changed: 67 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -876,22 +876,28 @@ struct CoverageMapping : public ASTWalker {
876876
/// Adjust the count for control flow when exiting a scope.
877877
void adjustForNonLocalExits(ASTNode Scope,
878878
llvm::Optional<CounterExpr> ControlFlowAdjust) {
879-
if (Parent.getAsDecl())
879+
// If there are no regions left, there's nothing to adjust.
880+
if (RegionStack.empty())
880881
return;
881882

883+
// If the region is for a brace, check to see if we have a parent labeled
884+
// statement, in which case the exit count needs to account for any direct
885+
// jumps to it though e.g break statements.
882886
llvm::Optional<CounterExpr> JumpsToLabel;
883-
if (auto *ParentStmt = Parent.getAsStmt()) {
884-
if (auto *DCS = dyn_cast<DoCatchStmt>(ParentStmt)) {
885-
// We need to handle the brace of a DoCatchStmt here specially,
886-
// applying the same logic we apply to the catch clauses (handled by
887-
// the CaseStmt logic), we add on the exit count of the branch to the
888-
// statement's exit count.
889-
addToCounter(DCS, getExitCounter());
890-
return;
891-
}
887+
if (Scope.isStmt(StmtKind::Brace)) {
888+
if (auto *ParentStmt = Parent.getAsStmt()) {
889+
if (auto *DCS = dyn_cast<DoCatchStmt>(ParentStmt)) {
890+
// We need to handle the brace of a DoCatchStmt here specially,
891+
// applying the same logic we apply to the catch clauses (handled by
892+
// the CaseStmt logic), we add on the exit count of the branch to the
893+
// statement's exit count.
894+
addToCounter(DCS, getExitCounter());
895+
return;
896+
}
892897

893-
if (auto *LS = dyn_cast<LabeledStmt>(ParentStmt))
894-
JumpsToLabel = getCounter(LS);
898+
if (auto *LS = dyn_cast<LabeledStmt>(ParentStmt))
899+
JumpsToLabel = getCounter(LS);
900+
}
895901
}
896902

897903
if (!ControlFlowAdjust && !JumpsToLabel)
@@ -942,46 +948,55 @@ struct CoverageMapping : public ASTWalker {
942948
return Lexer::getLocForEndOfToken(SM, Node.getEndLoc());
943949
}
944950

951+
/// Record a popped region in the resulting list of regions.
952+
void takePoppedRegion(SourceMappingRegion &&Region, SourceLoc ParentEndLoc) {
953+
LLVM_DEBUG({
954+
llvm::dbgs() << "Popped region: ";
955+
Region.print(llvm::dbgs(), SM);
956+
llvm::dbgs() << "\n";
957+
});
958+
959+
// Don't record incomplete regions.
960+
if (!Region.hasStartLoc())
961+
return;
962+
963+
// Set the region end location to the end location of the parent.
964+
if (!Region.hasEndLoc())
965+
Region.setEndLoc(ParentEndLoc);
966+
967+
// If the range ended up being empty, ignore it (this can happen when we
968+
// replace the counter, and don't extend the region any further).
969+
if (!Region.hasNonEmptyRange())
970+
return;
971+
972+
SourceRegions.push_back(std::move(Region));
973+
}
974+
945975
/// Pop regions from the stack into the function's list of regions.
946976
///
947977
/// Adds all regions from \c ParentNode to the top of the stack to the
948978
/// function's \c SourceRegions.
949979
void popRegions(ASTNode ParentNode) {
950-
auto I = RegionStack.begin(), E = RegionStack.end();
951-
while (I != E &&
952-
I->getNode().getOpaqueValue() != ParentNode.getOpaqueValue())
953-
++I;
980+
auto I = llvm::find_if(RegionStack, [&](const SourceMappingRegion &Region) {
981+
return Region.getNode().getOpaqueValue() == ParentNode.getOpaqueValue();
982+
});
983+
auto E = RegionStack.end();
954984
assert(I != E && "parent not in stack");
955-
auto ParentIt = I;
956-
SourceLoc EndLoc = ParentIt->getEndLoc();
957-
assert(ParentIt->hasNonEmptyRange() && "Pushed node with empty range?");
958-
959-
unsigned FirstPoppedIndex = SourceRegions.size();
960-
(void)FirstPoppedIndex;
961-
SourceRegions.push_back(std::move(*I++));
962-
for (; I != E; ++I) {
963-
if (!I->hasStartLoc())
964-
continue;
965-
if (!I->hasEndLoc())
966-
I->setEndLoc(EndLoc);
967-
968-
// If the range ended up being empty, ignore it (this can happen when we
969-
// replace the counter, and don't extend the region any further).
970-
if (!I->hasNonEmptyRange())
971-
continue;
972-
973-
SourceRegions.push_back(std::move(*I));
974-
}
985+
assert(I->hasNonEmptyRange() && "Pushed node with empty range?");
975986

976-
LLVM_DEBUG({
977-
for (unsigned Idx = FirstPoppedIndex; Idx < SourceRegions.size(); ++Idx) {
978-
llvm::dbgs() << "Popped region: ";
979-
SourceRegions[Idx].print(llvm::dbgs(), SM);
980-
llvm::dbgs() << "\n";
981-
}
982-
});
987+
auto EndLoc = I->getEndLoc();
988+
for (auto &Region : llvm::make_range(I, E))
989+
takePoppedRegion(std::move(Region), EndLoc);
983990

984-
RegionStack.erase(ParentIt, E);
991+
RegionStack.erase(I, E);
992+
}
993+
994+
/// Exit the given region, popping it and its children from the region stack,
995+
/// and adjusting the following counter if needed.
996+
void exitRegion(ASTNode Node) {
997+
auto Adjust = setExitCount(Node);
998+
popRegions(Node);
999+
adjustForNonLocalExits(Node, Adjust);
9851000
}
9861001

9871002
/// Return the currently active region.
@@ -1217,11 +1232,8 @@ struct CoverageMapping : public ASTWalker {
12171232
return Action::Continue(S);
12181233

12191234
if (isa<BraceStmt>(S)) {
1220-
if (hasCounter(S)) {
1221-
auto Adjust = setExitCount(S);
1222-
popRegions(S);
1223-
adjustForNonLocalExits(S, Adjust);
1224-
}
1235+
if (hasCounter(S))
1236+
exitRegion(S);
12251237

12261238
} else if (auto *WS = dyn_cast<WhileStmt>(S)) {
12271239
// Update the condition with the backedge count.
@@ -1339,18 +1351,18 @@ struct CoverageMapping : public ASTWalker {
13391351
if (shouldSkipExpr(E))
13401352
return Action::Continue(E);
13411353

1342-
if (hasCounter(E))
1343-
popRegions(E);
1344-
1345-
// The region following the expression gets current counter minus the
1346-
// error branch counter, i.e the number of times we didn't throw an error.
1347-
if (!RegionStack.empty() && mayExpressionThrow(E)) {
1354+
// The region following the expression gets current counter minus the error
1355+
// branch counter, i.e the number of times we didn't throw an error.
1356+
if (mayExpressionThrow(E)) {
13481357
auto ThrowCount = assignKnownCounter(ProfileCounterRef::errorBranchOf(E));
13491358
replaceCount(
13501359
CounterExpr::Sub(getCurrentCounter(), ThrowCount, CounterBuilder),
13511360
Lexer::getLocForEndOfToken(SM, E->getEndLoc()));
13521361
}
13531362

1363+
if (hasCounter(E))
1364+
exitRegion(E);
1365+
13541366
return Action::Continue(E);
13551367
}
13561368
};

test/Profiler/coverage_errors.swift

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,33 @@ enum SomeErr : Error {
150150
// CHECK: [[BB_ERR]]
151151
// CHECK-NEXT: increment_profiler_counter 1
152152

153+
// func test28() throws -> Int {
154+
// let x = try .random()
155+
// ? throwingFn()
156+
// : throwingFn()
157+
// return x
158+
// }
159+
// CHECK-LABEL: sil hidden @$s15coverage_errors6test28SiyKF : $@convention(thin) () -> (Int, @error any Error)
160+
// CHECK: bb0:
161+
// CHECK: increment_profiler_counter 0
162+
// CHECK: function_ref @$sSb6randomSbyFZ
163+
// CHECK: cond_br {{%[0-9]+}}, [[BB_TRUE:bb[0-9]+]], [[BB_FALSE:bb[0-9]+]]
164+
//
165+
// CHECK: [[BB_FALSE]]
166+
// CHECK: [[THROW_FN:%[0-9]+]] = function_ref @$s15coverage_errors10throwingFnSiyKF
167+
// CHECK: try_apply [[THROW_FN]]() : $@convention(thin) () -> (Int, @error any Error), normal [[BB_NORMAL:bb[0-9]+]], error [[BB_ERR:bb[0-9]+]]
168+
//
169+
// CHECK: [[BB_ERR]]
170+
// CHECK: increment_profiler_counter 3
171+
//
172+
// CHECK: [[BB_TRUE]]
173+
// CHECK: increment_profiler_counter 1
174+
// CHECK: [[THROW_FN:%[0-9]+]] = function_ref @$s15coverage_errors10throwingFnSiyKF
175+
// CHECK: try_apply [[THROW_FN]]() : $@convention(thin) () -> (Int, @error any Error), normal [[BB_NORMAL:bb[0-9]+]], error [[BB_ERR:bb[0-9]+]]
176+
//
177+
// CHECK: [[BB_ERR]]
178+
// CHECK: increment_profiler_counter 2
179+
153180
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors5test1SiyF"
154181
func test1() -> Int { // CHECK-NEXT: [[@LINE]]:21 -> [[@LINE+7]]:2 : 0
155182
do { // CHECK-NEXT: [[@LINE]]:6 -> [[@LINE+3]]:4 : 0
@@ -400,6 +427,68 @@ func test23() throws -> Int { // CHECK-NEXT: [[@LINE]]:29 -> [[@LINE+11]]:2 : 0
400427
return 2
401428
} // CHECK-NEXT: }
402429

430+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test24SiyKF"
431+
func test24() throws -> Int { // CHECK-NEXT: [[@LINE]]:29 -> [[@LINE+4]]:2 : 0
432+
let x = .random() ? try throwingFn() : 0 // CHECK-NEXT: [[@LINE]]:23 -> [[@LINE]]:39 : 1
433+
return x // CHECK-NEXT: [[@LINE-1]]:39 -> [[@LINE]]:11 : (0 - 2)
434+
// CHECK-NEXT: [[@LINE-2]]:42 -> [[@LINE-2]]:43 : (0 - 1)
435+
} // CHECK-NEXT: }
436+
437+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test25SiyKF"
438+
func test25() throws -> Int { // CHECK-NEXT: [[@LINE]]:29 -> [[@LINE+4]]:2 : 0
439+
let x = .random() ? 0 : try throwingFn() // CHECK-NEXT: [[@LINE]]:23 -> [[@LINE]]:24 : 1
440+
return x // CHECK-NEXT: [[@LINE-1]]:27 -> [[@LINE-1]]:43 : (0 - 1)
441+
// CHECK-NEXT: [[@LINE-2]]:43 -> [[@LINE-1]]:11 : (0 - 2)
442+
} // CHECK-NEXT: }
443+
444+
// Note in this case the throws region of the first branch overlaps the
445+
// second branch, which isn't ideal, but it matches what we already do
446+
// for e.g if statements and returns, and doesn't impact the resulting
447+
// coverage since we always take the counter for the smallest subrange,
448+
// which in this case is the region for the second branch.
449+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test26SiyKF"
450+
func test26() throws -> Int { // CHECK-NEXT: [[@LINE]]:29 -> [[@LINE+5]]:2 : 0
451+
let x = .random() ? try throwingFn() : try throwingFn() // CHECK-NEXT: [[@LINE]]:23 -> [[@LINE]]:39 : 1
452+
return x // CHECK-NEXT: [[@LINE-1]]:39 -> [[@LINE]]:11 : (0 - 2)
453+
// CHECK-NEXT: [[@LINE-2]]:42 -> [[@LINE-2]]:58 : (0 - 1)
454+
// CHECK-NEXT: [[@LINE-3]]:58 -> [[@LINE-2]]:11 : ((0 - 2) - 3)
455+
} // CHECK-NEXT: }
456+
457+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test27SiyKF"
458+
func test27() throws -> Int { // CHECK-NEXT: [[@LINE]]:29 -> [[@LINE+5]]:2 : 0
459+
let x = try .random() ? throwingFn() : throwingFn() // CHECK-NEXT: [[@LINE]]:27 -> [[@LINE]]:39 : 1
460+
return x // CHECK-NEXT: [[@LINE-1]]:39 -> [[@LINE]]:11 : (0 - 2)
461+
// CHECK-NEXT: [[@LINE-2]]:42 -> [[@LINE-2]]:54 : (0 - 1)
462+
// CHECK-NEXT: [[@LINE-3]]:54 -> [[@LINE-2]]:11 : ((0 - 2) - 3)
463+
} // CHECK-NEXT: }
464+
465+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test28SiyKF"
466+
func test28() throws -> Int { // CHECK-NEXT: [[@LINE]]:29 -> [[@LINE+5]]:2 : 0
467+
let x = try .random() // CHECK-NEXT: [[@LINE+1]]:7 -> [[@LINE+1]]:19 : 1
468+
? throwingFn() // CHECK-NEXT: [[@LINE]]:19 -> [[@LINE+2]]:11 : (0 - 2)
469+
: throwingFn() // CHECK-NEXT: [[@LINE]]:7 -> [[@LINE]]:19 : (0 - 1)
470+
return x // CHECK-NEXT: [[@LINE-1]]:19 -> [[@LINE]]:11 : ((0 - 2) - 3)
471+
} // CHECK-NEXT: }
472+
473+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test29Si_SityKF"
474+
func test29() throws -> (Int, Int) { // CHECK-NEXT: [[@LINE]]:36 -> [[@LINE+7]]:2 : 0
475+
let x = try .random() // CHECK-NEXT: [[@LINE+1]]:7 -> [[@LINE+1]]:24 : 1
476+
? (throwingFn(), 0) // CHECK-NEXT: [[@LINE]]:20 -> [[@LINE]]:24 : (1 - 2)
477+
: (0, throwingFn()) // CHECK-NEXT: [[@LINE-1]]:24 -> [[@LINE+1]]:11 : (0 - 2)
478+
return x // CHECK-NEXT: [[@LINE-1]]:7 -> [[@LINE-1]]:24 : (0 - 1)
479+
// CHECK-NEXT: [[@LINE-2]]:23 -> [[@LINE-2]]:24 : ((0 - 1) - 3)
480+
// CHECK-NEXT: [[@LINE-3]]:24 -> [[@LINE-2]]:11 : ((0 - 2) - 3)
481+
} // CHECK-NEXT: }
482+
483+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test30Si_SityKF"
484+
func test30() throws -> (Int, Int) { // CHECK-NEXT: [[@LINE]]:36 -> [[@LINE+6]]:2 : 0
485+
let x = try .random() // CHECK-NEXT: [[@LINE+1]]:7 -> [[@LINE+1]]:35 : 1
486+
? (throwingFn(), throwingFn()) // CHECK-NEXT: [[@LINE]]:20 -> [[@LINE]]:35 : (1 - 2)
487+
: (0, 0) // CHECK-NEXT: [[@LINE-1]]:34 -> [[@LINE-1]]:35 : ((1 - 2) - 3)
488+
return x // CHECK-NEXT: [[@LINE-2]]:35 -> [[@LINE]]:11 : ((0 - 2) - 3)
489+
// CHECK-NEXT: [[@LINE-2]]:7 -> [[@LINE-2]]:13 : (0 - 1)
490+
} // CHECK-NEXT: }
491+
403492
struct TestInit {
404493
// CHECK-LABEL: sil_coverage_map {{.*}}// coverage_errors.TestInit.init() -> coverage_errors.TestInit
405494
init() { // CHECK-NEXT: [[@LINE]]:10 -> [[@LINE+5]]:4 : 0

test/Profiler/coverage_errors_exec.swift

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,3 +178,31 @@ func test15() throws -> Int { // CHECK: {{ *}}[[@LINE]]|{{ *}}1
178178
return 2 // CHECK: {{ *}}[[@LINE]]|{{ *}}1
179179
} // CHECK: {{ *}}[[@LINE]]|{{ *}}1
180180
_ = try? test15() // CHECK: {{ *}}[[@LINE]]|{{ *}}1
181+
182+
func test16() throws -> Int { // CHECK: {{ *}}[[@LINE]]|{{ *}}1
183+
let x = try true // CHECK: {{ *}}[[@LINE]]|{{ *}}1
184+
? throwingFn() // CHECK: {{ *}}[[@LINE]]|{{ *}}1
185+
: throwingFn() // CHECK: {{ *}}[[@LINE]]|{{ *}}0
186+
return x // CHECK: {{ *}}[[@LINE]]|{{ *}}0
187+
} // CHECK: {{ *}}[[@LINE]]|{{ *}}1
188+
_ = try? test16() // CHECK: {{ *}}[[@LINE]]|{{ *}}1
189+
190+
// FIXME: The line execution count is misleading here as it includes the
191+
// trailing edge of the thrown error from the first branch (rdar://118654503).
192+
func test17() throws -> Int { // CHECK: {{ *}}[[@LINE]]|{{ *}}1
193+
let x = try false // CHECK: {{ *}}[[@LINE]]|{{ *}}1
194+
? throwingFn() // CHECK: {{ *}}[[@LINE]]|{{ *}}1
195+
: throwingFn() // CHECK: {{ *}}[[@LINE]]|{{ *}}1
196+
return x // CHECK: {{ *}}[[@LINE]]|{{ *}}0
197+
} // CHECK: {{ *}}[[@LINE]]|{{ *}}1
198+
_ = try? test17() // CHECK: {{ *}}[[@LINE]]|{{ *}}1
199+
200+
// FIXME: The line execution count is misleading here as it includes the
201+
// trailing edge of the thrown error from the first branch (rdar://118654503).
202+
func test18() throws -> Int { // CHECK: {{ *}}[[@LINE]]|{{ *}}1
203+
let x = try true // CHECK: {{ *}}[[@LINE]]|{{ *}}1
204+
? noThrowingFn() // CHECK: {{ *}}[[@LINE]]|{{ *}}1
205+
: noThrowingFn() // CHECK: {{ *}}[[@LINE]]|{{ *}}1
206+
return x // CHECK: {{ *}}[[@LINE]]|{{ *}}1
207+
} // CHECK: {{ *}}[[@LINE]]|{{ *}}1
208+
_ = try? test18() // CHECK: {{ *}}[[@LINE]]|{{ *}}1

0 commit comments

Comments
 (0)