Skip to content

Commit 600ed07

Browse files
tshortlitshortli
authored
Merge pull request #77519 from tshortli/fix-conditional-opaque-type-miscompile
Sema: Fix opaque type accessors with inactive availability conditions
2 parents 500ae61 + b97e272 commit 600ed07

File tree

6 files changed

+136
-24
lines changed

6 files changed

+136
-24
lines changed

‎lib/IRGen/GenMeta.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2535,7 +2535,7 @@ namespace {
25352535

25362536
SmallVector<llvm::BasicBlock *, 4> conditionalTypes;
25372537

2538-
// Pre-allocate a basic block per condition, so there it's
2538+
// Pre-allocate a basic block per condition, so that it's
25392539
// possible to jump between conditions.
25402540
for (unsigned index : indices(substitutionSet)) {
25412541
conditionalTypes.push_back(
@@ -2619,7 +2619,7 @@ namespace {
26192619
auto universal = substitutionSet.back();
26202620

26212621
assert(universal->getAvailability().size() == 1 &&
2622-
universal->getAvailability()[0].first.isEmpty());
2622+
universal->getAvailability()[0].first.isAll());
26232623

26242624
IGF.Builder.CreateRet(
26252625
getResultValue(IGF, genericEnv, universal->getSubstitutions()));

‎lib/Sema/MiscDiagnostics.cpp‎

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3469,7 +3469,6 @@ class OpaqueUnderlyingTypeChecker : public ASTWalker {
34693469
}
34703470

34713471
// Check whether all of the underlying type candidates match up.
3472-
// TODO [OPAQUE SUPPORT]: diagnose multiple opaque types
34733472

34743473
// There is a single unique signature, which means that all returns
34753474
// matched.
@@ -3596,6 +3595,7 @@ class OpaqueUnderlyingTypeChecker : public ASTWalker {
35963595

35973596
SmallVector<OpaqueTypeDecl::ConditionallyAvailableSubstitutions *, 4>
35983597
conditionalSubstitutions;
3598+
SubstitutionMap universalSubstMap = std::get<1>(universallyAvailable);
35993599

36003600
for (const auto &entry : Candidates) {
36013601
auto availabilityContext = entry.first;
@@ -3604,25 +3604,46 @@ class OpaqueUnderlyingTypeChecker : public ASTWalker {
36043604
if (!availabilityContext)
36053605
continue;
36063606

3607+
unsigned neverAvailableCount = 0, alwaysAvailableCount = 0;
36073608
SmallVector<AvailabilityCondition, 4> conditions;
36083609

36093610
for (const auto &elt : availabilityContext->getCond()) {
36103611
auto condition = elt.getAvailability();
36113612

36123613
auto availabilityRange = condition->getAvailableRange();
3613-
// If there is no lower endpoint it means that the
3614-
// current platform is unrelated to this condition
3615-
// and we can ignore it.
3616-
if (!availabilityRange.hasLowerEndpoint())
3614+
// If there is no lower endpoint it means that the condition has no
3615+
// OS version specification that matches the target platform.
3616+
if (!availabilityRange.hasLowerEndpoint()) {
3617+
// An inactive #unavailable condition trivially evaluates to false.
3618+
if (condition->isUnavailability()) {
3619+
neverAvailableCount++;
3620+
continue;
3621+
}
3622+
3623+
// An inactive #available condition trivially evaluates to true.
3624+
alwaysAvailableCount++;
36173625
continue;
3626+
}
36183627

36193628
conditions.push_back(
36203629
{availabilityRange, condition->isUnavailability()});
36213630
}
36223631

3623-
if (conditions.empty())
3632+
// If there were any conditions that were always false, then this
3633+
// candidate is unreachable at runtime.
3634+
if (neverAvailableCount > 0)
36243635
continue;
36253636

3637+
// If all the conditions were trivially true, then this candidate is
3638+
// effectively a universally available candidate and the rest of the
3639+
// candidates should be ignored since they are unreachable.
3640+
if (alwaysAvailableCount == availabilityContext->getCond().size()) {
3641+
universalSubstMap = std::get<1>(candidate);
3642+
break;
3643+
}
3644+
3645+
ASSERT(conditions.size() > 0);
3646+
36263647
conditionalSubstitutions.push_back(
36273648
OpaqueTypeDecl::ConditionallyAvailableSubstitutions::get(
36283649
Ctx, conditions,
@@ -3632,9 +3653,8 @@ class OpaqueUnderlyingTypeChecker : public ASTWalker {
36323653
// Add universally available choice as the last one.
36333654
conditionalSubstitutions.push_back(
36343655
OpaqueTypeDecl::ConditionallyAvailableSubstitutions::get(
3635-
Ctx, {{VersionRange::empty(), /*unavailable=*/false}},
3636-
std::get<1>(universallyAvailable)
3637-
.mapReplacementTypesOutOfContext()));
3656+
Ctx, {{VersionRange::all(), /*unavailable=*/false}},
3657+
universalSubstMap.mapReplacementTypesOutOfContext()));
36383658

36393659
OpaqueDecl->setConditionallyAvailableSubstitutions(
36403660
conditionalSubstitutions);

‎lib/Serialization/Deserialization.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4559,7 +4559,7 @@ class DeclDeserializer {
45594559
} else {
45604560
limitedAvailability.push_back(
45614561
OpaqueTypeDecl::ConditionallyAvailableSubstitutions::get(
4562-
ctx, {{VersionRange::empty(), /*unavailability=*/false}},
4562+
ctx, {{VersionRange::all(), /*unavailability=*/false}},
45634563
subMapOrError.get()));
45644564

45654565
opaqueDecl->setConditionallyAvailableSubstitutions(limitedAvailability);

‎test/IRGen/opaque_result_with_conditional_availability.swift‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,11 @@ struct C : P {
2424
func hello() { print("Hello from C") }
2525
}
2626

27+
@available(iOS 100, *)
28+
struct D : P {
29+
func hello() { print("Hello from D") }
30+
}
31+
2732
func test_multiple_single() -> some P {
2833
if #available(macOS 100.0.1, *) {
2934
return A()
@@ -61,6 +66,44 @@ func test_multiple_single() -> some P {
6166
// CHECK-NEXT: ret ptr @"$s43opaque_result_with_conditional_availability1CVAA1PAAWP"
6267
// CHECK-NEXT: }
6368

69+
func test_multiple_single_multiplatform() -> some P {
70+
if #available(macOS 100.0.1, *), #available(iOS 100, *) {
71+
return A()
72+
}
73+
74+
return C()
75+
}
76+
77+
// CHECK: define private ptr @"get_underlying_type_ref 43opaque_result_with_conditional_availabilityAA34test_multiple_single_multiplatformQryFQOQr"(ptr %0)
78+
// CHECK-NEXT: entry:
79+
// CHECK-NEXT: br label %conditional-0
80+
// CHECK: conditional-0: ; preds = %entry
81+
// CHECK-NEXT: br label %cond-0-0
82+
// CHECK: cond-0-0: ; preds = %conditional-0
83+
// CHECK-NEXT: [[COND_1:%.*]] = call i32 @__isPlatformVersionAtLeast(i32 1, i32 100, i32 0, i32 1)
84+
// CHECK-NEXT: [[IS_AT_LEAST:%.*]] = icmp ne i32 [[COND_1]], 0
85+
// CHECK-NEXT: br i1 [[IS_AT_LEAST]], label %result-0, label %universal
86+
// CHECK: result-0: ; preds = %cond-0-0
87+
// CHECK-NEXT: ret ptr {{.*}} @"$s43opaque_result_with_conditional_availability1AVMf"
88+
// CHECK: universal: ; preds = %cond-0-0
89+
// CHECK-NEXT: ret ptr {{.*}} @"$s43opaque_result_with_conditional_availability1CVMf"
90+
// CHECK-NEXT: }
91+
92+
// CHECK: define private ptr @"get_underlying_witness 43opaque_result_with_conditional_availabilityAA34test_multiple_single_multiplatformQryFQOxAA1PHC"(ptr %0)
93+
// CHECK-NEXT: entry:
94+
// CHECK-NEXT: br label %conditional-0
95+
// CHECK: conditional-0: ; preds = %entry
96+
// CHECK-NEXT: br label %cond-0-0
97+
// CHECK: cond-0-0: ; preds = %conditional-0
98+
// CHECK-NEXT: [[COND_1:%.*]] = call i32 @__isPlatformVersionAtLeast(i32 1, i32 100, i32 0, i32 1)
99+
// CHECK-NEXT: [[IS_AT_LEAST:%.*]] = icmp ne i32 [[COND_1]], 0
100+
// CHECK-NEXT: br i1 [[IS_AT_LEAST]], label %result-0, label %universal
101+
// CHECK: result-0: ; preds = %cond-0-0
102+
// CHECK-NEXT: ret ptr @"$s43opaque_result_with_conditional_availability1AVAA1PAAWP"
103+
// CHECK: universal: ; preds = %cond-0-0
104+
// CHECK-NEXT: ret ptr @"$s43opaque_result_with_conditional_availability1CVAA1PAAWP"
105+
// CHECK-NEXT: }
106+
64107
func test_multiple_conds() -> some P {
65108
if #available(macOS 100.0.1, *) {
66109
return A()
@@ -208,3 +251,51 @@ func test_cross_module() -> some SomeProtocol {
208251
// CHECK-NEXT: [[R0:%.*]] = call ptr @"$s49opaque_result_with_conditional_availability_types12PublicStructVAcA12SomeProtocolAAWl"()
209252
// CHECK-NEXT: ret ptr [[R0]]
210253
// CHECK: }
254+
255+
func test_other_platform_available() -> some P {
256+
if #available(iOS 100, *) {
257+
return D() // Always executed on macOS
258+
}
259+
260+
return C() // Never executed on macOS
261+
}
262+
263+
// CHECK: define private ptr @"get_underlying_type_ref 43opaque_result_with_conditional_availabilityAA29test_other_platform_availableQryFQOQr"(ptr %0)
264+
// CHECK-NEXT: entry:
265+
// CHECK-NEXT: br label %universal
266+
267+
// CHECK: universal:
268+
// CHECK-NEXT: ret ptr getelementptr inbounds (<{ ptr, ptr, i64, ptr }>, ptr @"$s43opaque_result_with_conditional_availability1DVMf", i32 0, i32 2)
269+
// CHECK-NEXT: }
270+
271+
// CHECK: define private ptr @"get_underlying_witness 43opaque_result_with_conditional_availabilityAA29test_other_platform_availableQryFQOxAA1PHC"(ptr %0)
272+
// CHECK-NEXT: entry:
273+
// CHECK-NEXT: br label %universal
274+
275+
// CHECK: universal:
276+
// CHECK-NEXT: ret ptr @"$s43opaque_result_with_conditional_availability1DVAA1PAAWP"
277+
// CHECK-NEXT: }
278+
279+
func test_other_platform_unavailable() -> some P {
280+
if #unavailable(iOS 100) {
281+
return C() // Never executed on macOS
282+
}
283+
284+
return D() // Always executed on macOS
285+
}
286+
287+
// CHECK: define private ptr @"get_underlying_type_ref 43opaque_result_with_conditional_availabilityAA31test_other_platform_unavailableQryFQOQr"(ptr %0)
288+
// CHECK-NEXT: entry:
289+
// CHECK-NEXT: br label %universal
290+
291+
// CHECK: universal:
292+
// CHECK-NEXT: ret ptr getelementptr inbounds (<{ ptr, ptr, i64, ptr }>, ptr @"$s43opaque_result_with_conditional_availability1DVMf", i32 0, i32 2)
293+
// CHECK-NEXT: }
294+
295+
// CHECK: define private ptr @"get_underlying_witness 43opaque_result_with_conditional_availabilityAA31test_other_platform_unavailableQryFQOxAA1PHC"(ptr %0)
296+
// CHECK-NEXT: entry:
297+
// CHECK-NEXT: br label %universal
298+
299+
// CHECK: universal:
300+
// CHECK-NEXT: ret ptr @"$s43opaque_result_with_conditional_availability1DVAA1PAAWP"
301+
// CHECK-NEXT: }

‎test/Serialization/Inputs/opaque_with_limited_availability.swift‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -44,37 +44,34 @@ public struct Example {
4444
}
4545
}
4646

47-
public func test() -> some P {
47+
public func testAvailableQueryWithUniversalResult() -> some P {
4848
if #available(macOS 100.0.1, *) {
4949
return Tuple<(Int, Int)>((0, 0))
5050
}
5151

5252
return Empty()
5353
}
5454

55-
public func testUnavailable() -> some P {
55+
public func testUnavailableQueryWithLimitedResult() -> some P {
5656
if #unavailable(macOS 100.0.1) {
5757
return Tuple<(Int, Int)>((0, 1))
5858
}
5959

6060
return Empty()
6161
}
6262

63-
public func test_return_from_conditional() -> some P {
63+
public func testAvailableQueryWithLimitedResult() -> some P {
6464
if #available(macOS 10.15, *) {
6565
return Named()
6666
}
6767

6868
return Tuple<(String, Int)>(("", 0))
6969
}
7070

71-
// This used to crash during serialization because
72-
// @available negates availability condition.
73-
@available(macOS, unavailable)
74-
public func testUnusable() -> some P {
71+
public func testInactiveAvailableQuery() -> some P {
7572
if #available(iOS 50, *) {
76-
return Named()
73+
return Empty()
7774
}
7875

79-
return Tuple<(String, Int)>(("", 0))
76+
return Named()
8077
}

‎test/Serialization/opaque_with_availability_cross_module.swift‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,18 +39,22 @@ public struct Test {
3939
}
4040
}
4141

42-
let result = LimitedAvailOpaque.test()
42+
let result = LimitedAvailOpaque.testAvailableQueryWithUniversalResult()
4343
result.hello()
4444
// CHECK: Hello from Empty
4545

4646
Test().sayHello()
4747
// CHECK: Hello from Empty
4848
// CHECK: Hello from Tuple
4949

50-
let conditionalR = LimitedAvailOpaque.test_return_from_conditional()
50+
let conditionalR = LimitedAvailOpaque.testAvailableQueryWithLimitedResult()
5151
conditionalR.hello()
5252
// CHECK: Hello from Named
5353

54-
let unavailableTest = LimitedAvailOpaque.testUnavailable()
54+
let unavailableTest = LimitedAvailOpaque.testUnavailableQueryWithLimitedResult()
5555
unavailableTest.hello()
5656
// CHECK: Hello from Tuple
57+
58+
let inactiveTest = LimitedAvailOpaque.testInactiveAvailableQuery()
59+
inactiveTest.hello()
60+
// CHECK: Hello from Empty

0 commit comments

Comments
 (0)