Skip to content

Commit 59275b2

Browse files
hamishknighthamishknight
committed
[Profiler] Correctly handle try? expressions
Ensure that `try?` expressions appropriately scope the regions generated by child error-throwing expressions, such that the non-error branch does not extend beyond the region of the `try?`.
1 parent e8d3d1a commit 59275b2

File tree

3 files changed

+310
-31
lines changed

3 files changed

+310
-31
lines changed

‎lib/SIL/IR/SILProfiler.cpp

+85-23
Original file line numberDiff line numberDiff line change
@@ -433,6 +433,23 @@ class CounterExpr {
433433

434434
/// A region of source code that can be mapped to a counter.
435435
class SourceMappingRegion {
436+
public:
437+
enum class Kind {
438+
/// A region that is associated with an ASTNode, and defines a scope under
439+
/// which the region is active.
440+
Node,
441+
442+
/// A node region that is only present for scoping of child regions, and
443+
/// doesn't need to be included in the resulting set of regions.
444+
ScopingOnly,
445+
446+
/// A region that refines the counter of a node region. This doesn't have
447+
/// an ASTNode of its own.
448+
Refined,
449+
};
450+
451+
private:
452+
Kind RegionKind;
436453
ASTNode Node;
437454

438455
/// The counter for an incomplete region. Note we do not store counters
@@ -445,21 +462,54 @@ class SourceMappingRegion {
445462
/// The region's ending location.
446463
llvm::Optional<SourceLoc> EndLoc;
447464

448-
public:
449-
SourceMappingRegion(ASTNode Node, llvm::Optional<CounterExpr> Counter,
450-
llvm::Optional<SourceLoc> StartLoc,
451-
llvm::Optional<SourceLoc> EndLoc)
452-
: Node(Node), Counter(std::move(Counter)), StartLoc(StartLoc),
453-
EndLoc(EndLoc) {
465+
SourceMappingRegion(Kind RegionKind, llvm::Optional<CounterExpr> Counter,
466+
llvm::Optional<SourceLoc> StartLoc)
467+
: RegionKind(RegionKind), Counter(Counter), StartLoc(StartLoc) {
454468
assert((!StartLoc || StartLoc->isValid()) &&
455469
"Expected start location to be valid");
456-
assert((!EndLoc || EndLoc->isValid()) &&
457-
"Expected end location to be valid");
470+
}
471+
472+
SourceMappingRegion(Kind RegionKind, ASTNode Node, SourceRange Range,
473+
const SourceManager &SM)
474+
: RegionKind(RegionKind), Node(Node) {
475+
assert(Range.isValid());
476+
StartLoc = Range.Start;
477+
EndLoc = Lexer::getLocForEndOfToken(SM, Range.End);
478+
}
479+
480+
public:
481+
/// Create a regular source region for an ASTNode.
482+
static SourceMappingRegion forNode(ASTNode Node, const SourceManager &SM,
483+
SourceRange Range = SourceRange()) {
484+
if (Range.isInvalid())
485+
Range = Node.getSourceRange();
486+
487+
// Note we don't store counters for nodes, as we need to be able to fix them
488+
// up later.
489+
return SourceMappingRegion(Kind::Node, Node, Range, SM);
490+
}
491+
492+
/// Create a source region for an ASTNode that is only present for scoping of
493+
/// child regions, and doesn't need to be included in the resulting set of
494+
/// regions.
495+
static SourceMappingRegion scopingOnly(ASTNode Node,
496+
const SourceManager &SM) {
497+
return SourceMappingRegion(Kind::ScopingOnly, Node, Node.getSourceRange(),
498+
SM);
499+
}
500+
501+
/// Create a refined region for a given counter.
502+
static SourceMappingRegion refined(CounterExpr Counter,
503+
llvm::Optional<SourceLoc> StartLoc) {
504+
return SourceMappingRegion(Kind::Refined, Counter, StartLoc);
458505
}
459506

460507
SourceMappingRegion(SourceMappingRegion &&Region) = default;
461508
SourceMappingRegion &operator=(SourceMappingRegion &&RHS) = default;
462509

510+
/// Whether this region is for scoping only.
511+
bool isForScopingOnly() const { return RegionKind == Kind::ScopingOnly; }
512+
463513
ASTNode getNode() const { return Node; }
464514

465515
CounterExpr getCounter(const llvm::DenseMap<ProfileCounterRef, CounterExpr>
@@ -867,6 +917,13 @@ struct CoverageMapping : public ASTWalker {
867917
/// Returns the delta of the count on entering \c Node and exiting, or null if
868918
/// there was no change.
869919
llvm::Optional<CounterExpr> setExitCount(ASTNode Node) {
920+
// A `try?` absorbs child error branches, so we can assume the exit count is
921+
// the same as the entry count in that case.
922+
// NOTE: This assumes there is no other kind of control flow that can happen
923+
// in a nested expression, which is true today, but may not always be.
924+
if (Node.isExpr(ExprKind::OptionalTry))
925+
return llvm::None;
926+
870927
ExitCounter = getCurrentCounter();
871928
if (hasCounter(Node) && getRegion().getNode() != Node)
872929
return CounterExpr::Sub(getCounter(Node), *ExitCounter, CounterBuilder);
@@ -914,20 +971,14 @@ struct CoverageMapping : public ASTWalker {
914971
replaceCount(Count, getEndLoc(Scope));
915972
}
916973

917-
/// Push a region covering \c Node onto the stack.
918-
void pushRegion(ASTNode Node, SourceRange Range = SourceRange()) {
919-
if (Range.isInvalid())
920-
Range = Node.getSourceRange();
921-
922-
// Note we don't store counters for nodes, as we need to be able to fix
923-
// them up later.
924-
RegionStack.emplace_back(Node, /*Counter*/ llvm::None, Range.Start,
925-
Lexer::getLocForEndOfToken(SM, Range.End));
974+
/// Push a region onto the stack.
975+
void pushRegion(SourceMappingRegion Region) {
926976
LLVM_DEBUG({
927977
llvm::dbgs() << "Pushed region: ";
928-
RegionStack.back().print(llvm::dbgs(), SM);
978+
Region.print(llvm::dbgs(), SM);
929979
llvm::dbgs() << "\n";
930980
});
981+
RegionStack.push_back(std::move(Region));
931982
}
932983

933984
/// Replace the current region at \p Start with a new counter. If \p Start is
@@ -940,7 +991,7 @@ struct CoverageMapping : public ASTWalker {
940991
if (Start && Counter.isZero())
941992
Start = llvm::None;
942993

943-
RegionStack.emplace_back(ASTNode(), Counter, Start, llvm::None);
994+
pushRegion(SourceMappingRegion::refined(Counter, Start));
944995
}
945996

946997
/// Get the location for the end of the last token in \c Node.
@@ -956,6 +1007,10 @@ struct CoverageMapping : public ASTWalker {
9561007
llvm::dbgs() << "\n";
9571008
});
9581009

1010+
// Don't bother recording regions that are only present for scoping.
1011+
if (Region.isForScopingOnly())
1012+
return;
1013+
9591014
// Don't record incomplete regions.
9601015
if (!Region.hasStartLoc())
9611016
return;
@@ -1128,7 +1183,7 @@ struct CoverageMapping : public ASTWalker {
11281183

11291184
if (auto *BS = dyn_cast<BraceStmt>(S)) {
11301185
if (hasCounter(BS))
1131-
pushRegion(BS);
1186+
pushRegion(SourceMappingRegion::forNode(BS, SM));
11321187

11331188
} else if (auto *IS = dyn_cast<IfStmt>(S)) {
11341189
if (auto *Cond = getConditionNode(IS->getCond()))
@@ -1222,7 +1277,7 @@ struct CoverageMapping : public ASTWalker {
12221277
Range = CS->getSourceRange();
12231278
break;
12241279
}
1225-
pushRegion(CS, Range);
1280+
pushRegion(SourceMappingRegion::forNode(CS, SM, Range));
12261281
}
12271282
return Action::Continue(S);
12281283
}
@@ -1326,8 +1381,15 @@ struct CoverageMapping : public ASTWalker {
13261381
if (isa<LazyInitializerExpr>(E))
13271382
assignKnownCounter(E);
13281383

1329-
if (hasCounter(E))
1330-
pushRegion(E);
1384+
if (hasCounter(E)) {
1385+
pushRegion(SourceMappingRegion::forNode(E, SM));
1386+
} else if (isa<OptionalTryExpr>(E)) {
1387+
// If we have a `try?`, that doesn't already have a counter, record it
1388+
// as a scoping-only region. We need it to scope child error branches,
1389+
// but don't need it in the resulting set of regions.
1390+
assignCounter(E, getCurrentCounter());
1391+
pushRegion(SourceMappingRegion::scopingOnly(E, SM));
1392+
}
13311393

13321394
assert(!RegionStack.empty() && "Must be within a region");
13331395

‎test/Profiler/coverage_errors.swift

+172
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,26 @@ enum SomeErr : Error {
177177
// CHECK: [[BB_ERR]]
178178
// CHECK: increment_profiler_counter 2
179179

180+
// func test43() -> Int? {
181+
// let x = try? throwingS.throwingMethod()
182+
// return x
183+
// }
184+
// CHECK-LABEL: sil hidden @$s15coverage_errors6test43SiSgyF : $@convention(thin) () -> Optional<Int>
185+
// CHECK: bb0:
186+
// CHECK: increment_profiler_counter 0
187+
// CHECK: [[GETTER:%[0-9]+]] = function_ref @$s15coverage_errors9throwingSAA1SVvg : $@convention(thin) () -> (S, @error any Error)
188+
// CHECK: try_apply [[GETTER]]() : $@convention(thin) () -> (S, @error any Error), normal [[BB_NORMAL:bb[0-9]+]], error [[BB_ERR:bb[0-9]+]]
189+
//
190+
// CHECK: [[BB_ERR]]
191+
// CHECK: increment_profiler_counter 1
192+
//
193+
// CHECK: [[BB_NORMAL]]
194+
// CHECK: [[METHOD:%[0-9]+]] = function_ref @$s15coverage_errors1SV14throwingMethodSiyKF : $@convention(method) (S) -> (Int, @error any Error) // user: %5
195+
// CHECK: try_apply [[METHOD]]({{%[0-9]+}}) : $@convention(method) (S) -> (Int, @error any Error), normal [[BB_NORMAL:bb[0-9]+]], error [[BB_ERR:bb[0-9]+]]
196+
//
197+
// CHECK: [[BB_ERR]]
198+
// CHECK: increment_profiler_counter 2
199+
180200
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors5test1SiyF"
181201
func test1() -> Int { // CHECK-NEXT: [[@LINE]]:21 -> [[@LINE+7]]:2 : 0
182202
do { // CHECK-NEXT: [[@LINE]]:6 -> [[@LINE+3]]:4 : 0
@@ -489,6 +509,129 @@ func test30() throws -> (Int, Int) { // CHECK-NEXT: [[@LINE]]:36 -> [[@LINE+6]
489509
// CHECK-NEXT: [[@LINE-2]]:7 -> [[@LINE-2]]:13 : (0 - 1)
490510
} // CHECK-NEXT: }
491511

512+
@discardableResult
513+
func takesOptInts(_ x: Int?, _ y: Int?) -> Int { 0 }
514+
515+
// `throwingFn` is within a `try?`, so the non-error branch is empty.
516+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test31yyF"
517+
func test31() { // CHECK-NEXT: [[@LINE]]:15 -> [[@LINE+2]]:2 : 0
518+
takesOptInts(try? throwingFn(), 0)
519+
} // CHECK-NEXT: }
520+
521+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test32yyF"
522+
func test32() { // CHECK-NEXT: [[@LINE]]:15 -> [[@LINE+5]]:2 : 0
523+
takesOptInts(
524+
try? throwingFn(),
525+
try? throwingFn()
526+
)
527+
} // CHECK-NEXT: }
528+
529+
// Here the throws region extends into the second arg.
530+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test33SiSgyF"
531+
func test33() -> Int? { // CHECK-NEXT: [[@LINE]]:23 -> [[@LINE+2]]:2 : 0
532+
try? takesOptInts(throwingFn(), 0) // CHECK-NEXT: [[@LINE]]:33 -> [[@LINE]]:37 : (0 - 1)
533+
} // CHECK-NEXT: }
534+
535+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test34SiSgyF"
536+
func test34() -> Int? { // CHECK-NEXT: [[@LINE]]:23 -> [[@LINE+3]]:2 : 0
537+
try? takesOptInts(throwingFn(), throwingFn()) // CHECK-NEXT: [[@LINE]]:33 -> [[@LINE]]:48 : (0 - 1)
538+
// CHECK-NEXT: [[@LINE-1]]:47 -> [[@LINE-1]]:48 : ((0 - 1) - 2)
539+
} // CHECK-NEXT: }
540+
541+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test35SiSgyF"
542+
func test35() -> Int? { // CHECK-NEXT: [[@LINE]]:23 -> [[@LINE+5]]:2 : 0
543+
try? takesOptInts(
544+
throwingFn(), // CHECK-NEXT: [[@LINE]]:17 -> [[@LINE+2]]:4 : (0 - 1)
545+
throwingFn() // CHECK-NEXT: [[@LINE]]:17 -> [[@LINE+1]]:4 : ((0 - 1) - 2)
546+
)
547+
} // CHECK-NEXT: }
548+
549+
// The 'try's here are redundant.
550+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test36SiSgyF"
551+
func test36() -> Int? { // CHECK-NEXT: [[@LINE]]:23 -> [[@LINE+5]]:2 : 0
552+
try? takesOptInts(
553+
try throwingFn(), // CHECK-NEXT: [[@LINE]]:21 -> [[@LINE+2]]:4 : (0 - 1)
554+
try throwingFn() // CHECK-NEXT: [[@LINE]]:21 -> [[@LINE+1]]:4 : ((0 - 1) - 2)
555+
)
556+
} // CHECK-NEXT: }
557+
558+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test37SiSgyF"
559+
func test37() -> Int? { // CHECK-NEXT: [[@LINE]]:23 -> [[@LINE+5]]:2 : 0
560+
try? takesOptInts(
561+
try throwingFn(), // CHECK-NEXT: [[@LINE]]:21 -> [[@LINE+2]]:4 : (0 - 1)
562+
try? S[]
563+
)
564+
} // CHECK-NEXT: }
565+
566+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test38SiSgyF"
567+
func test38() -> Int? { // CHECK-NEXT: [[@LINE]]:23 -> [[@LINE+5]]:2 : 0
568+
try? takesOptInts(
569+
try? throwingFn(),
570+
try S[] // CHECK-NEXT: [[@LINE]]:12 -> [[@LINE+1]]:4 : (0 - 2)
571+
)
572+
} // CHECK-NEXT: }
573+
574+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test39Si_SitSgyF"
575+
func test39(
576+
) -> (Int, Int)? { // CHECK-NEXT: [[@LINE]]:18 -> [[@LINE+5]]:2 : 0
577+
try? (takesOptInts(
578+
throwingFn(), // CHECK-NEXT: [[@LINE]]:17 -> [[@LINE+2]]:8 : (0 - 1)
579+
try? throwingProp
580+
), 0)
581+
} // CHECK-NEXT: }
582+
583+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test40SiyKF"
584+
func test40(
585+
) throws -> Int { // CHECK-NEXT: [[@LINE]]:17 -> [[@LINE+5]]:2 : 0
586+
try takesOptInts(
587+
try? throwingFn(),
588+
throwingProp // CHECK-NEXT: [[@LINE]]:17 -> [[@LINE+2]]:2 : (0 - 2)
589+
)
590+
} // CHECK-NEXT: }
591+
592+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test41SiyKF"
593+
func test41(
594+
) throws -> Int { // CHECK-NEXT: [[@LINE]]:17 -> [[@LINE+5]]:2 : 0
595+
try takesOptInts(
596+
throwingFn(), // CHECK-NEXT: [[@LINE]]:17 -> [[@LINE+3]]:2 : (0 - 1)
597+
try? throwingFn()
598+
)
599+
} // CHECK-NEXT: }
600+
601+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test42SiSgyF"
602+
func test42() -> Int? { // CHECK-NEXT: [[@LINE]]:23 -> [[@LINE+5]]:2 : 0
603+
guard let x = try? throwingFn() else { // CHECK-NEXT: [[@LINE]]:40 -> [[@LINE+2]]:4 : 1
604+
return nil
605+
} // CHECK-NEXT: [[@LINE]]:4 -> [[@LINE+1]]:11 : (0 - 1)
606+
return x
607+
} // CHECK-NEXT: }
608+
609+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test43SiSgyF"
610+
func test43() -> Int? { // CHECK-NEXT: [[@LINE]]:23 -> [[@LINE+3]]:2 : 0
611+
let x = try? throwingS.throwingMethod() // CHECK-NEXT: [[@LINE]]:25 -> [[@LINE]]:42 : (0 - 1)
612+
return x
613+
} // CHECK-NEXT: }
614+
615+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test44SiSgyKF"
616+
func test44() throws -> Int? { // CHECK-NEXT: [[@LINE]]:30 -> [[@LINE+3]]:2 : 0
617+
let x = try (try? throwingS)?.throwingMethod() // CHECK-NEXT: [[@LINE]]:49 -> [[@LINE+1]]:11 : (0 - 2)
618+
return x
619+
}
620+
621+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test45yyF"
622+
func test45() { // CHECK-NEXT: [[@LINE]]:15 -> [[@LINE+2]]:2 : 0
623+
_ = try? { throw SomeErr.Err1 }()
624+
} // CHECK-NEXT: }
625+
626+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test45yyFyyKXEfU_"
627+
// CHECK-NEXT: [[@LINE-4]]:12 -> [[@LINE-4]]:34 : 0
628+
// CHECK-NEXT: }
629+
630+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test46Si_SitSgyF"
631+
func test46() -> (Int, Int)? { // CHECK-NEXT: [[@LINE]]:30 -> [[@LINE+2]]:2 : 0
632+
try? ({ throw SomeErr.Err1 }(), 0) // CHECK-NEXT: [[@LINE]]:33 -> [[@LINE]]:37 : (0 - 1)
633+
} // CHECK-NEXT: }
634+
492635
struct TestInit {
493636
// CHECK-LABEL: sil_coverage_map {{.*}}// coverage_errors.TestInit.init() -> coverage_errors.TestInit
494637
init() { // CHECK-NEXT: [[@LINE]]:10 -> [[@LINE+5]]:4 : 0
@@ -498,3 +641,32 @@ struct TestInit {
498641
} // CHECK-NEXT: [[@LINE]]:6 -> [[@LINE+1]]:4 : 1
499642
} // CHECK-NEXT: }
500643
}
644+
645+
struct TestProp {
646+
let a = try? throwingFn()
647+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors8TestPropV1aSiSgvpfi"
648+
// CHECK-NEXT: [[@LINE-2]]:11 -> [[@LINE-2]]:28 : 0
649+
// CHECK-NEXT: }
650+
651+
let b = try? (throwingFn(), 0)
652+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors8TestPropV1bSi_SitSgvpfi"
653+
// CHECK-NEXT: [[@LINE-2]]:11 -> [[@LINE-2]]:33 : 0
654+
// CHECK-NEXT: [[@LINE-3]]:29 -> [[@LINE-3]]:33 : (0 - 1)
655+
// CHECK-NEXT: }
656+
657+
let c = try? (throwingFn(), .random() ? 0 : 1)
658+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors8TestPropV1cSi_SitSgvpfi"
659+
// CHECK-NEXT: [[@LINE-2]]:11 -> [[@LINE-2]]:49 : 0
660+
// CHECK-NEXT: [[@LINE-3]]:29 -> [[@LINE-3]]:49 : (0 - 1)
661+
// CHECK-NEXT: [[@LINE-4]]:43 -> [[@LINE-4]]:44 : 2
662+
// CHECK-NEXT: [[@LINE-5]]:47 -> [[@LINE-5]]:48 : ((0 - 1) - 2)
663+
// CHECK-NEXT: }
664+
665+
let d = (try? (throwingFn(), .random() ? 0 : 1), 0)
666+
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors8TestPropV1dSi_SitSg_Sitvpfi"
667+
// CHECK-NEXT: [[@LINE-2]]:11 -> [[@LINE-2]]:54 : 0
668+
// CHECK-NEXT: [[@LINE-3]]:30 -> [[@LINE-3]]:50 : (0 - 1)
669+
// CHECK-NEXT: [[@LINE-4]]:44 -> [[@LINE-4]]:45 : 2
670+
// CHECK-NEXT: [[@LINE-5]]:48 -> [[@LINE-5]]:49 : ((0 - 1) - 2)
671+
// CHECK-NEXT: }
672+
}

0 commit comments

Comments
 (0)