Skip to content

Commit e07362b

Browse files
committed
[SIL Diagnostics] Create a mandatory pass to check correct usage of
yields in generalized accessors: _read and _modify, which are yield-once corountines. This pass is based on the existing SIL verifier checks but diagnoses only those errors that can be introduced by programmers when using yields. <rdar://43578476>
1 parent 2e50a43 commit e07362b

File tree

8 files changed

+632
-1
lines changed

8 files changed

+632
-1
lines changed

include/swift/AST/DiagnosticsSIL.def

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,21 @@ WARNING(warning_int_to_fp_inexact, none,
358358
"'%1' is not exactly representable as %0; it becomes '%2'",
359359
(Type, StringRef, StringRef))
360360

361+
362+
// Yield usage errors
363+
ERROR(return_before_yield, none, "accessor doesn't yield before returning",())
364+
365+
ERROR(multiple_yields, none, "encountered multiple yields along "
366+
"a single path in yield_once accessor", ())
367+
368+
NOTE(previous_yield, none, "previous yield was here", ())
369+
370+
ERROR(possible_return_before_yield, none, "cannot guarantee that the accessor "
371+
"yields a value before returning", ())
372+
373+
NOTE(conflicting_join, none,
374+
"some paths reaching here have a yield and some don't", ())
375+
361376
#ifndef DIAG_NO_UNDEF
362377
# if defined(DIAG)
363378
# undef DIAG

include/swift/SILOptimizer/PassManager/Passes.def

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,8 @@ PASS(SimplifyUnreachableContainingBlocks, "simplify-unreachable-containing-block
294294
"Utility pass. Removes all non-term insts from blocks with unreachable terms")
295295
PASS(SerializeSILPass, "serialize-sil",
296296
"Utility pass. Serializes the current SILModule")
297+
PASS(YieldOnceCheck, "yield-once-check",
298+
"Check correct usage of yields in yield-once coroutines")
297299
PASS(BugReducerTester, "bug-reducer-tester",
298300
"sil-bug-reducer Tool Testing by Asserting on a Sentinel Function")
299301
PASS_RANGE(AllPasses, AADumper, BugReducerTester)

