AST: Fix bad interaction between vtable layout, access control and -enable-testing

slavapestov · slavapestov · commit 94287ea6a965 · 2021-02-09T23:35:14.000-05:00
Swift allows a method override to be more visible than the base method.

In practice, this means that since it might be possible for client
code to see the override but not the base method, we have to take
extra care when emitting the override.

Specifically, the override always receives a new vtable entry, and
a vtable thunk is emitted in place of the base method's vtable entry
which re-dispatches via the override's vtable entry.

This allows client code to further override the method without any
knowledge of the base method's vtable entry, which may be inaccessible
to the client.

In order for the above to work, three places in the code perform
co-ordinated checks:

- needsNewVTableEntry() determines whether the override is more
  visible than the base, in which case it receives a new vtable
  entry

- SILGenModule::emitVTableMethod() performs the same check in order
  to emit the re-dispatching vtable thunk in place of the base
  method's entry

- in the client, SILVTableVisitor then skips the base method's
  vtable entry entirely when emitting the derived class, since no
  thunk is to be emitted.

The problem was that the first two used effective access (where
internal declarations become public with -enable-testing), while
the last check used formal access. As a result, it was possible
for the method override vtable entry to never be emitted in the
client.

Consistently using either effective access or formal access would
fix the problem. I fixed the first two to rely on formal access;
the reason is that using effective access makes vtable layout
depend on whether the library was built with -enable-testing or
not, which is undesirable since we do not want -enable-testing to
impact the ABI, even for non-resilient frameworks.

