Skip to content

Commit d6599c4

Browse files
authored
Merge pull request swiftlang#37915 from atrick/fix-weakwarn
Fix spurious weak lifetime diagnostic.
2 parents cfd6913 + 4a80638 commit d6599c4

File tree

5 files changed

+209
-28
lines changed

5 files changed

+209
-28
lines changed

include/swift/SIL/SILValue.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -904,6 +904,9 @@ inline bool canAcceptUnownedValue(OperandOwnership operandOwnership) {
904904
llvm_unreachable("covered switch");
905905
}
906906

907+
/// Return true if all OperandOwnership invariants hold.
908+
bool checkOperandOwnershipInvariants(const Operand *operand);
909+
907910
/// Return the OperandOwnership for a forwarded operand when the forwarding
908911
/// operation has this "forwarding ownership" (as returned by
909912
/// getForwardingOwnershipKind()). \p allowUnowned is true for a subset of

lib/SIL/IR/OperandOwnership.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,16 @@
1919

2020
using namespace swift;
2121

22+
/// Return true if all OperandOwnership invariants hold.
23+
bool swift::checkOperandOwnershipInvariants(const Operand *operand) {
24+
OperandOwnership opOwnership = operand->getOperandOwnership();
25+
if (opOwnership == OperandOwnership::Borrow) {
26+
// Must be a valid BorrowingOperand.
27+
return bool(BorrowingOperand::get(operand));
28+
}
29+
return true;
30+
}
31+
2232
//===----------------------------------------------------------------------===//
2333
// OperandOwnershipClassifier
2434
//===----------------------------------------------------------------------===//

lib/SIL/Verifier/SILOwnershipVerifier.cpp

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -735,21 +735,31 @@ void SILInstruction::verifyOperandOwnership() const {
735735
if (isTypeDependentOperand(op))
736736
continue;
737737

738-
if (op.satisfiesConstraints())
739-
continue;
738+
if (!checkOperandOwnershipInvariants(&op)) {
739+
errorBuilder->handleMalformedSIL([&] {
740+
llvm::errs() << "Found an operand with invalid invariants.\n";
741+
llvm::errs() << "Value: " << op.get();
742+
llvm::errs() << "Instruction:\n";
743+
printInContext(llvm::errs());
744+
llvm::errs() << "OperandOwnership: " << op.getOperandOwnership()
745+
<< "\n";
746+
});
747+
}
740748

741-
auto constraint = op.getOwnershipConstraint();
742-
SILValue opValue = op.get();
743-
auto valueOwnershipKind = opValue.getOwnershipKind();
744-
errorBuilder->handleMalformedSIL([&] {
745-
llvm::errs() << "Found an operand with a value that is not compatible "
746-
"with the operand's operand ownership kind map.\n";
747-
llvm::errs() << "Value: " << opValue;
748-
llvm::errs() << "Value Ownership Kind: " << valueOwnershipKind << "\n";
749-
llvm::errs() << "Instruction:\n";
750-
printInContext(llvm::errs());
751-
llvm::errs() << "Constraint: " << constraint << "\n";
752-
});
749+
if (!op.satisfiesConstraints()) {
750+
auto constraint = op.getOwnershipConstraint();
751+
SILValue opValue = op.get();
752+
auto valueOwnershipKind = opValue.getOwnershipKind();
753+
errorBuilder->handleMalformedSIL([&] {
754+
llvm::errs() << "Found an operand with a value that is not compatible "
755+
"with the operand's operand ownership kind map.\n";
756+
llvm::errs() << "Value: " << opValue;
757+
llvm::errs() << "Value Ownership Kind: " << valueOwnershipKind << "\n";
758+
llvm::errs() << "Instruction:\n";
759+
printInContext(llvm::errs());
760+
llvm::errs() << "Constraint: " << constraint << "\n";
761+
});
762+
}
753763
}
754764
}
755765

lib/SILOptimizer/Mandatory/DiagnoseLifetimeIssues.cpp

Lines changed: 51 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -31,14 +31,15 @@
3131

