Skip to content

Commit 7c721c0

Browse files
hamishknighthamishknight
authored
Merge pull request #60643 from hamishknight/new-coverage-just-dropped
2 parents df66f27 + 6a6ba83 commit 7c721c0

File tree

11 files changed

+177
-47
lines changed

11 files changed

+177
-47
lines changed

‎include/swift/AST/ASTWalker.h

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,25 @@ struct ReferenceMetaData {
5050
: Kind(Kind), AccKind(AccKind), isImplicit(isImplicit) {}
5151
};
5252

53+
/// Specifies how the initialization expression of a \c lazy variable should be
54+
/// walked by the ASTWalker.
55+
enum class LazyInitializerWalking {
56+
/// No lazy initialization expressions will be walked.
57+
None,
58+
59+
/// The lazy initialization expression will only be walked as a part of
60+
/// the variable's pattern binding decl. This is the default behavior, and is
61+
/// consistent with the initializer being syntactically part of the pattern
62+
/// binding.
63+
InPatternBinding,
64+
65+
/// The lazy initialization expression will only be walked as part of the
66+
/// body of the synthesized accessor for the lazy variable. In such an
67+
/// accessor, the expression is denoted by LazyInitializerExpr. This is mainly
68+
/// useful for code emission.
69+
InAccessor
70+
};
71+
5372
/// An abstract class used to traverse an AST.
5473
class ASTWalker {
5574
public:
@@ -193,15 +212,13 @@ class ASTWalker {
193212
/// params in AbstractFunctionDecl and NominalTypeDecl.
194213
virtual bool shouldWalkIntoGenericParams() { return false; }
195214

196-
/// This method configures whether the walker should walk into the
197-
/// initializers of lazy variables. These initializers are semantically
198-
/// different from other initializers in their context and so sometimes
199-
/// should not be visited.
200-
///
201-
/// Note that visiting the body of the lazy getter will find a
202-
/// LazyInitializerExpr with the initializer as its sub-expression.
203-
/// However, ASTWalker does not walk into LazyInitializerExprs on its own.
204-
virtual bool shouldWalkIntoLazyInitializers() { return true; }
215+
/// This method configures how the walker should walk the initializers of
216+
/// lazy variables. These initializers are semantically different from other
217+
/// initializers in their context and so sometimes should be visited as part
218+
/// of the synthesized getter, or should not be visited at all.
219+
virtual LazyInitializerWalking getLazyInitializerWalkingBehavior() {
220+
return LazyInitializerWalking::InPatternBinding;
221+
}
205222

206223
/// This method configures whether the walker should visit the body of a
207224
/// closure that was checked separately from its enclosing expression.

‎include/swift/AST/Expr.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5314,6 +5314,7 @@ class LazyInitializerExpr : public Expr {
53145314
SourceLoc getLoc() const { return SubExpr->getLoc(); }
53155315

53165316
Expr *getSubExpr() const { return SubExpr; }
5317+
void setSubExpr(Expr *subExpr) { SubExpr = subExpr; }
53175318

53185319
static bool classof(const Expr *E) {
53195320
return E->getKind() == ExprKind::LazyInitializer;

‎lib/AST/ASTVerifier.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2338,11 +2338,11 @@ class Verifier : public ASTWalker {
23382338
verifyCheckedBase(VD);
23392339
}
23402340

2341-
bool shouldWalkIntoLazyInitializers() override {
2341+
LazyInitializerWalking getLazyInitializerWalkingBehavior() override {
23422342
// We don't want to walk into lazy initializers because they should
23432343
// have been reparented to their synthesized getter, which will
23442344
// invalidate various invariants.
2345-
return false;
2345+
return LazyInitializerWalking::None;
23462346
}
23472347

23482348
void verifyChecked(PatternBindingDecl *binding) {

‎lib/AST/ASTWalker.cpp

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -195,18 +195,23 @@ class Traversal : public ASTVisitor<Traversal, Expr*, Stmt*,
195195
PBD->setPattern(idx, Pat, PBD->getInitContext(idx));
196196
else
197197
return true;
198-
if (PBD->getInit(idx) &&
199-
!isPropertyWrapperBackingProperty &&
200-
(!PBD->isInitializerSubsumed(idx) ||
201-
Walker.shouldWalkIntoLazyInitializers())) {
198+
199+
if (!PBD->getInit(idx) || isPropertyWrapperBackingProperty)
200+
continue;
201+
202+
if (PBD->isInitializerSubsumed(idx) &&
203+
Walker.getLazyInitializerWalkingBehavior() !=
204+
LazyInitializerWalking::InPatternBinding) {
205+
break;
206+
}
207+
202208
#ifndef NDEBUG
203-
PrettyStackTraceDecl debugStack("walking into initializer for", PBD);
209+
PrettyStackTraceDecl debugStack("walking into initializer for", PBD);
204210
#endif
205-
if (Expr *E2 = doIt(PBD->getInit(idx)))
206-
PBD->setInit(idx, E2);
207-
else
208-
return true;
209-
}
211+
if (Expr *E2 = doIt(PBD->getInit(idx)))
212+
PBD->setInit(idx, E2);
213+
else
214+
return true;
210215
}
211216
return false;
212217
}
@@ -1073,7 +1078,17 @@ class Traversal : public ASTVisitor<Traversal, Expr*, Stmt*,
10731078
}
10741079

10751080
Expr *visitLazyInitializerExpr(LazyInitializerExpr *E) {
1076-
// Initializer is totally opaque for most visitation purposes.
1081+
// The initializer is opaque unless we specifically want to visit it as part
1082+
// of the accessor body.
1083+
if (Walker.getLazyInitializerWalkingBehavior() !=
1084+
LazyInitializerWalking::InAccessor) {
1085+
return E;
1086+
}
1087+
auto *sub = doIt(E->getSubExpr());
1088+
if (!sub)
1089+
return nullptr;
1090+
1091+
E->setSubExpr(sub);
10771092
return E;
10781093
}
10791094

‎lib/SIL/IR/SILProfiler.cpp

Lines changed: 36 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,13 @@ static bool skipExpr(Expr *E) {
196196
return !E->getStartLoc().isValid() || !E->getEndLoc().isValid();
197197
}
198198

199+
/// Whether the children of an unmapped decl should still be walked.
200+
static bool shouldWalkUnmappedDecl(const Decl *D) {
201+
// We want to walk into the initializer for a pattern binding decl. This
202+
// allows us to map LazyInitializerExprs.
203+
return isa<PatternBindingDecl>(D);
204+
}
205+
199206
/// An ASTWalker that maps ASTNodes to profiling counters.
200207
struct MapRegionCounters : public ASTWalker {
201208
/// The next counter value to assign.
@@ -207,6 +214,12 @@ struct MapRegionCounters : public ASTWalker {
207214
MapRegionCounters(llvm::DenseMap<ASTNode, unsigned> &CounterMap)
208215
: CounterMap(CounterMap) {}
209216

217+
LazyInitializerWalking getLazyInitializerWalkingBehavior() override {
218+
// We want to walk lazy initializers present in the synthesized getter for
219+
// a lazy variable.
220+
return LazyInitializerWalking::InAccessor;
221+
}
222+
210223
void mapRegion(ASTNode N) {
211224
CounterMap[N] = NextCounter;
212225

@@ -225,7 +238,7 @@ struct MapRegionCounters : public ASTWalker {
225238

226239
bool walkToDeclPre(Decl *D) override {
227240
if (isUnmapped(D))
228-
return false;
241+
return shouldWalkUnmappedDecl(D);
229242

230243
if (auto *AFD = dyn_cast<AbstractFunctionDecl>(D)) {
231244
return visitFunctionDecl(*this, AFD, [&] { mapRegion(AFD->getBody()); });
@@ -274,14 +287,8 @@ struct MapRegionCounters : public ASTWalker {
274287
mapRegion(IE->getThenExpr());
275288
}
276289

277-
// rdar://42792053
278-
// TODO: There's an outstanding issue here with LazyInitializerExpr. A LIE
279-
// is copied into the body of a property getter after type-checking (before
280-
// coverage). ASTWalker only visits this expression once via the property's
281-
// VarDecl, and does not visit it again within the getter. This results in
282-
// missing coverage. SILGen treats the init expr as part of the getter, but
283-
// its SILProfiler has no information about the init because the LIE isn't
284-
// visited here.
290+
if (isa<LazyInitializerExpr>(E))
291+
mapRegion(E);
285292

286293
return {true, E};
287294
}
@@ -579,7 +586,7 @@ struct PGOMapping : public ASTWalker {
579586

580587
bool walkToDeclPre(Decl *D) override {
581588
if (isUnmapped(D))
582-
return false;
589+
return shouldWalkUnmappedDecl(D);
583590
if (auto *AFD = dyn_cast<AbstractFunctionDecl>(D)) {
584591
return visitFunctionDecl(*this, AFD, [&] {
585592
setKnownExecutionCount(AFD->getBody());
@@ -591,6 +598,12 @@ struct PGOMapping : public ASTWalker {
591598
return true;
592599
}
593600

601+
LazyInitializerWalking getLazyInitializerWalkingBehavior() override {
602+
// We want to walk lazy initializers present in the synthesized getter for
603+
// a lazy variable.
604+
return LazyInitializerWalking::InAccessor;
605+
}
606+
594607
std::pair<bool, Stmt *> walkToStmtPre(Stmt *S) override {
595608
unsigned parent = getParentCounter();
596609
auto parentCount = LoadedCounts.Counts[parent];
@@ -674,6 +687,9 @@ struct PGOMapping : public ASTWalker {
674687
}
675688
setExecutionCount(elseExpr, subtract(count, thenCount));
676689
}
690+
if (isa<LazyInitializerExpr>(E))
691+
setKnownExecutionCount(E);
692+
677693
return {true, E};
678694
}
679695
};
@@ -903,6 +919,12 @@ struct CoverageMapping : public ASTWalker {
903919
public:
904920
CoverageMapping(const SourceManager &SM) : SM(SM) {}
905921

922+
LazyInitializerWalking getLazyInitializerWalkingBehavior() override {
923+
// We want to walk lazy initializers present in the synthesized getter for
924+
// a lazy variable.
925+
return LazyInitializerWalking::InAccessor;
926+
}
927+
906928
/// Generate the coverage counter mapping regions from collected
907929
/// source regions.
908930
SILCoverageMap *emitSourceRegions(
@@ -930,7 +952,7 @@ struct CoverageMapping : public ASTWalker {
930952

931953
bool walkToDeclPre(Decl *D) override {
932954
if (isUnmapped(D))
933-
return false;
955+
return shouldWalkUnmappedDecl(D);
934956

935957
if (auto *AFD = dyn_cast<AbstractFunctionDecl>(D)) {
936958
return visitFunctionDecl(*this, AFD, [&] {
@@ -1124,6 +1146,9 @@ struct CoverageMapping : public ASTWalker {
11241146
}
11251147
}
11261148

1149+
if (isa<LazyInitializerExpr>(E))
1150+
assignCounter(E);
1151+
11271152
if (hasCounter(E) && !Parent.isNull())
11281153
pushRegion(E);
11291154
return {true, E};

‎lib/SILGen/SILGenExpr.cpp

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -415,7 +415,9 @@ namespace {
415415
return RValue(SGF, E, SGF.emitAddressOfLValue(E->getSubExpr(),
416416
std::move(lv)));
417417
}
418-
418+
419+
RValue visitLazyInitializerExpr(LazyInitializerExpr *E, SGFContext C);
420+
419421
RValue visitApplyExpr(ApplyExpr *E, SGFContext C);
420422

421423
RValue visitDiscardAssignmentExpr(DiscardAssignmentExpr *E, SGFContext C) {
@@ -708,6 +710,15 @@ tryEmitAsBridgingConversion(SILGenFunction &SGF, Expr *E, bool isExplicit,
708710
return SGF.emitConvertedRValue(subExpr, conversion, C);
709711
}
710712

713+
RValue RValueEmitter::visitLazyInitializerExpr(LazyInitializerExpr *E,
714+
SGFContext C) {
715+
// We need to emit a profiler count increment specifically for the lazy
716+
// initialization, as we don't want to record an increment for every call to
717+
// the getter.
718+
SGF.emitProfilerIncrement(E);
719+
return visit(E->getSubExpr(), C);
720+
}
721+
711722
RValue RValueEmitter::visitApplyExpr(ApplyExpr *E, SGFContext C) {
712723
return SGF.emitApplyExpr(E, C);
713724
}

‎lib/SILGen/SILGenFunction.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1163,9 +1163,8 @@ void SILGenFunction::emitProfilerIncrement(ASTNode N) {
11631163
const auto &RegionCounterMap = SP->getRegionCounterMap();
11641164
auto CounterIt = RegionCounterMap.find(N);
11651165

1166-
// TODO: Assert that this cannot happen (rdar://42792053).
1167-
if (CounterIt == RegionCounterMap.end())
1168-
return;
1166+
assert(CounterIt != RegionCounterMap.end() &&
1167+
"cannot increment non-existent counter");
11691168

11701169
auto Int32Ty = getLoweredType(BuiltinIntegerType::get(32, C));
11711170
auto Int64Ty = getLoweredType(BuiltinIntegerType::get(64, C));

‎lib/Sema/TypeCheckCaptures.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -210,11 +210,11 @@ class FindCapturedVars : public ASTWalker {
210210
checkType(VD->getInterfaceType(), VD->getLoc());
211211
}
212212

213-
bool shouldWalkIntoLazyInitializers() override {
213+
LazyInitializerWalking getLazyInitializerWalkingBehavior() override {
214214
// We don't want to walk into lazy initializers because they're not
215215
// really present at this level. We'll catch them when processing
216216
// the getter.
217-
return false;
217+
return LazyInitializerWalking::None;
218218
}
219219

220220
std::pair<bool, Expr *> walkToDeclRefExpr(DeclRefExpr *DRE) {

‎test/Profiler/coverage_class.swift

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -52,11 +52,3 @@ struct S2 {
5252
// CHECK-NEXT: [[@LINE+1]]:17 -> [[@LINE+1]]:27 : 0
5353
var m1: Int = g1 ? 0 : 1
5454
}
55-
56-
// Test that the crash from SR-8429 is avoided. Follow-up work is
57-
// needed to generate the correct coverage mapping here. Coverage for
58-
// `offset` should be associated with its getter, not the class
59-
// constructor.
60-
class C2 {
61-
lazy var offset: Int = true ? 30 : 55
62-
}

‎test/Profiler/coverage_lazy.swift

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
// RUN: %target-swift-frontend -Xllvm -sil-full-demangle -profile-generate -profile-coverage-mapping -emit-sil -module-name coverage_lazy %s | %FileCheck %s
2+
// RUN: %target-swift-frontend -profile-generate -profile-coverage-mapping -emit-ir %s
3+
4+
// Test that the crash from SR-8429 is avoided, and that we generate the
5+
// correct coverage.
6+
class C {
7+
// CHECK-LABEL: sil hidden [lazy_getter] [noinline] @$s13coverage_lazy1CC6offsetSivg : $@convention(method) (@guaranteed C) -> Int
8+
// CHECK: switch_enum {{%[0-9]+}} : $Optional<Int>, case #Optional.some!enumelt: {{bb[0-9]}}, case #Optional.none!enumelt: [[INITBB:bb[0-9]]]
9+
// CHECK: [[INITBB]]
10+
// CHECK-NEXT: string_literal
11+
// CHECK-NEXT: integer_literal $Builtin.Int64, 0
12+
// CHECK-NEXT: integer_literal $Builtin.Int32, 4
13+
// CHECK-NEXT: integer_literal $Builtin.Int32, 2
14+
// CHECK-NEXT: int_instrprof_increment
15+
// CHECK: function_ref @$sSb6randomSbyFZ : $@convention(method) (@thin Bool.Type) -> Bool
16+
// CHECK: cond_br {{%[0-9]+}}, [[TRUEBB:bb[0-9]]], {{bb[0-9]}}
17+
// CHECK: [[TRUEBB]]
18+
// CHECK-NEXT: string_literal
19+
// CHECK-NEXT: integer_literal $Builtin.Int64, 0
20+
// CHECK-NEXT: integer_literal $Builtin.Int32, 4
21+
// CHECK-NEXT: integer_literal $Builtin.Int32, 3
22+
23+
// CHECK-LABEL: sil_coverage_map {{.*}} // coverage_lazy.C.offset.getter : Swift.Int
24+
// CHECK-NEXT: [[@LINE+4]]:38 -> [[@LINE+4]]:40 : 3
25+
// CHECK-NEXT: [[@LINE+3]]:43 -> [[@LINE+3]]:45 : (2 - 3)
26+
// CHECK-NEXT: [[@LINE+2]]:26 -> [[@LINE+2]]:45 : 2
27+
// CHECK-NEXT: }
28+
lazy var offset: Int = .random() ? 30 : 55
29+
}

0 commit comments

Comments
 (0)