Fixes rdar://problem/74108928.
diff --git a/include/swift/AST/Decl.h b/include/swift/AST/Decl.h
@@ -2321,10 +2321,8 @@ class ValueDecl : public Decl {
   /// dynamic methods on generic classes (see above).
   bool isNativeMethodReplacement() const;
 
-  bool isEffectiveLinkageMoreVisibleThan(ValueDecl *other) const {
-    return (std::min(getEffectiveAccess(), AccessLevel::Public) >
-            std::min(other->getEffectiveAccess(), AccessLevel::Public));
-  }
+  /// Returns if this declaration has more visible formal access than 'other'.
+  bool isMoreVisibleThan(ValueDecl *other) const;
 
   /// Set whether this type is 'dynamic' or not.
   void setIsDynamic(bool value);
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -3417,6 +3417,23 @@ static bool checkAccess(const DeclContext *useDC, const ValueDecl *VD,
   llvm_unreachable("bad access level");
 }
 
+bool ValueDecl::isMoreVisibleThan(ValueDecl *other) const {
+  auto scope = getFormalAccessScope();
+
+  // 'other' may have come from a @testable import, so we need to upgrade it's
+  // visibility to public here. That is not the same as whether 'other' is
+  // being built with -enable-testing though -- we don't want to treat it
+  // differently in that case.
+  auto otherScope = other->getFormalAccessScope(getDeclContext());
+
+  if (scope.isPublic())
+    return !otherScope.isPublic();
+  else if (scope.isInternal())
+    return !otherScope.isPublic() && !otherScope.isInternal();
+  else
+    return false;
+}
+
 bool ValueDecl::isAccessibleFrom(const DeclContext *useDC,
                                  bool forConformance,
                                  bool allowUsableFromInline) const {
diff --git a/lib/SILGen/SILGenType.cpp b/lib/SILGen/SILGenType.cpp
@@ -111,7 +111,7 @@ SILGenModule::emitVTableMethod(ClassDecl *theClass,
   bool baseLessVisibleThanDerived =
     (!usesObjCDynamicDispatch &&
      !derivedDecl->isFinal() &&
-     derivedDecl->isEffectiveLinkageMoreVisibleThan(baseDecl));
+     derivedDecl->isMoreVisibleThan(baseDecl));
 
   // Determine the derived thunk type by lowering the derived type against the
   // abstraction pattern of the base.
diff --git a/lib/Sema/TypeCheckDecl.cpp b/lib/Sema/TypeCheckDecl.cpp
@@ -1022,7 +1022,7 @@ NeedsNewVTableEntryRequest::evaluate(Evaluator &evaluator,
   // If the base is less visible than the override, we might need a vtable
   // entry since callers of the override might not be able to see the base
   // at all.
-  if (decl->isEffectiveLinkageMoreVisibleThan(base))
+  if (decl->isMoreVisibleThan(base))
     return true;
 
   using Direction = ASTContext::OverrideGenericSignatureReqCheck;
diff --git a/test/Interpreter/Inputs/vtables_multifile_testable_helper.swift b/test/Interpreter/Inputs/vtables_multifile_testable_helper.swift
@@ -0,0 +1,21 @@
+open class Base {
+  public init() {}
+
+  internal func method() -> Int {
+    return 1
+  }
+}
+
+open class Middle : Base {
+  open override func method() -> Int {
+    return super.method() + 1
+  }
+}
+
+public func callBaseMethod(_ b: Base) -> Int {
+  return b.method()
+}
+
+public func callMiddleMethod(_ m: Middle) -> Int {
+  return m.method()
+}
diff --git a/test/Interpreter/vtables_multifile_testable.swift b/test/Interpreter/vtables_multifile_testable.swift
@@ -0,0 +1,115 @@
+// We test various combinations to make sure that -enable-testing does not
+// break ABI with or without -enable-library-evolution.
+
+////
+
+// RUN: %empty-directory(%t)
+
+// 1) -enable-testing OFF / -enable-library-evolution OFF
+
+// 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
+// RUN: %target-codesign %t/%target-library-name(vtables_multifile_testable_helper)
+
+// RUN: %target-build-swift %s -L %t -I %t -lvtables_multifile_testable_helper -o %t/main %target-rpath(%t)
+// RUN: %target-codesign %t/main
+
+// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
+
+// 2) -enable-testing ON / -enable-library-evolution OFF
+
+// ... first without rebuilding the client:
+
+// 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
+// RUN: %target-codesign %t/%target-library-name(vtables_multifile_testable_helper)
+
+// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
+
+// ... now try to rebuild the client:
+
+// RUN: %target-build-swift %s -L %t -I %t -lvtables_multifile_testable_helper -o %t/main %target-rpath(%t)
+// RUN: %target-codesign %t/main
+
+// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
+
+////
+
+// Delete build artifacts
+// RUN: %empty-directory(%t)
+
+// 3) -enable-testing OFF / -enable-library-evolution ON
+
+// 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
+// RUN: %target-codesign %t/%target-library-name(vtables_multifile_testable_helper)
+
+// RUN: %target-build-swift %s -L %t -I %t -lvtables_multifile_testable_helper -o %t/main %target-rpath(%t)
+// RUN: %target-codesign %t/main
+
+// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
+
+// 4) -enable-testing ON / -enable-library-evolution ON
+
+// ... first without rebuilding the client:
+
+// 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
+// RUN: %target-codesign %t/%target-library-name(vtables_multifile_testable_helper)
+
+// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
+
+// ... now try to rebuild the client:
+
+// RUN: %target-build-swift %s -L %t -I %t -lvtables_multifile_testable_helper -o %t/main %target-rpath(%t)
+// RUN: %target-codesign %t/main
+
+// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
+
+////
+
+// Delete build artifacts
+// RUN: %empty-directory(%t)
+
+// 5) -enable-testing OFF / -enable-library-evolution ON / textual interfaces
+
+// 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
+// RUN: %target-codesign %t/%target-library-name(vtables_multifile_testable_helper)
+
+// RUN: %target-build-swift %s -L %t -I %t -lvtables_multifile_testable_helper -o %t/main %target-rpath(%t)
+// RUN: %target-codesign %t/main
+
+// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
+
+// 6) -enable-testing ON / -enable-library-evolution ON / textual interfaces
+
+// ... first without rebuilding the client:
+
+// 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
+// RUN: %target-codesign %t/%target-library-name(vtables_multifile_testable_helper)
+
+// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
+
+// ... now try to rebuild the client:
+
+// RUN: %target-build-swift %s -L %t -I %t -lvtables_multifile_testable_helper -o %t/main %target-rpath(%t)
+// RUN: %target-codesign %t/main
+
+// RUN: %target-run %t/main %t/%target-library-name(vtables_multifile_testable_helper)
+
+
+// REQUIRES: executable_test
+
+import StdlibUnittest
+import vtables_multifile_testable_helper
+
+var VTableTestSuite = TestSuite("VTable")
+
+public class Derived : Middle {
+  public override func method() -> Int {
+    return super.method() + 1
+  }
+}
+
+VTableTestSuite.test("Derived") {
+  expectEqual(3, callBaseMethod(Derived()))
+  expectEqual(3, callMiddleMethod(Derived()))
+}
+
+runAllTests()
diff --git a/test/SILGen/Inputs/accessibility_vtables_testable_helper.swift b/test/SILGen/Inputs/accessibility_vtables_testable_helper.swift
@@ -0,0 +1,7 @@
+open class Base {
+  internal func method() {}
+}
+
+open class Middle : Base {
+  open override func method() {}
+}
diff --git a/test/SILGen/accessibility_vtables_testable.swift b/test/SILGen/accessibility_vtables_testable.swift
@@ -0,0 +1,49 @@
+// RUN: %empty-directory(%t)
+
+// RUN: %target-swift-frontend -emit-silgen %S/Inputs/accessibility_vtables_testable_helper.swift | %FileCheck %s --check-prefix=LIBRARY
+// RUN: %target-swift-frontend -enable-library-evolution -emit-silgen %S/Inputs/accessibility_vtables_testable_helper.swift | %FileCheck %s --check-prefix=LIBRARY
+// RUN: %target-swift-frontend -emit-silgen %S/Inputs/accessibility_vtables_testable_helper.swift | %FileCheck %s --check-prefix=LIBRARY
+// RUN: %target-swift-frontend -enable-library-evolution -emit-silgen %S/Inputs/accessibility_vtables_testable_helper.swift | %FileCheck %s --check-prefix=LIBRARY
+
+// RUN: %target-swift-frontend -emit-module -o %t %S/Inputs/accessibility_vtables_testable_helper.swift
+// RUN: %target-swift-emit-silgen -primary-file %s -I %t | %FileCheck %s --check-prefix=FRAGILE-CLIENT
+
+// RUN: %target-swift-frontend -emit-module -enable-testing -o %t %S/Inputs/accessibility_vtables_testable_helper.swift
+// RUN: %target-swift-emit-silgen -primary-file %s -I %t | %FileCheck %s --check-prefix=FRAGILE-CLIENT
+
+// RUN: %target-swift-frontend -enable-library-evolution -emit-module -o %t %S/Inputs/accessibility_vtables_testable_helper.swift
+// RUN: %target-swift-emit-silgen -primary-file %s -I %t | %FileCheck %s --check-prefix=RESILIENT-CLIENT
+
+// RUN: %target-swift-frontend -enable-library-evolution -emit-module -enable-testing -o %t %S/Inputs/accessibility_vtables_testable_helper.swift
+// RUN: %target-swift-emit-silgen -primary-file %s -I %t | %FileCheck %s --check-prefix=RESILIENT-CLIENT
+
+import accessibility_vtables_testable_helper
+
+public class Derived : Middle {
+	open override func method() {}
+}
+
+// LIBRARY-LABEL: sil_vtable {{(\[serialized\] )?}}Base {
+// LIBRARY-NEXT:    #Base.method: (Base) -> () -> () : @$s37accessibility_vtables_testable_helper4BaseC6methodyyF
+// LIBRARY-NEXT:    #Base.init!allocator: (Base.Type) -> () -> Base : @$s37accessibility_vtables_testable_helper4BaseCACycfC
+// LIBRARY-NEXT:    #Base.deinit!deallocator: @$s37accessibility_vtables_testable_helper4BaseCfD
+// LIBRARY-NEXT:  }
+
+// LIBRARY-LABEL: sil_vtable {{(\[serialized\] )?}}Middle {
+// LIBRARY-NEXT:    #Base.method: (Base) -> () -> () : @$s37accessibility_vtables_testable_helper6MiddleC6methodyyFAA4BaseCADyyFTV [override]
+// LIBRARY-NEXT:    #Base.init!allocator: (Base.Type) -> () -> Base : @$s37accessibility_vtables_testable_helper6MiddleCACycfC [override]
+// LIBRARY-NEXT:    #Middle.method: (Middle) -> () -> () : @$s37accessibility_vtables_testable_helper6MiddleC6methodyyF
+// LIBRARY-NEXT:    #Middle.deinit!deallocator: @$s37accessibility_vtables_testable_helper6MiddleCfD
+// LIBRARY-NEXT:  }
+
+// FRAGILE-CLIENT-LABEL: sil_vtable [serialized] Derived {
+// FRAGILE-CLIENT-NEXT:    #Base.method: (Base) -> () -> () : @$s37accessibility_vtables_testable_helper6MiddleC6methodyyFAA4BaseCADyyFTV [inherited]
+// FRAGILE-CLIENT-NEXT:    #Base.init!allocator: (Base.Type) -> () -> Base : @$s37accessibility_vtables_testable_helper6MiddleCACycfC [inherited]
+// FRAGILE-CLIENT-NEXT:    #Middle.method: (Middle) -> () -> () : @$s30accessibility_vtables_testable7DerivedC6methodyyF [override]
+// FRAGILE-CLIENT-NEXT:    #Derived.deinit!deallocator: @$s30accessibility_vtables_testable7DerivedCfD
+// FRAGILE-CLIENT-NEXT:  }
+
+// RESILIENT-CLIENT-LABEL: sil_vtable [serialized] Derived {
+// RESILIENT-CLIENT-NEXT:    #Middle.method: (Middle) -> () -> () : @$s30accessibility_vtables_testable7DerivedC6methodyyF [override]	// Derived.method()
+// RESILIENT-CLIENT-NEXT:    #Derived.deinit!deallocator: @$s30accessibility_vtables_testable7DerivedCfD	// Derived.__deallocating_deinit
+// RESILIENT-CLIENT-NEXT:  }