3232
#define DEBUG_TYPE "diagnose-lifetime-issues"
3333
#include "swift/AST/DiagnosticsSIL.h"
34+
#include "swift/Demangling/Demangler.h"
3435
#include "swift/SIL/ApplySite.h"
3536
#include "swift/SIL/BasicBlockBits.h"
37+
#include "swift/SIL/OwnershipUtils.h"
3638
#include "swift/SILOptimizer/PassManager/Transforms.h"
3739
#include "swift/SILOptimizer/Utils/PrunedLiveness.h"
38-
#include "swift/Demangling/Demangler.h"
40+
#include "clang/AST/DeclObjC.h"
3941
#include "llvm/ADT/DenseMap.h"
4042
#include "llvm/Support/Debug.h"
41-
#include "clang/AST/DeclObjC.h"
4243

4344
using namespace swift;
4445

@@ -156,19 +157,24 @@ static bool isStoreObjcWeak(SILInstruction *inst, Operand *op) {
156157
/// Returns the state of \p def. See DiagnoseLifetimeIssues::State.
157158
DiagnoseLifetimeIssues::State DiagnoseLifetimeIssues::
158159
visitUses(SILValue def, bool updateLivenessAndWeakStores, int callDepth) {
159-
SmallSetVector<SILValue, 32> defUseWorklist;
160-
defUseWorklist.insert(def);
160+
SmallPtrSet<SILValue, 32> defUseVisited;
161+
SmallVector<SILValue, 32> defUseVector;
162+
auto pushDef = [&](SILValue value) {
163+
if (defUseVisited.insert(value).second)
164+
defUseVector.push_back(value);
165+
};
166+
167+
pushDef(def);
161168
bool foundWeakStore = false;
162-
while (!defUseWorklist.empty()) {
163-
SILValue value = defUseWorklist.pop_back_val();
169+
while (!defUseVector.empty()) {
170+
SILValue value = defUseVector.pop_back_val();
164171
for (Operand *use : value->getUses()) {
165172
auto *user = use->getUser();
166173

167174
// Recurse through copies and enums. Enums are important because the
168175
// operand of a store_weak is always an Optional.
169-
if (isa<CopyValueInst>(user) || isa<EnumInst>(user) ||
170-
isa<InitExistentialRefInst>(user)) {
171-
defUseWorklist.insert(cast<SingleValueInstruction>(user));
176+
if (isa<CopyValueInst>(user)) {
177+
pushDef(cast<SingleValueInstruction>(user));
172178
continue;
173179
}
174180
if (isa<StoreWeakInst>(user) || isStoreObjcWeak(user, use)) {
@@ -208,22 +214,53 @@ visitUses(SILValue def, bool updateLivenessAndWeakStores, int callDepth) {
208214
if (updateLivenessAndWeakStores)
209215
liveness.updateForUse(user, /*lifetimeEnding*/ false);
210216
break;
217+
case OperandOwnership::ForwardingBorrow:
211218
case OperandOwnership::ForwardingConsume:
212-
// TODO: handle forwarding instructions, e.g. casts.
213-
return CanEscape;
219+
// TermInst includes ReturnInst, which is generally an escape.
220+
// If this is called as part of getArgumentState, then it is not really
221+
// an escape, but we don't currently follow returned values.
222+
if (isa<TermInst>(user))
223+
return CanEscape;
224+
225+
for (SILValue result : user->getResults()) {
226+
// This assumes that forwarding to a trivial value cannot extend the
227+
// lifetime. This way, simply projecting a trivial value out of an
228+
// aggregate isn't considered an escape.
229+
if (result.getOwnershipKind() == OwnershipKind::None)
230+
continue;
231+
232+
pushDef(result);
233+
}
234+
continue;
214235
case OperandOwnership::DestroyingConsume:
215236
// destroy_value does not force pruned liveness (but store etc. does).
216237
if (!isa<DestroyValueInst>(user))
217238
return CanEscape;
218239
break;
219-
case OperandOwnership::Borrow:
240+
case OperandOwnership::Borrow: {
220241
if (updateLivenessAndWeakStores &&
221242
!liveness.updateForBorrowingOperand(use))
222243
return CanEscape;
244+
BorrowingOperand borrowOper(use);
245+
if (borrowOper.hasBorrowIntroducingUser()) {
246+
if (auto *beginBorrow = dyn_cast<BeginBorrowInst>(user))
247+
pushDef(beginBorrow);
248+
else
249+
return CanEscape;
250+
}
223251
break;
224-
case OperandOwnership::InteriorPointer:
225-
case OperandOwnership::ForwardingBorrow:
252+
}
226253
case OperandOwnership::EndBorrow:
254+
continue;
255+
256+
case OperandOwnership::InteriorPointer:
257+
// Treat most interior pointers as escapes until they can be audited.
258+
// But if the interior pointer cannot be used to copy the parent
259+
// reference, then it does not need to be considered an escape.
260+
if (isa<RefElementAddrInst>(user)) {
261+
continue;
262+
}
263+
return CanEscape;
227264
case OperandOwnership::Reborrow:
228265
return CanEscape;
229266
}
Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
// RUN: %target-sil-opt -enable-sil-verify-all %s -diagnose-lifetime-issues -o /dev/null -verify
2+
3+
import Builtin
4+
import Swift
5+
6+
enum FakeOptional<T> {
7+
case none
8+
case some(T)
9+
}
10+
11+
class Delegate {
12+
func foo() { }
13+
}
14+
15+
class MyDelegate : Delegate {}
16+
17+
final class Container {
18+
weak var delegate: Delegate?
19+
var strongRef: Delegate
20+
21+
func callDelegate()
22+
23+
func strongDelegate(d: Delegate)
24+
}
25+
26+
class WeakCycle {
27+
weak var c: WeakCycle?
28+
}
29+
30+
sil [ossa] @$s24diagnose_lifetime_issues10MyDelegateCACycfC : $@convention(method) (@thick MyDelegate.Type) -> @owned MyDelegate
31+
sil [ossa] @$s24diagnose_lifetime_issues14strongDelegate1dyAA0D0C_tF : $@convention(method) (@guaranteed Delegate, @guaranteed Container) -> ()
32+
33+
// Test a warning for a reference that is copied, cast, and assigned
34+
// to an enum before it is assigned to a weak reference.
35+
sil [ossa] @testCastWarn : $@convention(thin) (@guaranteed Container) -> () {
36+
bb0(%0 : @guaranteed $Container):
37+
debug_value %0 : $Container, let, name "container", argno 1
38+
%2 = metatype $@thick MyDelegate.Type
39+
// function_ref MyDelegate.__allocating_init()
40+
%3 = function_ref @$s24diagnose_lifetime_issues10MyDelegateCACycfC : $@convention(method) (@thick MyDelegate.Type) -> @owned MyDelegate
41+
42+
// This is the owned allocation.
43+
%4 = apply %3(%2) : $@convention(method) (@thick MyDelegate.Type) -> @owned MyDelegate
44+
%11 = copy_value %4 : $MyDelegate
45+
46+
// This upcast+enum is not an escape.
47+
%12 = upcast %11 : $MyDelegate to $Delegate
48+
%13 = enum $Optional<Delegate>, #Optional.some!enumelt, %12 : $Delegate
49+
%14 = ref_element_addr %0 : $Container, #Container.delegate
50+
%15 = begin_access [modify] [dynamic] %14 : $*@sil_weak Optional<Delegate>
51+
52+
// This is the weak assignment.
53+
store_weak %13 to %15 : $*@sil_weak Optional<Delegate> // expected-warning {{weak reference will always be nil because the referenced object is deallocated here}}
54+
destroy_value %13 : $Optional<Delegate>
55+
end_access %15 : $*@sil_weak Optional<Delegate>
56+
destroy_value %4 : $MyDelegate
57+
%20 = tuple ()
58+
return %20 : $()
59+
}
60+
61+
// Test that a reference that escapes within a borrow scope has no warning.
62+
sil hidden [ossa] @testBorrowNoWarn : $@convention(thin) (@guaranteed Container) -> () {
63+
// %0 "container"
64+
bb0(%0 : @guaranteed $Container):
65+
debug_value %0 : $Container, let, name "container", argno 1
66+
%2 = metatype $@thick MyDelegate.Type
67+
// function_ref MyDelegate.__allocating_init()
68+
%3 = function_ref @$s24diagnose_lifetime_issues10MyDelegateCACycfC : $@convention(method) (@thick MyDelegate.Type) -> @owned MyDelegate
69+
70+
// This is the owned allocation.
71+
%4 = apply %3(%2) : $@convention(method) (@thick MyDelegate.Type) -> @owned MyDelegate
72+
debug_value %4 : $MyDelegate, let, name "delegate"
73+
%6 = begin_borrow %4 : $MyDelegate
74+
%7 = upcast %6 : $MyDelegate to $Delegate
75+
// function_ref Container.strongDelegate(d:)
76+
%8 = function_ref @$s24diagnose_lifetime_issues14strongDelegate1dyAA0D0C_tF : $@convention(method) (@guaranteed Delegate, @guaranteed Container) -> ()
77+
78+
// This apply is an escape.
79+
%9 = apply %8(%7, %0) : $@convention(method) (@guaranteed Delegate, @guaranteed Container) -> ()
80+
end_borrow %6 : $MyDelegate
81+
%11 = copy_value %4 : $MyDelegate
82+
%12 = upcast %11 : $MyDelegate to $Delegate
83+
%13 = enum $Optional<Delegate>, #Optional.some!enumelt, %12 : $Delegate
84+
%14 = ref_element_addr %0 : $Container, #Container.delegate
85+
%15 = begin_access [modify] [dynamic] %14 : $*@sil_weak Optional<Delegate>
86+
87+
// This is the weak assignment.
88+
store_weak %13 to %15 : $*@sil_weak Optional<Delegate>
89+
destroy_value %13 : $Optional<Delegate>
90+
end_access %15 : $*@sil_weak Optional<Delegate>
91+
destroy_value %4 : $MyDelegate
92+
%20 = tuple ()
93+
return %20 : $()
94+
}
95+
96+
// Helper for testReturnsAfterStore
97+
sil hidden [ossa] @testStoresWeakly : $@convention(method) (@owned WeakCycle) -> @owned WeakCycle {
98+
bb0(%0 : @owned $WeakCycle):
99+
%18 = begin_borrow %0 : $WeakCycle
100+
%19 = copy_value %0 : $WeakCycle
101+
%20 = enum $Optional<WeakCycle>, #Optional.some!enumelt, %19 : $WeakCycle
102+
%21 = ref_element_addr %18 : $WeakCycle, #WeakCycle.c
103+
%22 = begin_access [modify] [dynamic] %21 : $*@sil_weak Optional<WeakCycle>
104+
store_weak %20 to %22 : $*@sil_weak Optional<WeakCycle>
105+
destroy_value %20 : $Optional<WeakCycle>
106+
end_access %22 : $*@sil_weak Optional<WeakCycle>
107+
end_borrow %18 : $WeakCycle
108+
%27 = copy_value %0 : $WeakCycle
109+
destroy_value %0 : $WeakCycle
110+
return %27 : $WeakCycle
111+
}
112+
113+
// Test no warning for a value returned after a weak store.
114+
sil hidden [exact_self_class] [ossa] @testReturnsAfterStore : $@convention(method) (@thick WeakCycle.Type) -> @owned WeakCycle {
115+
bb0(%0 : $@thick WeakCycle.Type):
116+
%1 = alloc_ref $WeakCycle
117+
118+
%2 = function_ref @testStoresWeakly : $@convention(method) (@owned WeakCycle) -> @owned WeakCycle
119+
%3 = apply %2(%1) : $@convention(method) (@owned WeakCycle) -> @owned WeakCycle
120+
return %3 : $WeakCycle
121+
}

0 commit comments

Comments
 (0)