lib/SIL/SILVerifier.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4479,7 +4479,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
44794479
/// - accesses must be uniquely ended
44804480
/// - flow-sensitive states must be equivalent on all paths into a block
44814481
void verifyFlowSensitiveRules(SILFunction *F) {
4482-
// Do a breath-first search through the basic blocks.
4482+
// Do a traversal of the basic blocks.
44834483
// Note that we intentionally don't verify these properties in blocks
44844484
// that can't be reached from the entry block.
44854485
llvm::DenseMap<SILBasicBlock*, VerifyFlowSensitiveRulesDetails::BBState> visitedBBs;

lib/SILOptimizer/Mandatory/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,4 +18,5 @@ silopt_register_sources(
1818
ClosureLifetimeFixup.cpp
1919
RawSILInstLowering.cpp
2020
MandatoryOptUtils.cpp
21+
YieldOnceCheck.cpp
2122
)
Lines changed: 228 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,228 @@
1+
//===------ YieldOnceCheck.cpp - Check usage of yields in accessors ------===//
2+
//
3+
// This source file is part of the Swift.org open source project
4+
//
5+
// Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
6+
// Licensed under Apache License v2.0 with Runtime Library Exception
7+
//
8+
// See https://swift.org/LICENSE.txt for license information
9+
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
10+
//
11+
//===----------------------------------------------------------------------===//
12+
13+
#define DEBUG_TYPE "yield-once-check"
14+
#include "swift/AST/DiagnosticsSIL.h"
15+
#include "swift/SIL/BasicBlockUtils.h"
16+
#include "swift/SILOptimizer/PassManager/Transforms.h"
17+
#include "llvm/ADT/DenseSet.h"
18+
19+
using namespace swift;
20+
21+
namespace {
22+
23+
class YieldOnceCheck : public SILFunctionTransform {
24+
25+
template <typename... T, typename... U>
26+
static InFlightDiagnostic diagnose(ASTContext &Context, SourceLoc loc,
27+
Diag<T...> diag, U &&... args) {
28+
return Context.Diags.diagnose(loc, diag, std::forward<U>(args)...);
29+
}
30+
31+
/// A state that is associated with basic blocks to record whether a basic
32+
/// block appears before a yield or after a yield, or could be in either
33+
/// state, which is denoted by 'Conflict'.
34+
enum YieldState { BeforeYield, AfterYield, Conflict };
35+
36+
struct BBState {
37+
YieldState yieldState = BeforeYield;
38+
39+
private:
40+
// For AfterYield and Conflict states, track the yield instruction.
41+
llvm::Optional<SILInstruction *> yieldInst = None;
42+
// For Conflict state, track the first instruction that is found to be in
43+
// conflict state. This should be the first instruction of a join node in
44+
// the CFG.
45+
llvm::Optional<SILInstruction *> conflictInst = None;
46+
47+
public:
48+
void updateToAfterYield(SILInstruction *yldInst) {
49+
assert(yieldState == BeforeYield);
50+
assert(yldInst != nullptr);
51+
52+
yieldState = AfterYield;
53+
yieldInst = yldInst;
54+
}
55+
56+
void updateToConflict(SILInstruction *yldInst, SILInstruction *confInst) {
57+
assert(yieldState != Conflict);
58+
assert(yldInst != nullptr && confInst != nullptr);
59+
60+
yieldState = Conflict;
61+
yieldInst = yldInst;
62+
conflictInst = confInst;
63+
}
64+
65+
SILInstruction *getYieldInstruction() const {
66+
assert(yieldState == AfterYield || yieldState == Conflict);
67+
68+
return yieldInst.getValue();
69+
}
70+
71+
SILInstruction *getConflictInstruction() const {
72+
assert(yieldState == Conflict);
73+
74+
return conflictInst.getValue();
75+
}
76+
};
77+
78+
/// Checks whether there is exactly one yield before a return in every path
79+
/// in the control-flow graph.
80+
/// Diagnostics are not reported for nodes unreachable from the entry, and
81+
/// also for nodes that may not reach the exit of the control-flow graph.
82+
void diagnoseYieldOnceUsage(SILFunction &fun) {
83+
// Do a traversal of the basic blocks starting from the entry node
84+
// in a breadth-first fashion. The traversal order is not important for
85+
// correctness, but it could change the errors diagnosed when there are
86+
// multiple errors. Breadth-first search could diagnose errors along
87+
// shorter paths.
88+
llvm::DenseMap<SILBasicBlock *, BBState> visitedBBs;
89+
SmallVector<SILBasicBlock *, 16> worklist;
90+
91+
auto *entryBB = &(*fun.begin());
92+
visitedBBs.try_emplace(entryBB);
93+
worklist.push_back(entryBB);
94+
95+
// Track that last return instruction seen by the analysis, if any, before
96+
// encountering a yield.
97+
// This is needed for emitting diagnostics when there is no yield
98+
// instruction along any path reaching a return instruction.
99+
llvm::Optional<SILInstruction *> returnInst = None;
100+
ASTContext &astCtx = fun.getModule().getASTContext();
101+
102+
while (!worklist.empty()) {
103+
SILBasicBlock *bb = worklist.pop_back_val();
104+
BBState state = visitedBBs[bb];
105+
106+
auto *term = bb->getTerminator();
107+
if (isa<ReturnInst>(term)) {
108+
// A conflict state implies that there is a path to return before
109+
// seeing a yield.
110+
if (state.yieldState == YieldState::Conflict) {
111+
diagnose(astCtx, term->getLoc().getSourceLoc(),
112+
diag::possible_return_before_yield);
113+
// Add a note to the instruction where the conflict first appeared.
114+
diagnose(astCtx,
115+
state.getConflictInstruction()->getLoc().getSourceLoc(),
116+
diag::conflicting_join);
117+
return;
118+
}
119+
// If the state is BeforeYield, it is an error. But, defer emitting
120+
// diagnostics until we see a Conflict state or have visited all nodes
121+
// in order to gather more information.
122+
if (state.yieldState != YieldState::AfterYield) {
123+
returnInst = term;
124+
}
125+
continue;
126+
}
127+
128+
// Check whether there are multiple yields.
129+
if (isa<YieldInst>(term)) {
130+
if (state.yieldState != YieldState::BeforeYield) {
131+
diagnose(astCtx, term->getLoc().getSourceLoc(),
132+
diag::multiple_yields);
133+
// Add a note that points to the previous yield.
134+
diagnose(astCtx, state.getYieldInstruction()->getLoc().getSourceLoc(),
135+
diag::previous_yield);
136+
return;
137+
}
138+
}
139+
140+
for (auto &succ : term->getSuccessors()) {
141+
SILBasicBlock *succBB = succ.getBB();
142+
143+
// Optimistically try to set our current state as the state
144+
// of the successor.
145+
auto insertResult = visitedBBs.try_emplace(succBB, state);
146+
147+
// If the insertion was successful, it means we are seeing the successor
148+
// for the first time. Propogate the state and add the successor to the
149+
// worklist.
150+
if (insertResult.second) {
151+
worklist.insert(worklist.begin(), succBB);
152+
153+
// When the successor follows a 'yield', update the successor state.
154+
if (isa<YieldInst>(term)) {
155+
insertResult.first->second.updateToAfterYield(term);
156+
}
157+
continue;
158+
}
159+
160+
const auto &succState = insertResult.first->second;
161+
if (succState.yieldState == Conflict) {
162+
// The successor is already in the error state, so we need
163+
// not propagate anything. Diagnostics will be reported, where
164+
// appropriate, when the successor is removed from the worklist.
165+
continue;
166+
}
167+
168+
// If the successor state is not a conflict but the current state is,
169+
// propagate the conflict down to the successor.
170+
// (This is needed to identify the conflicting yields and
171+
// present good diagnostics.)
172+
if (state.yieldState == Conflict) {
173+
worklist.insert(worklist.begin(), succBB);
174+
insertResult.first->second = state;
175+
continue;
176+
}
177+
178+
// Here, neither 'state' nor 'succState' is equal to 'Conflict'.
179+
180+
if (state.yieldState != succState.yieldState) {
181+
// We have found that the successor can appear before and
182+
// also after a yield. Therefore, set the state as a conflict and
183+
// propagate it to its successors to emit diagnostics.
184+
// (Note that the successor must be a join node in the control graph
185+
// for this scenario to happen.)
186+
auto *yieldInst = (state.yieldState == YieldState::AfterYield)
187+
? state.getYieldInstruction()
188+
: succState.getYieldInstruction();
189+
insertResult.first->second.updateToConflict(yieldInst,
190+
&*(succBB->begin()));
191+
worklist.insert(worklist.begin(), succBB);
192+
193+
continue;
194+
// Even though at this point we know there has to be an error as
195+
// there is an inconsistent state, we cannot stop here as we do not
196+
// know for sure whether the error will result in multiple yields
197+
// or a return before a yield.
198+
}
199+
}
200+
}
201+
202+
if (returnInst.hasValue()) {
203+
// Here, we haven't seen a yield along any path that leads to this return.
204+
// Otherwise, the analysis must have propagated a conflict state to this
205+
// return instruction, which must have been diagnosed earlier.
206+
diagnose(astCtx, returnInst.getValue()->getLoc().getSourceLoc(),
207+
diag::return_before_yield);
208+
return;
209+
}
210+
}
211+
212+
/// The entry point to the transformation.
213+
void run() override {
214+
auto *fun = getFunction();
215+
216+
if (fun->getLoweredFunctionType()->getCoroutineKind() !=
217+
SILCoroutineKind::YieldOnce)
218+
return;
219+
220+
diagnoseYieldOnceUsage(*fun);
221+
}
222+
};
223+
224+
} // end anonymous namespace
225+
226+
SILTransform *swift::createYieldOnceCheck() {
227+
return new YieldOnceCheck();
228+
}

lib/SILOptimizer/PassManager/PassPipeline.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ static void addMandatoryOptPipeline(SILPassPipelinePlan &P,
108108
P.addGuaranteedARCOpts();
109109
P.addDiagnoseUnreachable();
110110
P.addDiagnoseInfiniteRecursion();
111+
P.addYieldOnceCheck();
111112
P.addEmitDFDiagnostics();
112113
// Canonical swift requires all non cond_br critical edges to be split.
113114
P.addSplitNonCondBrCriticalEdges();
Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
// RUN: %target-sil-opt %s -yield-once-check -o /dev/null -verify
2+
//
3+
// SIL-level tests for yield-once diagnostics emitted for yield-once coroutines.
4+
// Note that the yield-once check runs on all functions with yield_once
5+
// qualifier in their type, regardless of whether or not they are
6+
// accessors of a property. In fact, currently it is not possible to parse
7+
// a property with _read and _modify accessors from a SIL source.
8+
9+
sil_stage raw
10+
11+
import Builtin
12+
import Swift
13+
14+
sil @testNoYield : $@yield_once @convention(thin) () -> @yields Int {
15+
bb0:
16+
%2 = tuple ()
17+
return %2 : $() // expected-error {{accessor doesn't yield before returning}}
18+
}
19+
20+
struct TestYield {
21+
var flag : Builtin.Int1
22+
var stored: Int
23+
}
24+
25+
sil @testMultipleYields : $@yield_once @convention(thin) (TestYield) -> @yields Int {
26+
bb0(%0 : $TestYield):
27+
%1 = struct_extract %0 : $TestYield, #TestYield.flag
28+
cond_br %1, bb1, bb3
29+
30+
bb1:
31+
%5 = struct_extract %0 : $TestYield, #TestYield.stored
32+
yield %5 : $Int, resume bb2, unwind bb6 // expected-note {{previous yield was here}}
33+
34+
bb2:
35+
br bb4
36+
37+
bb3:
38+
br bb4
39+
40+
bb4:
41+
%9 = struct_extract %0 : $TestYield, #TestYield.stored
42+
yield %9 : $Int, resume bb5, unwind bb7 // expected-error {{encountered multiple yields along a single path in yield_once accessor}}
43+
44+
bb5:
45+
%11 = tuple ()
46+
return %11 : $()
47+
48+
bb6:
49+
br bb8
50+
51+
bb7:
52+
br bb8
53+
54+
bb8:
55+
unwind
56+
}
57+
58+
sil @testPartialReturnWithoutYield : $@yield_once @convention(thin) (TestYield) -> @yields Int {
59+
bb0(%0 : $TestYield):
60+
%1 = struct_extract %0 : $TestYield, #TestYield.flag
61+
cond_br %1, bb1, bb3
62+
63+
bb1:
64+
%5 = struct_extract %0 : $TestYield, #TestYield.stored
65+
yield %5 : $Int, resume bb2, unwind bb5
66+
67+
bb2:
68+
br bb4
69+
70+
bb3:
71+
br bb4
72+
73+
bb4:
74+
%21 = tuple () // expected-note {{some paths reaching here have a yield and some don't}}
75+
return %21 : $() // expected-error {{cannot guarantee that the accessor yields a value before returning}}
76+
77+
bb5:
78+
unwind
79+
}
80+
81+
sil @testExplicitReturns : $@yield_once @convention(thin) (TestYield) -> @yields Int {
82+
bb0(%0 : $TestYield):
83+
%2 = struct_extract %0 : $TestYield, #TestYield.stored
84+
%3 = integer_literal $Builtin.Int64, 0
85+
%4 = struct_extract %2 : $Int, #Int._value
86+
%5 = builtin "cmp_slt_Int64"(%3 : $Builtin.Int64, %4 : $Builtin.Int64) : $Builtin.Int1
87+
%6 = struct $Bool (%5 : $Builtin.Int1)
88+
%7 = struct_extract %6 : $Bool, #Bool._value
89+
cond_br %7, bb1, bb2
90+
91+
bb1:
92+
br bb4
93+
94+
bb2:
95+
%10 = struct_extract %0 : $TestYield, #TestYield.stored
96+
yield %10 : $Int, resume bb3, unwind bb5
97+
98+
bb3:
99+
br bb4
100+
101+
bb4:
102+
%13 = tuple () // expected-note {{some paths reaching here have a yield and some don't}}
103+
return %13 : $() // expected-error {{cannot guarantee that the accessor yields a value before returning}}
104+
105+
bb5:
106+
unwind
107+
}

0 commit comments

Comments
 (0)