From 9e5e2407047a7a5a7f5cf2f24de59ed36bbe305a Mon Sep 17 00:00:00 2001 From: Joe Groff Date: Wed, 19 Mar 2025 11:02:38 -0700 Subject: [PATCH] SILGen: Fix inconsistency between vtable layout and vtable entry emission. As part of the criteria to determine whether a method override requires a new vtable entry in addition to overriding the base class's vtable entry for the method, we compare the visibility of the base class to that of the derived class, since if further subclasses of the derived class cannot see the original base class's method descriptor, they would not be able to directly override its vtable entry. However, there was a subtle inconsistency in our checks in two different places: `NeedsNewVTableEntryRequest::evaluate` compared the visibility of the derived method to its immediate override method, whereas `SILGenModule::emitVTableMethod` compared visibility with the ultimate base method that originated the vtable entry in question. Internal imports create a situation where this leads to inconsistent answers, causing us to emit a vtable thunk but without a corresponding new vtable entry, causing the thunk to become self-recursive (rdar://147360093). If the base class is in a separate module: ``` // Module1 open class Base { public required init() {} } ``` and an intermediate class inherits that base class, it must `public import` the module in the file defining the class to do so: ``` // Module2 public import Module1 open class MidDerived: Base { } ``` However, another file in the same module can further inherit that class, but with only an `internal import` of the base module: ``` // Also Module2 import Module1 open class MostDerived: MidDerived { } ``` The implicit `init()` is `public` in each class, but from the perspective of `MostDerived.init`, `Base.init` is `internal` because of the import. However, the fact that its immediate parent is `public` should be sufficient evidence that its original definition had visibility to `Base`, so no thunk should be necessary. --- lib/SILGen/SILGenType.cpp | 10 +++++++++- .../Repro1.swift | 3 +++ .../Repro2.swift | 4 ++++ .../vtable_internal_imported_ancestor.swift | 14 ++++++++++++++ test/SILGen/vtables_multifile.swift | 16 ++++++++-------- 5 files changed, 38 insertions(+), 9 deletions(-) create mode 100644 test/SILGen/Inputs/vtable_internal_imported_ancestor/Repro1.swift create mode 100644 test/SILGen/Inputs/vtable_internal_imported_ancestor/Repro2.swift create mode 100644 test/SILGen/vtable_internal_imported_ancestor.swift diff --git a/lib/SILGen/SILGenType.cpp b/lib/SILGen/SILGenType.cpp index 306ee8fe71a1b..c612fa6583697 100644 --- a/lib/SILGen/SILGenType.cpp +++ b/lib/SILGen/SILGenType.cpp @@ -112,10 +112,18 @@ SILGenModule::emitVTableMethod(ClassDecl *theClass, SILDeclRef derived, // If the base method is less visible than the derived method, we need // a thunk. + // + // Note that we check the visibility of the derived method's immediate base + // here, rather than the ultimate base of the vtable entry, because it is + // possible through import visibility for an intermediate base class in one + // file to have public visibility to the ultimate base via a public import, + // but then in turn be overridden by a derived class in another file in + // the same module that either doesn't import the ultimate base class's + // module or else imports it non-publicly in its file. bool baseLessVisibleThanDerived = (!usesObjCDynamicDispatch && !derivedDecl->isFinal() && - derivedDecl->isMoreVisibleThan(baseDecl)); + derivedDecl->isMoreVisibleThan(derivedDecl->getOverriddenDecl())); // Determine the derived thunk type by lowering the derived type against the // abstraction pattern of the base. diff --git a/test/SILGen/Inputs/vtable_internal_imported_ancestor/Repro1.swift b/test/SILGen/Inputs/vtable_internal_imported_ancestor/Repro1.swift new file mode 100644 index 0000000000000..251661de9a930 --- /dev/null +++ b/test/SILGen/Inputs/vtable_internal_imported_ancestor/Repro1.swift @@ -0,0 +1,3 @@ +open class Base { + public required init() {} +} diff --git a/test/SILGen/Inputs/vtable_internal_imported_ancestor/Repro2.swift b/test/SILGen/Inputs/vtable_internal_imported_ancestor/Repro2.swift new file mode 100644 index 0000000000000..387e4e007abe4 --- /dev/null +++ b/test/SILGen/Inputs/vtable_internal_imported_ancestor/Repro2.swift @@ -0,0 +1,4 @@ +public import Repro1 + +open class MidDerived: Base { +} diff --git a/test/SILGen/vtable_internal_imported_ancestor.swift b/test/SILGen/vtable_internal_imported_ancestor.swift new file mode 100644 index 0000000000000..0adafa0c605b7 --- /dev/null +++ b/test/SILGen/vtable_internal_imported_ancestor.swift @@ -0,0 +1,14 @@ +// RUN: %empty-directory(%t) +// RUN: %target-swift-frontend -enable-library-evolution -emit-module-path %t/Repro1.swiftmodule -module-name Repro1 %S/Inputs/vtable_internal_imported_ancestor/Repro1.swift +// RUN: %target-swift-frontend -enable-upcoming-feature InternalImportsByDefault -I %t -emit-silgen %S/Inputs/vtable_internal_imported_ancestor/Repro2.swift -primary-file %s | %FileCheck %s + +// REQUIRES: swift_feature_InternalImportsByDefault + +import Repro1 + +public class MostDerived: MidDerived { +} + +// CHECK-NOT: vtable thunk +// CHECK-LABEL: sil_vtable{{.*}} MostDerived { +// CHECK: #Base.init!allocator: {{.*}} @$s6Repro211MostDerivedCACycfC [override] diff --git a/test/SILGen/vtables_multifile.swift b/test/SILGen/vtables_multifile.swift index 6423542a88071..e7bc1f534c419 100644 --- a/test/SILGen/vtables_multifile.swift +++ b/test/SILGen/vtables_multifile.swift @@ -225,15 +225,15 @@ public final class FinalDerived : Base { // -- // CHECK-LABEL: sil_vtable [serialized] MostDerived { -// CHECK-NEXT: #Base.privateMethod1: (Base) -> () -> () : @$s17vtables_multifile11MostDerivedC14privateMethod1yyFAA4BaseCAD33_63E5F2521A3C787F5F9EFD57FB9237EALLyyFTV [override] // vtable thunk for Base.privateMethod1() dispatching to MostDerived.privateMethod1() -// CHECK-NEXT: #Base.privateMethod2: (Base) -> (AnyObject) -> () : @$s17vtables_multifile11MostDerivedC14privateMethod2yyyXlSgFAA4BaseCAD33_63E5F2521A3C787F5F9EFD57FB9237EALLyyyXlFTV [override] // vtable thunk for Base.privateMethod2(_:) dispatching to MostDerived.privateMethod2(_:) -// CHECK-NEXT: #Base.privateMethod3: (Base) -> (Int) -> () : @$s17vtables_multifile11MostDerivedC14privateMethod3yySiSgFAA4BaseCAD33_63E5F2521A3C787F5F9EFD57FB9237EALLyySiFTV [override] // vtable thunk for Base.privateMethod3(_:) dispatching to MostDerived.privateMethod3(_:) -// CHECK-NEXT: #Base.privateMethod4: (Base) -> (T) -> () : @$s17vtables_multifile11MostDerivedC14privateMethod4yySiFAA4BaseCAD33_63E5F2521A3C787F5F9EFD57FB9237EALLyyxFTV [override] // vtable thunk for Base.privateMethod4(_:) dispatching to MostDerived.privateMethod4(_:) +// CHECK-NEXT: #Base.privateMethod1: (Base) -> () -> () : @$s17vtables_multifile11MostDerivedC14privateMethod1yyF +// CHECK-NEXT: #Base.privateMethod2: (Base) -> (AnyObject) -> () : @$s17vtables_multifile11MostDerivedC14privateMethod2yyyXlSgF +// CHECK-NEXT: #Base.privateMethod3: (Base) -> (Int) -> () : @$s17vtables_multifile11MostDerivedC14privateMethod3yySiSgF +// CHECK-NEXT: #Base.privateMethod4: (Base) -> (T) -> () : @$s17vtables_multifile11MostDerivedC14privateMethod4yySiF // CHECK-NEXT: #Base.init!allocator: (Base.Type) -> () -> Base : @$s17vtables_multifile11MostDerivedCACycfC [override] // MostDerived.__allocating_init() -// CHECK-NEXT: #Derived.privateMethod1: (Derived) -> () -> () : @$s17vtables_multifile11MostDerivedC14privateMethod1yyFAA0D0CADyyFTV [override] // vtable thunk for Derived.privateMethod1() dispatching to MostDerived.privateMethod1() -// CHECK-NEXT: #Derived.privateMethod2: (Derived) -> (AnyObject?) -> () : @$s17vtables_multifile11MostDerivedC14privateMethod2yyyXlSgFAA0D0CADyyAEFTV [override] // vtable thunk for Derived.privateMethod2(_:) dispatching to MostDerived.privateMethod2(_:) -// CHECK-NEXT: #Derived.privateMethod3: (Derived) -> (Int?) -> () : @$s17vtables_multifile11MostDerivedC14privateMethod3yySiSgFAA0D0CADyyAEFTV [override] // vtable thunk for Derived.privateMethod3(_:) dispatching to MostDerived.privateMethod3(_:) -// CHECK-NEXT: #Derived.privateMethod4: (Derived) -> (Int) -> () : @$s17vtables_multifile11MostDerivedC14privateMethod4yySiFAA0D0CADyySiFTV [override] // vtable thunk for Derived.privateMethod4(_:) dispatching to MostDerived.privateMethod4(_:) +// CHECK-NEXT: #Derived.privateMethod1: (Derived) -> () -> () : @$s17vtables_multifile11MostDerivedC14privateMethod1yyF +// CHECK-NEXT: #Derived.privateMethod2: (Derived) -> (AnyObject?) -> () : @$s17vtables_multifile11MostDerivedC14privateMethod2yyyXlSgF +// CHECK-NEXT: #Derived.privateMethod3: (Derived) -> (Int?) -> () : @$s17vtables_multifile11MostDerivedC14privateMethod3yySiSgF +// CHECK-NEXT: #Derived.privateMethod4: (Derived) -> (Int) -> () : @$s17vtables_multifile11MostDerivedC14privateMethod4yySiF // CHECK-NEXT: #MoreDerived.privateMethod1: (MoreDerived) -> () -> () : @$s17vtables_multifile11MostDerivedC14privateMethod1yyF [override] // MostDerived.privateMethod1() // CHECK-NEXT: #MoreDerived.privateMethod2: (MoreDerived) -> (AnyObject?) -> () : @$s17vtables_multifile11MostDerivedC14privateMethod2yyyXlSgF [override] // MostDerived.privateMethod2(_:) // CHECK-NEXT: #MoreDerived.privateMethod3: (MoreDerived) -> (Int?) -> () : @$s17vtables_multifile11MostDerivedC14privateMethod3yySiSgF [override] // MostDerived.privateMethod3(_:)