From c8d7e94fdd2c8ceb276a6dc363861872f13104ba Mon Sep 17 00:00:00 2001 From: elsh Date: Tue, 24 Sep 2024 13:28:55 -0700 Subject: [PATCH] [PackageCMO] Make binary module work for package-external client. A binary module with PackageCMO includes instructions that are typically disallowed in resilient mode. If the client module belongs to the same package, these instructions can be deserialized and inlined during optimization. However, this must be prevented for clients outside the package, as such instructions are invalid beyond the package domain and could trigger an assertion failure. Resolves rdar://135345358 --- .../swift/Serialization/SerializedSILLoader.h | 1 - lib/Serialization/DeserializeSIL.cpp | 33 +++++ ...-cmo-deserialize-for-external-client.swift | 129 ++++++++++++++++++ 3 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 test/SILOptimizer/package-cmo-deserialize-for-external-client.swift diff --git a/include/swift/Serialization/SerializedSILLoader.h b/include/swift/Serialization/SerializedSILLoader.h index 3545e1403cc11..cbcbadb294412 100644 --- a/include/swift/Serialization/SerializedSILLoader.h +++ b/include/swift/Serialization/SerializedSILLoader.h @@ -47,7 +47,6 @@ class SerializedSILLoader { explicit SerializedSILLoader( ASTContext &ctx, SILModule *SILMod, DeserializationNotificationHandlerSet *callbacks); - public: /// Create a new loader. /// diff --git a/lib/Serialization/DeserializeSIL.cpp b/lib/Serialization/DeserializeSIL.cpp index 785f89541834d..ddc636392c799 100644 --- a/lib/Serialization/DeserializeSIL.cpp +++ b/lib/Serialization/DeserializeSIL.cpp @@ -584,6 +584,25 @@ SILDeserializer::readSILFunctionChecked(DeclID FID, SILFunction *existingFn, if (funcTyID == 0) return MF->diagnoseFatal("SILFunction typeID is 0"); + + // A function with [serialized_for_package] and its body should only be + // deserialized in clients within the same package as its defining module; + // for external clients, it should appear only as a declaration. If the + // function has shared linkage, it must have been internal or private in + // its defining module that's optimized, thus should remain inaccessible + // outside the package boundary. This ensures that instructions, which may + // only be valid in resilient mode when package optimization is enabled, + // aren't inlined at the call site, preventing potential assert fails during + // sil-verify. + if (serializedKind == SerializedKind_t::IsSerializedForPackage) { + if (!SILMod.getSwiftModule()->inSamePackage(getFile()->getParentModule())) { + if (rawLinkage == (unsigned int)(SILLinkage::Shared)) + return nullptr; + if (!declarationOnly) + declarationOnly = true; + } + } + auto astType = MF->getTypeChecked(funcTyID); if (!astType) { if (!existingFn || errorIfEmptyBody) { @@ -3828,6 +3847,13 @@ SILVTable *SILDeserializer::readVTable(DeclID VId) { return nullptr; } + // Vtable with [serialized_for_package], i.e. optimized with Package CMO, + // should only be deserialized into a client if its defning module and the + // client are in the package. + if (!SILMod.getSwiftModule()->inSamePackage(getFile()->getParentModule()) && + SerializedKind_t(Serialized) == SerializedKind_t::IsSerializedForPackage) + return nullptr; + ClassDecl *theClass = cast(MF->getDecl(ClassID)); PrettyStackTraceDecl trace("deserializing SIL vtable for", theClass); @@ -4201,6 +4227,13 @@ llvm::Expected MF->fatal("invalid linkage code"); } + // Witness with [serialized_for_package], i.e. optimized with Package CMO, + // should only be deserialized into a client if its defning module and the + // client are in the package. + if (!SILMod.getSwiftModule()->inSamePackage(getFile()->getParentModule()) && + SerializedKind_t(Serialized) == SerializedKind_t::IsSerializedForPackage) + return nullptr; + // Deserialize Conformance. auto maybeConformance = MF->getConformanceChecked(conformance); if (!maybeConformance) diff --git a/test/SILOptimizer/package-cmo-deserialize-for-external-client.swift b/test/SILOptimizer/package-cmo-deserialize-for-external-client.swift new file mode 100644 index 0000000000000..6646365916ca5 --- /dev/null +++ b/test/SILOptimizer/package-cmo-deserialize-for-external-client.swift @@ -0,0 +1,129 @@ +// RUN: %empty-directory(%t) +// RUN: split-file %s %t + +// RUN: %target-swift-frontend -emit-module %t/Lib.swift -I %t \ +// RUN: -module-name Lib -package-name pkg \ +// RUN: -enable-library-evolution -swift-version 5 \ +// RUN: -O -wmo -experimental-allow-non-resilient-access -experimental-package-cmo \ +// RUN: -emit-module -emit-module-path %t/Lib.swiftmodule +// RUN: %target-sil-opt %t/Lib.swiftmodule -o %t/Lib.sil + +// RUN: %target-swift-frontend -emit-sil -O %t/Client.swift -I %t -package-name pkg \ +// RUN: -Xllvm -sil-disable-pass=DeadFunctionAndGlobalElimination -o %t/InPkgClient.sil + +// RUN: %target-swift-frontend -emit-sil -O %t/Client.swift -I %t \ +// RUN: -Xllvm -sil-disable-pass=DeadFunctionAndGlobalElimination -o %t/ExtClient.sil + +/// TEST how functions (and bodies) and tables optimized with [serialized_for_package] in Lib are +/// deserialized into Client depending on package boundary. +/// +/// Test 1: They should be deserialized into Client as Lib and Client are in the same package. +// RUN: %FileCheck -check-prefix=CHECK-INPKG %s < %t/InPkgClient.sil + +// Pub.init(_:) is removed after getting lined below. +// CHECK-INPKG-NOT: @$s3Lib3PubCyACSicfc + +// Pub.__allocating_init(_:) +// CHECK-INPKG: sil public_external @$s3Lib3PubCyACSicfC : $@convention(method) (Int, @thick Pub.Type) -> @owned Pub { +// Pub.init(_:) is inlined in this block, as its body was deserialized. +// CHECK-INPKG: ref_element_addr {{.*}} : $Pub, #Pub.pubVar + +// CHECK-INPKG: sil_vtable Pub { +// CHECK-INPKG: #Pub.pubVar!getter: (Pub) -> () -> Int : @$s3Lib3PubC6pubVarSivg // Pub.pubVar.getter +// CHECK-INPKG: #Pub.pubVar!setter: (Pub) -> (Int) -> () : @$s3Lib3PubC6pubVarSivs // Pub.pubVar.setter +// CHECK-INPKG: #Pub.pubVar!modify: (Pub) -> () -> () : @$s3Lib3PubC6pubVarSivM // Pub.pubVar.modify +// CHECK-INPKG: #Pub.pkgVar!getter: (Pub) -> () -> Int : @$s3Lib3PubC6pkgVarSivg // Pub.pkgVar.getter +// CHECK-INPKG: #Pub.pkgVar!setter: (Pub) -> (Int) -> () : @$s3Lib3PubC6pkgVarSivs // Pub.pkgVar.setter +// CHECK-INPKG: #Pub.pkgVar!modify: (Pub) -> () -> () : @$s3Lib3PubC6pkgVarSivM // Pub.pkgVar.modify +// CHECK-INPKG: #Pub.init!allocator: (Pub.Type) -> (Int) -> Pub : @$s3Lib3PubCyACSicfC // Pub.__allocating_init(_:) +// CHECK-INPKG: #Pub.deinit!deallocator: @$s3Lib3PubCfD // Pub.__deallocating_deinit + +// CHECK-INPKG: sil_witness_table public_external Pub: PubProto module Lib { +// CHECK-INPKG: method #PubProto.pubVar!getter: (Self) -> () -> Int : @$s3Lib3PubCAA0B5ProtoA2aDP6pubVarSivgTW // protocol witness for PubProto.pubVar.getter in conformance Pub +// CHECK-INPKG: method #PubProto.pubVar!setter: (inout Self) -> (Int) -> () : @$s3Lib3PubCAA0B5ProtoA2aDP6pubVarSivsTW // protocol witness for PubProto.pubVar.setter in conformance Pub +// CHECK-INPKG: method #PubProto.pubVar!modify: (inout Self) -> () -> () : @$s3Lib3PubCAA0B5ProtoA2aDP6pubVarSivMTW // protocol witness for PubProto.pubVar.modify in conformance Pub + + +/// Test 2: They should NOT be deserialized into Client as Lib and Client are NOT in the same package; +/// only declaration should be deserialized, not the body. +// RUN: %FileCheck -check-prefix=CHECK-EXT %s < %t/ExtClient.sil + +// CHECK-EXT-NOT: sil_vtable Pub { +// CHECK-EXT-NOT: #Pub.pubVar!getter: (Pub) -> () -> Int : @$s3Lib3PubC6pubVarSivg // Pub.pubVar.getter +// CHECK-EXT-NOT: #Pub.pubVar!setter: (Pub) -> (Int) -> () : @$s3Lib3PubC6pubVarSivs // Pub.pubVar.setter +// CHECK-EXT-NOT: #Pub.pubVar!modify: (Pub) -> () -> () : @$s3Lib3PubC6pubVarSivM // Pub.pubVar.modify +// CHECK-EXT-NOT: #Pub.pkgVar!getter: (Pub) -> () -> Int : @$s3Lib3PubC6pkgVarSivg // Pub.pkgVar.getter +// CHECK-EXT-NOT: #Pub.pkgVar!setter: (Pub) -> (Int) -> () : @$s3Lib3PubC6pkgVarSivs // Pub.pkgVar.setter +// CHECK-EXT-NOT: #Pub.pkgVar!modify: (Pub) -> () -> () : @$s3Lib3PubC6pkgVarSivM // Pub.pkgVar.modify +// CHECK-EXT-NOT: #Pub.init!allocator: (Pub.Type) -> (Int) -> Pub : @$s3Lib3PubCyACSicfC // Pub.__allocating_init(_:) +// CHECK-EXT-NOT: #Pub.deinit!deallocator: @$s3Lib3PubCfD // Pub.__deallocating_deinit + +// CHECK-EXT-NOT: sil_witness_table public_external Pub: PubProto module Lib { +// CHECK-EXT-NOT: method #PubProto.pubVar!getter: (Self) -> () -> Int : @$s3Lib3PubCAA0B5ProtoA2aDP6pubVarSivgTW // protocol witness for PubProto.pubVar.getter in conformance Pub +// CHECK-EXT-NOT: method #PubProto.pubVar!setter: (inout Self) -> (Int) -> () : @$s3Lib3PubCAA0B5ProtoA2aDP6pubVarSivsTW // protocol witness for PubProto.pubVar.setter in conformance Pub +// CHECK-EXT-NOT: method #PubProto.pubVar!modify: (inout Self) -> () -> () : @$s3Lib3PubCAA0B5ProtoA2aDP6pubVarSivMTW // protocol witness for PubProto.pubVar.modify in conformance Pub + +// Pub.init(_:) is just a decl; does not contain the body. +// CHECK-EXT: sil @$s3Lib3PubCyACSicfc : $@convention(method) (Int, @owned Pub) -> @owned Pub + + +/// Verify functions and tables are optimized with Package CMO in Lib. +// RUN: %FileCheck -check-prefix=CHECK-LIB %s < %t/Lib.sil + +// CHECK-LIB: sil [serialized] [exact_self_class] [canonical] @$s3Lib3PubCyACSicfC : $@convention(method) (Int, @thick Pub.Type) -> @owned Pub { +// CHECK-LIB: function_ref @$s3Lib3PubCyACSicfc : $@convention(method) (Int, @owned Pub) -> @owned Pub +// Pub.init(_:) + +// CHECK-LIB: sil [serialized_for_package] [canonical] @$s3Lib3PubCyACSicfc : $@convention(method) (Int, @owned Pub) -> @owned Pub { +// CHECK-LIB: ref_element_addr {{.*}} : $Pub, #Pub.pubVar +// Pub.__allocating_init(_:) + +// Pub.pkgVar.getter +// CHECK-LIB: sil package [serialized_for_package] [canonical] @$s3Lib3PubC6pkgVarSivg : $@convention(method) (@guaranteed Pub) -> Int { + +// CHECK-LIB: sil_vtable [serialized_for_package] Pub { +// CHECK-LIB: #Pub.pubVar!getter: (Pub) -> () -> Int : @$s3Lib3PubC6pubVarSivg // Pub.pubVar.getter +// CHECK-LIB: #Pub.pubVar!setter: (Pub) -> (Int) -> () : @$s3Lib3PubC6pubVarSivs // Pub.pubVar.setter +// CHECK-LIB: #Pub.pubVar!modify: (Pub) -> () -> () : @$s3Lib3PubC6pubVarSivM // Pub.pubVar.modify +// CHECK-LIB: #Pub.pkgVar!getter: (Pub) -> () -> Int : @$s3Lib3PubC6pkgVarSivg // Pub.pkgVar.getter +// CHECK-LIB: #Pub.pkgVar!setter: (Pub) -> (Int) -> () : @$s3Lib3PubC6pkgVarSivs // Pub.pkgVar.setter +// CHECK-LIB: #Pub.pkgVar!modify: (Pub) -> () -> () : @$s3Lib3PubC6pkgVarSivM // Pub.pkgVar.modify +// CHECK-LIB: #Pub.init!allocator: (Pub.Type) -> (Int) -> Pub : @$s3Lib3PubCyACSicfC // Pub.__allocating_init(_:) +// CHECK-LIB: #Pub.deinit!deallocator: @$s3Lib3PubCfD // Pub.__deallocating_deinit + +// CHECK-LIB: sil_witness_table [serialized_for_package] Pub: PubProto module Lib { +// CHECK-LIB: method #PubProto.pubVar!getter: (Self) -> () -> Int : @$s3Lib3PubCAA0B5ProtoA2aDP6pubVarSivgTW // protocol witness for PubProto.pubVar.getter in conformance Pub +// CHECK-LIB: method #PubProto.pubVar!setter: (inout Self) -> (Int) -> () : @$s3Lib3PubCAA0B5ProtoA2aDP6pubVarSivsTW // protocol witness for PubProto.pubVar.setter in conformance Pub +// CHECK-LIB: method #PubProto.pubVar!modify: (inout Self) -> () -> () : @$s3Lib3PubCAA0B5ProtoA2aDP6pubVarSivMTW // protocol witness for PubProto.pubVar.modify in conformance Pub + + +//--- Lib.swift + +public protocol PubProto { + var pubVar: Int { get set } +} + +public class Pub: PubProto { + public var pubVar: Int = 1 + package var pkgVar: Int = 2 + public init(_ arg: Int) { + pubVar = arg + pkgVar = arg + } +} + +//--- Client.swift +import Lib + +public func test(_ arg: Int) -> Int { + let x = Pub(arg) + x.pubVar = 3 + return x.run() +} + +public extension PubProto { + func run() -> Int { + return pubVar + } +} +