Skip to content

Commit e2bb7e8

Browse files
committed
SILOptimizer: Remove switch cases matching unavailable enum elements.
Unavailable enum elements cannot be instantiated at runtime without invoking UB. Therefore the optimizer can consider a basic block unreachable if its only predecessor is a block that terminates in a switch instruction matching an unavailable enum element. Furthermore, removing the switch instruction cases that refer to unavailable enum elements is _mandatory_ when `-unavailable-decl-optimization=complete` is specified because otherwise lowered IR for these instructions could refer to enum tag accessors that will not be lowered, resulting in a failure during linking. Resolves rdar://113872720.
1 parent 1219459 commit e2bb7e8

File tree

11 files changed

+478
-3
lines changed

11 files changed

+478
-3
lines changed

include/swift/AST/Decl.h

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1237,7 +1237,10 @@ class alignas(1 << DeclAlignInBits) Decl : public ASTAllocated<Decl> {
12371237
/// Returns true if this declaration should be considered available during
12381238
/// SIL/IR lowering. A declaration would not be available during lowering if,
12391239
/// for example, it is annotated as unavailable with `@available` and
1240-
/// optimization settings require that it be omitted.
1240+
/// optimization settings require that it be omitted. Canonical SIL must not
1241+
/// contain any references to declarations that are unavailable during
1242+
/// lowering because the resulting IR could reference non-existent symbols
1243+
/// and fail to link.
12411244
bool isAvailableDuringLowering() const;
12421245

12431246
/// Returns true if ABI compatibility stubs must be emitted for the given
@@ -4204,6 +4207,11 @@ class EnumDecl final : public NominalTypeDecl {
42044207
/// A range for iterating the elements of an enum.
42054208
using ElementRange = DowncastFilterRange<EnumElementDecl, DeclRange>;
42064209

4210+
/// A range for iterating the elements of an enum that are available during
4211+
/// lowering.
4212+
using ElementRangeForLowering = OptionalTransformRange<
4213+
ElementRange, AvailableDuringLoweringDeclFilter<EnumElementDecl>>;
4214+
42074215
/// A range for iterating the cases of an enum.
42084216
using CaseRange = DowncastFilterRange<EnumCaseDecl, DeclRange>;
42094217

@@ -4217,6 +4225,13 @@ class EnumDecl final : public NominalTypeDecl {
42174225
return std::distance(eltRange.begin(), eltRange.end());
42184226
}
42194227

4228+
/// Returns a range that iterates over all the elements of an enum for which
4229+
/// \c isAvailableDuringLowering() is true.
4230+
ElementRangeForLowering getAllElementsForLowering() const {
4231+
return ElementRangeForLowering(
4232+
getAllElements(), AvailableDuringLoweringDeclFilter<EnumElementDecl>());
4233+
}
4234+
42204235
/// If this enum has a unique element, return it. A unique element can
42214236
/// either hold a value or not, and the existence of one unique element does
42224237
/// not imply the existence or non-existence of the opposite unique element.

include/swift/SIL/SILInstruction.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6583,7 +6583,7 @@ class SelectEnumInstBase : public BaseTy {
65836583
}
65846584

65856585
llvm::SmallPtrSet<EnumElementDecl *, 4> unswitchedElts;
6586-
for (auto elt : decl->getAllElements())
6586+
for (auto elt : decl->getAllElementsForLowering())
65876587
unswitchedElts.insert(elt);
65886588

65896589
for (unsigned i = 0, e = this->getNumCases(); i != e; ++i) {
@@ -9812,7 +9812,7 @@ class SwitchEnumInstBase : public BaseTy {
98129812
assert(decl && "switch_enum operand is not an enum");
98139813

98149814
SmallPtrSet<EnumElementDecl *, 4> unswitchedElts;
9815-
for (auto elt : decl->getAllElements())
9815+
for (auto elt : decl->getAllElementsForLowering())
98169816
unswitchedElts.insert(elt);
98179817

98189818
for (unsigned i = 0, e = getNumCases(); i != e; ++i) {

lib/SIL/IR/SILType.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -940,6 +940,12 @@ TypeBase::replaceSubstitutedSILFunctionTypesWithUnsubstituted(SILModule &M) cons
940940
bool SILType::isEffectivelyExhaustiveEnumType(SILFunction *f) {
941941
EnumDecl *decl = getEnumOrBoundGenericEnum();
942942
assert(decl && "Called for a non enum type");
943+
944+
// Since unavailable enum elements cannot be referenced in canonical SIL,
945+
// enums with these elements cannot be treated as exhaustive types.
946+
if (decl->hasCasesUnavailableDuringLowering())
947+
return false;
948+
943949
return decl->isEffectivelyExhaustive(f->getModule().getSwiftModule(),
944950
f->getResilienceExpansion());
945951
}

lib/SIL/Verifier/SILVerifier.cpp

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4906,6 +4906,14 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
49064906
requireSameType(
49074907
result->getType(), SEO->getType(),
49084908
"select_enum case operand must match type of instruction");
4909+
4910+
// In canonical SIL, select instructions must not cover any enum elements
4911+
// that are unavailable.
4912+
if (F.getModule().getStage() >= SILStage::Canonical) {
4913+
require(elt->isAvailableDuringLowering(),
4914+
"select_enum dispatches on enum element that is unavailable "
4915+
"during lowering.");
4916+
}
49094917
}
49104918

49114919
// If the select is non-exhaustive, we require a default.
@@ -4994,6 +5002,15 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
49945002
"arguments");
49955003
return;
49965004
}
5005+
5006+
// In canonical SIL, switch instructions must not cover any enum elements
5007+
// that are unavailable.
5008+
if (F.getModule().getStage() >= SILStage::Canonical) {
5009+
require(elt->isAvailableDuringLowering(),
5010+
"switch_enum dispatches on enum element that is unavailable "
5011+
"during lowering.");
5012+
}
5013+
49975014
// Check for a valid switch result type.
49985015
if (dest->getArguments().size() == 1) {
49995016
SILType eltArgTy = uTy.getEnumElementType(elt, F.getModule(),
@@ -5098,6 +5115,14 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
50985115
"more than once");
50995116
unswitchedElts.erase(elt);
51005117

5118+
// In canonical SIL, switch instructions must not cover any enum elements
5119+
// that are unavailable.
5120+
if (F.getModule().getStage() >= SILStage::Canonical) {
5121+
require(elt->isAvailableDuringLowering(),
5122+
"switch_enum_addr dispatches on enum element that is "
5123+
"unavailable during lowering.");
5124+
}
5125+
51015126
// The destination BB must not have BB arguments.
51025127
require(dest->getArguments().empty(),
51035128
"switch_enum_addr destination must take no BB args");

lib/SILOptimizer/Mandatory/DiagnoseUnreachable.cpp

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ enum class UnreachableKind {
5151
FoldedBranch,
5252
FoldedSwitchEnum,
5353
NoreturnCall,
54+
UnavailableEnumElement,
5455
};
5556

5657
/// Information about a folded conditional branch instruction: it's location
@@ -867,6 +868,92 @@ static bool simplifyBlocksWithCallsToNoReturn(SILBasicBlock &BB,
867868
return true;
868869
}
869870

871+
/// Replaces `switch_enum` / `switch_enum_addr` instructions that have cases
872+
/// matching unavailable enum elements with new instructions that have those
873+
/// cases removed since unavailable enum elements cannot be instantiated at
874+
/// run time.
875+
///
876+
/// If this pass removes a case, it will add a default case that traps if there
877+
/// was not already an existing default case. This artificial default case
878+
/// serves two purposes. The first is to ensure the new switch instruction still
879+
/// "covers" the entire enum as required by SIL verification (this analysis
880+
/// could be changed in the future to ignore unavailable elements). Secondly,
881+
/// the trapping default case may help developers catch issues at runtime where
882+
/// UB has been invoked and an unavailable element has been instantiated
883+
/// unexpectedly. Ideally, this debugging aid would be available consistently,
884+
/// but since we cannot leave any references to the unavailable elements in SIL
885+
/// it is not possible to transform switch instructions that already have
886+
/// default cases in such a way that unavailable elements would be detected.
887+
static bool eliminateSwitchDispatchOnUnavailableElements(
888+
SILBasicBlock &BB, UnreachableUserCodeReportingState *State) {
889+
SwitchEnumTermInst SWI(BB.getTerminator());
890+
if (!SWI)
891+
return false;
892+
893+
// FIXME: Return early if EnumDecl doesn't have any elements that are
894+
// unavailable.
895+
896+
ASTContext &ctx = BB.getModule().getASTContext();
897+
SILLocation Loc = SWI->getLoc();
898+
bool DidRemoveUnavailableCase = false;
899+
900+
SmallVector<std::pair<EnumElementDecl *, SILBasicBlock *>, 4> NewCaseBBs;
901+
for (unsigned i : range(SWI.getNumCases())) {
902+
auto CaseBB = SWI.getCase(i);
903+
auto availableAtr = CaseBB.first->getAttrs().getUnavailable(ctx);
904+
905+
if (availableAtr && availableAtr->isUnconditionallyUnavailable()) {
906+
// Mark the basic block as potentially unreachable.
907+
SILBasicBlock *UnreachableBlock = CaseBB.second;
908+
if (!State->PossiblyUnreachableBlocks.contains(UnreachableBlock)) {
909+
// If this is the first time we see this unreachable block, store it.
910+
State->PossiblyUnreachableBlocks.insert(UnreachableBlock);
911+
State->MetaMap.insert(
912+
{UnreachableBlock,
913+
UnreachableInfo{UnreachableKind::UnavailableEnumElement, Loc,
914+
true}});
915+
}
916+
DidRemoveUnavailableCase = true;
917+
} else {
918+
NewCaseBBs.push_back(CaseBB);
919+
}
920+
}
921+
922+
if (!DidRemoveUnavailableCase)
923+
return false;
924+
925+
// Since at least one case was removed, we need to add a default case that
926+
// traps if there isn't already an existing default case. The resulting SIL
927+
// will have a structure that matches what SILGen would have produced for a
928+
// switch statment that was written in source with unavailable cases
929+
// unhandled.
930+
SILBasicBlock *DefaultBB = SWI.getDefaultBBOrNull().getPtrOrNull();
931+
bool DidCreateDefault = false;
932+
if (!DefaultBB) {
933+
DefaultBB = BB.getParent()->createBasicBlock();
934+
SILLocation genLoc = Loc.asAutoGenerated();
935+
SILBuilder B(DefaultBB);
936+
B.createUnconditionalFail(genLoc, "unexpected enum value");
937+
B.createUnreachable(genLoc);
938+
DidCreateDefault = true;
939+
}
940+
941+
if (isa<SwitchEnumInst>(*SWI)) {
942+
auto *SEI = SILBuilderWithScope(SWI).createSwitchEnum(
943+
Loc, SWI.getOperand(), DefaultBB, NewCaseBBs);
944+
945+
if (DidCreateDefault)
946+
SEI->createDefaultResult();
947+
} else {
948+
assert(isa<SwitchEnumAddrInst>(*SWI) && "unknown switch_enum instruction");
949+
SILBuilderWithScope(SWI).createSwitchEnumAddr(Loc, SWI.getOperand(),
950+
DefaultBB, NewCaseBBs);
951+
}
952+
SWI->eraseFromParent();
953+
954+
return true;
955+
}
956+
870957
/// Issue an "unreachable code" diagnostic if the blocks contains or
871958
/// leads to another block that contains user code.
872959
///
@@ -935,6 +1022,11 @@ static bool diagnoseUnreachableBlock(
9351022
}
9361023
break;
9371024
}
1025+
case (UnreachableKind::UnavailableEnumElement): {
1026+
// Don't diagnose blocks removed because they were only reachable from
1027+
// a case matching an unavailable enum element.
1028+
break;
1029+
}
9381030
}
9391031

9401032
// Record that we've reported this unreachable block to avoid duplicates
@@ -1076,6 +1168,11 @@ static void diagnoseUnreachable(SILFunction &Fn) {
10761168
// function.
10771169
if (simplifyBlocksWithCallsToNoReturn(BB, &State))
10781170
continue;
1171+
1172+
// Remove the cases of switch_enum / switch_enum_addr instructions that
1173+
// match unavailable enum elements.
1174+
if (eliminateSwitchDispatchOnUnavailableElements(BB, &State))
1175+
continue;
10791176
}
10801177

10811178
// Remove unreachable blocks.
@@ -1094,6 +1191,10 @@ static void diagnoseUnreachable(SILFunction &Fn) {
10941191
// function.
10951192
if (simplifyBlocksWithCallsToNoReturn(BB, &State))
10961193
continue;
1194+
// Remove the cases of switch_enum / switch_enum_addr instructions that
1195+
// match unavailable enum elements.
1196+
if (eliminateSwitchDispatchOnUnavailableElements(BB, &State))
1197+
continue;
10971198
}
10981199

10991200
// Remove unreachable blocks.

test/Inputs/resilient_enum.swift

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,3 +208,28 @@ public enum ResilientIndirectEnum {
208208
// -2
209209
indirect case B(ResilientIndirectEnum, ResilientIndirectEnum)
210210
}
211+
212+
public enum ResilientEnumWithUnavailableCase {
213+
case available
214+
215+
@available(*, unavailable)
216+
case unavailable
217+
}
218+
219+
public enum ResilientEnumWithUnavailableCaseAndPayload {
220+
@available(*, unavailable)
221+
case int(_ i: UnavailableResilientInt)
222+
223+
case double(_ d: ResilientDouble)
224+
}
225+
226+
extension ResilientEnumWithUnavailableCaseAndPayload {
227+
@_alwaysEmitIntoClient
228+
public var intValue: Int {
229+
switch self {
230+
case .int(let i): return i.i
231+
case .double(let d): return Int(d.d)
232+
default: return -1
233+
}
234+
}
235+
}

test/Inputs/resilient_struct.swift

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,3 +110,12 @@ public struct Container {
110110
public struct PairContainer {
111111
public var pair : (Container, Container)
112112
}
113+
114+
@available(*, unavailable)
115+
public struct UnavailableResilientInt {
116+
public let i: Int
117+
118+
public init(i: Int) {
119+
self.i = i
120+
}
121+
}

test/Interpreter/enum_resilience.swift

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111

1212
// RUN: %target-run %t/main %t/%target-library-name(resilient_struct) %t/%target-library-name(resilient_enum)
1313

14+
// Test against libraries built with -whole-module-optimization.
15+
1416
// RUN: %target-build-swift-dylib(%t/%target-library-name(resilient_struct_wmo)) -enable-library-evolution %S/../Inputs/resilient_struct.swift -emit-module -emit-module-path %t/resilient_struct.swiftmodule -module-name resilient_struct -whole-module-optimization
1517
// RUN: %target-codesign %t/%target-library-name(resilient_struct_wmo)
1618

@@ -22,6 +24,20 @@
2224

2325
// RUN: %target-run %t/main2 %t/%target-library-name(resilient_struct_wmo) %t/%target-library-name(resilient_enum_wmo)
2426

27+
// Test with -unavailable-decl-optimization=complete.
28+
29+
// RUN: %target-build-swift-dylib(%t/%target-library-name(resilient_struct_udoc)) -enable-library-evolution %S/../Inputs/resilient_struct.swift -emit-module -emit-module-path %t/resilient_struct.swiftmodule -module-name resilient_struct -unavailable-decl-optimization=complete
30+
// RUN: %target-codesign %t/%target-library-name(resilient_struct_udoc)
31+
32+
// RUN: %target-build-swift-dylib(%t/%target-library-name(resilient_enum_udoc)) -enable-library-evolution %S/../Inputs/resilient_enum.swift -emit-module -emit-module-path %t/resilient_enum.swiftmodule -module-name resilient_enum -I%t -L%t -lresilient_struct_udoc -unavailable-decl-optimization=complete
33+
// RUN: %target-codesign %t/%target-library-name(resilient_enum_udoc)
34+
35+
// RUN: %target-build-swift %s -L %t -I %t -lresilient_struct_udoc -lresilient_enum_udoc -o %t/main3 %target-rpath(%t)
36+
// RUN: %target-codesign %t/main3
37+
38+
// RUN: %target-run %t/main3 %t/%target-library-name(resilient_struct_udoc) %t/%target-library-name(resilient_enum_udoc)
39+
40+
2541
// REQUIRES: executable_test
2642

2743
import StdlibUnittest
@@ -491,4 +507,31 @@ ResilientEnumTestSuite.test("ResilientEnumSingleCase") {
491507
// This used to crash.
492508
test(Status(fst: .only(nested: Nested(str: "foobar", r: ResilientInt(i: 1))), snd: false))
493509
}
510+
511+
ResilientEnumTestSuite.test("ResilientEnumWithUnavailableCase") {
512+
let a: [ResilientEnumWithUnavailableCase] = [.available]
513+
514+
let b: [Int] = a.map {
515+
switch $0 {
516+
case .available:
517+
return 0
518+
case .unavailable:
519+
return 1
520+
default:
521+
return -1
522+
}
523+
}
524+
525+
expectEqual(b, [0])
526+
}
527+
528+
ResilientEnumTestSuite.test("ResilientEnumWithUnavailableCaseAndPayload") {
529+
let a: [ResilientEnumWithUnavailableCaseAndPayload] =
530+
[.double(ResilientDouble(d: 42.0))]
531+
532+
let b: [Int] = a.map { return $0.intValue }
533+
534+
expectEqual(b, [42])
535+
}
536+
494537
runAllTests()
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// RUN: %target-swift-frontend -emit-sil -unavailable-decl-optimization=none -verify %s
2+
// RUN: %target-swift-frontend -emit-sil -unavailable-decl-optimization=complete -verify %s
3+
4+
public enum HasUnavailableElement {
5+
case available
6+
7+
@available(*, unavailable)
8+
case unavailable
9+
}
10+
11+
public func markUsed<T>(_ t: T) {}
12+
13+
public func testSwitch(_ e: HasUnavailableElement) {
14+
switch e {
15+
case .available:
16+
()
17+
case .unavailable:
18+
let x: Int // expected-note {{constant defined here}}
19+
markUsed(x) // expected-error {{constant 'x' used before being initialized}}
20+
}
21+
}
22+
23+
public func testIfCase(_ e: HasUnavailableElement) {
24+
if case .unavailable = e {
25+
let x: Int // expected-note {{constant defined here}}
26+
markUsed(x) // expected-error {{constant 'x' used before being initialized}}
27+
}
28+
}
29+
30+
public func testInitSwitch() {
31+
struct S {
32+
var x: Int // expected-note {{'self.x' not initialized}}
33+
34+
init(_ e: HasUnavailableElement) {
35+
switch e {
36+
case .available:
37+
x = 1
38+
case .unavailable:
39+
()
40+
}
41+
} // expected-error {{return from initializer without initializing all stored properties}}
42+
}
43+
}

0 commit comments

Comments
 (0)