Skip to content

Commit 65df4ba

Browse files
slavapestovslavapestov
authored
Merge pull request #35864 from slavapestov/vtable-layout-vs-enable-testing
AST: Fix bad interaction between vtable layout, access control and -enable-testing
2 parents 82bd04e + 94287ea commit 65df4ba

File tree

8 files changed

+213
-6
lines changed

8 files changed

+213
-6
lines changed

‎include/swift/AST/Decl.h

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2324,10 +2324,8 @@ class ValueDecl : public Decl {
23242324
/// dynamic methods on generic classes (see above).
23252325
bool isNativeMethodReplacement() const;
23262326

2327-
bool isEffectiveLinkageMoreVisibleThan(ValueDecl *other) const {
2328-
return (std::min(getEffectiveAccess(), AccessLevel::Public) >
2329-
std::min(other->getEffectiveAccess(), AccessLevel::Public));
2330-
}
2327+
/// Returns if this declaration has more visible formal access than 'other'.
2328+
bool isMoreVisibleThan(ValueDecl *other) const;
23312329

23322330
/// Set whether this type is 'dynamic' or not.
23332331
void setIsDynamic(bool value);

‎lib/AST/Decl.cpp

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3417,6 +3417,23 @@ static bool checkAccess(const DeclContext *useDC, const ValueDecl *VD,
34173417
llvm_unreachable("bad access level");
34183418
}
34193419

3420+
bool ValueDecl::isMoreVisibleThan(ValueDecl *other) const {
3421+
auto scope = getFormalAccessScope();
3422+
3423+
// 'other' may have come from a @testable import, so we need to upgrade it's
3424+
// visibility to public here. That is not the same as whether 'other' is
3425+
// being built with -enable-testing though -- we don't want to treat it
3426+
// differently in that case.
3427+
auto otherScope = other->getFormalAccessScope(getDeclContext());
3428+
3429+
if (scope.isPublic())
3430+
return !otherScope.isPublic();
3431+
else if (scope.isInternal())
3432+
return !otherScope.isPublic() && !otherScope.isInternal();
3433+
else
3434+
return false;
3435+
}
3436+
34203437
bool ValueDecl::isAccessibleFrom(const DeclContext *useDC,
34213438
bool forConformance,
34223439
bool allowUsableFromInline) const {

‎lib/SILGen/SILGenType.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ SILGenModule::emitVTableMethod(ClassDecl *theClass,
111111
bool baseLessVisibleThanDerived =
112112
(!usesObjCDynamicDispatch &&
113113
!derivedDecl->isFinal() &&
114-
derivedDecl->isEffectiveLinkageMoreVisibleThan(baseDecl));
114+
derivedDecl->isMoreVisibleThan(baseDecl));
115115

116116
// Determine the derived thunk type by lowering the derived type against the
117117
// abstraction pattern of the base.

‎lib/Sema/TypeCheckDecl.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1022,7 +1022,7 @@ NeedsNewVTableEntryRequest::evaluate(Evaluator &evaluator,
10221022
// If the base is less visible than the override, we might need a vtable
10231023
// entry since callers of the override might not be able to see the base
10241024
// at all.
1025-
if (decl->isEffectiveLinkageMoreVisibleThan(base))
1025+
if (decl->isMoreVisibleThan(base))
10261026
return true;
10271027

10281028
using Direction = ASTContext::OverrideGenericSignatureReqCheck;

‎test/Interpreter/Inputs/vtables_multifile_testable_helper.swift

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
open class Base {
2+
public init() {}
3+
4+
internal func method() -> Int {
5+
return 1
6+
}
7+
}
8+
9+
open class Middle : Base {
10+
open override func method() -> Int {
11+
return super.method() + 1
12+
}
13+
}
14+
15+
public func callBaseMethod(_ b: Base) -> Int {
16+
return b.method()
17+
}
18+
19+
public func callMiddleMethod(_ m: Middle) -> Int {
20+
return m.method()
21+
}

‎test/Interpreter/vtables_multifile_testable.swift

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
// We test various combinations to make sure that -enable-testing does not
2+
// break ABI with or without -enable-library-evolution.
3+
4+
////
5+
6+
// RUN: %empty-directory(%t)
7+
8+
// 1) -enable-testing OFF / -enable-library-evolution OFF
9+
10+
// RUN: %target-build-swift-dylib(%t/%target-library-name(vtables_multifile_testable_helper)) %S/Inputs/vtables_multifile_testable_helper.swift -emit-module -emit-module-path %t/vtables_multifile_testable_helper.swiftmodule
11+
// RUN: %target-codesign %t/%target-library-name(vtables_multifile_testable_helper)
12+
13+
// RUN: %target-build-swift %s -L %t -I %t -lvtables_multifile_testable_helper -o %t/main %target-rpath(%t)
14+
// RUN: %target-codesign %t/main
15+
16+
// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
17+
18+
// 2) -enable-testing ON / -enable-library-evolution OFF
19+
20+
// ... first without rebuilding the client:
21+
22+
// RUN: %target-build-swift-dylib(%t/%target-library-name(vtables_multifile_testable_helper)) %S/Inputs/vtables_multifile_testable_helper.swift -enable-testing -emit-module -emit-module-path %t/vtables_multifile_testable_helper.swiftmodule
23+
// RUN: %target-codesign %t/%target-library-name(vtables_multifile_testable_helper)
24+
25+
// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
26+
27+
// ... now try to rebuild the client:
28+
29+
// RUN: %target-build-swift %s -L %t -I %t -lvtables_multifile_testable_helper -o %t/main %target-rpath(%t)
30+
// RUN: %target-codesign %t/main
31+
32+
// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
33+
34+
////
35+
36+
// Delete build artifacts
37+
// RUN: %empty-directory(%t)
38+
39+
// 3) -enable-testing OFF / -enable-library-evolution ON
40+
41+
// RUN: %target-build-swift-dylib(%t/%target-library-name(vtables_multifile_testable_helper)) %S/Inputs/vtables_multifile_testable_helper.swift -enable-library-evolution -emit-module -emit-module-path %t/vtables_multifile_testable_helper.swiftmodule
42+
// RUN: %target-codesign %t/%target-library-name(vtables_multifile_testable_helper)
43+
44+
// RUN: %target-build-swift %s -L %t -I %t -lvtables_multifile_testable_helper -o %t/main %target-rpath(%t)
45+
// RUN: %target-codesign %t/main
46+
47+
// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
48+
49+
// 4) -enable-testing ON / -enable-library-evolution ON
50+
51+
// ... first without rebuilding the client:
52+
53+
// RUN: %target-build-swift-dylib(%t/%target-library-name(vtables_multifile_testable_helper)) %S/Inputs/vtables_multifile_testable_helper.swift -enable-testing -enable-library-evolution -emit-module -emit-module-path %t/vtables_multifile_testable_helper.swiftmodule
54+
// RUN: %target-codesign %t/%target-library-name(vtables_multifile_testable_helper)
55+
56+
// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
57+
58+
// ... now try to rebuild the client:
59+
60+
// RUN: %target-build-swift %s -L %t -I %t -lvtables_multifile_testable_helper -o %t/main %target-rpath(%t)
61+
// RUN: %target-codesign %t/main
62+
63+
// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
64+
65+
////
66+
67+
// Delete build artifacts
68+
// RUN: %empty-directory(%t)
69+
70+
// 5) -enable-testing OFF / -enable-library-evolution ON / textual interfaces
71+
72+
// RUN: %target-build-swift-dylib(%t/%target-library-name(vtables_multifile_testable_helper)) %S/Inputs/vtables_multifile_testable_helper.swift -enable-library-evolution -emit-module-interface -emit-module-interface-path %t/vtables_multifile_testable_helper.swiftinterface
73+
// RUN: %target-codesign %t/%target-library-name(vtables_multifile_testable_helper)
74+
75+
// RUN: %target-build-swift %s -L %t -I %t -lvtables_multifile_testable_helper -o %t/main %target-rpath(%t)
76+
// RUN: %target-codesign %t/main
77+
78+
// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
79+
80+
// 6) -enable-testing ON / -enable-library-evolution ON / textual interfaces
81+
82+
// ... first without rebuilding the client:
83+
84+
// RUN: %target-build-swift-dylib(%t/%target-library-name(vtables_multifile_testable_helper)) %S/Inputs/vtables_multifile_testable_helper.swift -enable-testing -enable-library-evolution -emit-module-interface -emit-module-interface-path %t/vtables_multifile_testable_helper.swiftinterface
85+
// RUN: %target-codesign %t/%target-library-name(vtables_multifile_testable_helper)
86+
87+
// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
88+
89+
// ... now try to rebuild the client:
90+
91+
// RUN: %target-build-swift %s -L %t -I %t -lvtables_multifile_testable_helper -o %t/main %target-rpath(%t)
92+
// RUN: %target-codesign %t/main
93+
94+
// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
95+
96+
97+
// REQUIRES: executable_test
98+
99+
import StdlibUnittest
100+
import vtables_multifile_testable_helper
101+
102+
var VTableTestSuite = TestSuite("VTable")
103+
104+
public class Derived : Middle {
105+
public override func method() -> Int {
106+
return super.method() + 1
107+
}
108+
}
109+
110+
VTableTestSuite.test("Derived") {
111+
expectEqual(3, callBaseMethod(Derived()))
112+
expectEqual(3, callMiddleMethod(Derived()))
113+
}
114+
115+
runAllTests()

‎test/SILGen/Inputs/accessibility_vtables_testable_helper.swift

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
open class Base {
2+
internal func method() {}
3+
}
4+
5+
open class Middle : Base {
6+
open override func method() {}
7+
}

‎test/SILGen/accessibility_vtables_testable.swift

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
// RUN: %empty-directory(%t)
2+
3+
// RUN: %target-swift-frontend -emit-silgen %S/Inputs/accessibility_vtables_testable_helper.swift | %FileCheck %s --check-prefix=LIBRARY
4+
// RUN: %target-swift-frontend -enable-library-evolution -emit-silgen %S/Inputs/accessibility_vtables_testable_helper.swift | %FileCheck %s --check-prefix=LIBRARY
5+
// RUN: %target-swift-frontend -emit-silgen %S/Inputs/accessibility_vtables_testable_helper.swift | %FileCheck %s --check-prefix=LIBRARY
6+
// RUN: %target-swift-frontend -enable-library-evolution -emit-silgen %S/Inputs/accessibility_vtables_testable_helper.swift | %FileCheck %s --check-prefix=LIBRARY
7+
8+
// RUN: %target-swift-frontend -emit-module -o %t %S/Inputs/accessibility_vtables_testable_helper.swift
9+
// RUN: %target-swift-emit-silgen -primary-file %s -I %t | %FileCheck %s --check-prefix=FRAGILE-CLIENT
10+
11+
// RUN: %target-swift-frontend -emit-module -enable-testing -o %t %S/Inputs/accessibility_vtables_testable_helper.swift
12+
// RUN: %target-swift-emit-silgen -primary-file %s -I %t | %FileCheck %s --check-prefix=FRAGILE-CLIENT
13+
14+
// RUN: %target-swift-frontend -enable-library-evolution -emit-module -o %t %S/Inputs/accessibility_vtables_testable_helper.swift
15+
// RUN: %target-swift-emit-silgen -primary-file %s -I %t | %FileCheck %s --check-prefix=RESILIENT-CLIENT
16+
17+
// RUN: %target-swift-frontend -enable-library-evolution -emit-module -enable-testing -o %t %S/Inputs/accessibility_vtables_testable_helper.swift
18+
// RUN: %target-swift-emit-silgen -primary-file %s -I %t | %FileCheck %s --check-prefix=RESILIENT-CLIENT
19+
20+
import accessibility_vtables_testable_helper
21+
22+
public class Derived : Middle {
23+
open override func method() {}
24+
}
25+
26+
// LIBRARY-LABEL: sil_vtable {{(\[serialized\] )?}}Base {
27+
// LIBRARY-NEXT: #Base.method: (Base) -> () -> () : @$s37accessibility_vtables_testable_helper4BaseC6methodyyF
28+
// LIBRARY-NEXT: #Base.init!allocator: (Base.Type) -> () -> Base : @$s37accessibility_vtables_testable_helper4BaseCACycfC
29+
// LIBRARY-NEXT: #Base.deinit!deallocator: @$s37accessibility_vtables_testable_helper4BaseCfD
30+
// LIBRARY-NEXT: }
31+
32+
// LIBRARY-LABEL: sil_vtable {{(\[serialized\] )?}}Middle {
33+
// LIBRARY-NEXT: #Base.method: (Base) -> () -> () : @$s37accessibility_vtables_testable_helper6MiddleC6methodyyFAA4BaseCADyyFTV [override]
34+
// LIBRARY-NEXT: #Base.init!allocator: (Base.Type) -> () -> Base : @$s37accessibility_vtables_testable_helper6MiddleCACycfC [override]
35+
// LIBRARY-NEXT: #Middle.method: (Middle) -> () -> () : @$s37accessibility_vtables_testable_helper6MiddleC6methodyyF
36+
// LIBRARY-NEXT: #Middle.deinit!deallocator: @$s37accessibility_vtables_testable_helper6MiddleCfD
37+
// LIBRARY-NEXT: }
38+
39+
// FRAGILE-CLIENT-LABEL: sil_vtable [serialized] Derived {
40+
// FRAGILE-CLIENT-NEXT: #Base.method: (Base) -> () -> () : @$s37accessibility_vtables_testable_helper6MiddleC6methodyyFAA4BaseCADyyFTV [inherited]
41+
// FRAGILE-CLIENT-NEXT: #Base.init!allocator: (Base.Type) -> () -> Base : @$s37accessibility_vtables_testable_helper6MiddleCACycfC [inherited]
42+
// FRAGILE-CLIENT-NEXT: #Middle.method: (Middle) -> () -> () : @$s30accessibility_vtables_testable7DerivedC6methodyyF [override]
43+
// FRAGILE-CLIENT-NEXT: #Derived.deinit!deallocator: @$s30accessibility_vtables_testable7DerivedCfD
44+
// FRAGILE-CLIENT-NEXT: }
45+
46+
// RESILIENT-CLIENT-LABEL: sil_vtable [serialized] Derived {
47+
// RESILIENT-CLIENT-NEXT: #Middle.method: (Middle) -> () -> () : @$s30accessibility_vtables_testable7DerivedC6methodyyF [override] // Derived.method()
48+
// RESILIENT-CLIENT-NEXT: #Derived.deinit!deallocator: @$s30accessibility_vtables_testable7DerivedCfD // Derived.__deallocating_deinit
49+
// RESILIENT-CLIENT-NEXT: }

0 commit comments

Comments
 (0)