Merge pull request swiftlang#79173 from swiftlang/elsh/rel/pcmo-implOnly

[6.1][Package CMO] Prevent serializing types from SDK/system modules imported as @_implementationOnly.

Author: elsh

lib/SILOptimizer/IPO/CrossModuleOptimization.cpp

@@ -784,11 +784,26 @@ bool CrossModuleOptimization::checkImports(DeclContext *ctxt) const {
   ModuleDecl::ImportFilter filter;
 
   if (isPackageCMOEnabled(M.getSwiftModule())) {
-    // If Package CMO is enabled, decls imported with `package import`
+    // When Package CMO is enabled, types imported with `package import`
     // or `@_spiOnly import` into this module should be allowed to be
-    // serialized. They are used in decls with `package` or higher
-    // access level, with or without @_spi; a client of this module
-    // should be able to access them directly if in the same package.
+    // serialized. These types may be used in APIs with `package` or
+    // higher access level, with or without `@_spi`, and such APIs should
+    // be serializable to allow direct access by another module if it's
+    // in the same package.
+    //
+    // However, types are from modules imported as `@_implementationOnly`
+    // should not be serialized, even if their defining modules are SDK
+    // or system modules. Since these types are intended to remain hidden
+    // from external clients, their metadata (e.g. field offsets) may be
+    // stripped, making it unavailable for look up at runtime. If serialized,
+    // the client will attempt to use the serialized accessor and fail
+    // because the metadata is missing, leading to a linker error.
+    //
+    // This issue applies to transitively imported types as well;
+    // `@_implementationOnly import Foundation` imports `ObjectiveC`
+    // indirectly, and metadata for types like `NSObject` from `ObjectiveC`
+    // can also be stripped, thus such types should not be allowed for
+    // serialization.
     filter = { ModuleDecl::ImportFilterKind::ImplementationOnly };
   } else {
     // See if context is imported in a "regular" way, i.e. not with
@@ -804,17 +819,8 @@ bool CrossModuleOptimization::checkImports(DeclContext *ctxt) const {
 
   auto &imports = M.getSwiftModule()->getASTContext().getImportCache();
   for (auto &desc : results) {
-    if (imports.isImportedBy(moduleOfCtxt, desc.importedModule)) {
-      // E.g. `@_implementationOnly import QuartzCore_Private.CALayerPrivate`
-      // imports `Foundation` as its transitive dependency module; use of a
-      // a `public` decl in `Foundation` such as `IndexSet` in a function
-      // signature should not block serialization in Package CMO given the
-      // function has `package` or higher access level.
-      if (isPackageCMOEnabled(M.getSwiftModule()) &&
-          moduleOfCtxt->isNonUserModule())
-          continue;
+    if (imports.isImportedBy(moduleOfCtxt, desc.importedModule))
       return false;
-    }
   }
   return true;
 }

test/SILOptimizer/package-cmo-import-filter.swift

@@ -1,6 +1,26 @@
 // RUN: %empty-directory(%t)
 // RUN: split-file %s %t
 
+/// 1. Test `@_implementationOnly import`.
+// RUN: %target-build-swift-dylib(%t/%target-library-name(Utils)) \
+// RUN: %t/UtilsA.swift %t/UtilsB.swift \
+// RUN: -module-name Utils -emit-module -package-name Pkg \
+// RUN: -Xfrontend -experimental-package-cmo -Xfrontend -experimental-allow-non-resilient-access \
+// RUN: -enable-library-evolution -O -wmo
+// RUN: %target-sil-opt %t/Utils.swiftmodule -I %t -sil-verify-all -o %t/Utils.sil
+// RUN: %FileCheck %s --check-prefix=CHECK-UTILS < %t/Utils.sil
+
+/// Verify accessing PkgKlass.second from a client in a dynamic context does not cause a linker error.
+// RUN: %target-build-swift -I %t -L %t %t/Client.swift -package-name Pkg \
+// RUN: -O -wmo -enable-library-evolution \
+// RUN: %target-rpath(%t) -lUtils -o %t/a.out
+
+// RUN: %target-swift-frontend -emit-sil -I %t -L %t %t/Client.swift \
+// RUN: -package-name Pkg -O -wmo -enable-library-evolution \
+// RUN: -lUtils -o %t/Client.sil
+// RUN: %FileCheck %s --check-prefix=CHECK-CLIENT < %t/Client.sil
+
+/// 2. Test `package import` and `@_spiOnly import`.
 // RUN: %target-swift-frontend %t/CoreA.swift \
 // RUN: -module-name=CoreA -package-name Pkg \
 // RUN: -parse-as-library -emit-module \
@@ -13,32 +33,73 @@
 // RUN: -emit-module-path %t/CoreB.swiftmodule -I%t \
 // RUN: -O -wmo -enable-library-evolution
 
-// RUN: %target-swift-frontend %t/Lib.swift \
-// RUN: -module-name=Lib -package-name Pkg \
+// RUN: %target-swift-frontend %t/UI.swift \
+// RUN: -module-name=UI -package-name Pkg \
 // RUN: -parse-as-library -emit-module \
 // RUN: -experimental-spi-only-imports \
-// RUN: -emit-module-path %t/Lib.swiftmodule -I %t \
+// RUN: -emit-module-path %t/UI.swiftmodule -I %t \
 // RUN: -experimental-package-cmo -experimental-allow-non-resilient-access \
-// RUN: -O -wmo -enable-library-evolution -Rmodule-loading 2> %t/Lib-result.txt
-// RUN: %target-sil-opt %t/Lib.swiftmodule -I %t -sil-verify-all -o %t/Lib.sil
-// RUN: %FileCheck %s < %t/Lib.sil
+// RUN: -O -wmo -enable-library-evolution -Rmodule-loading 2> %t/UI-result.txt
+// RUN: %target-sil-opt %t/UI.swiftmodule -I %t -sil-verify-all -o %t/UI.sil
+// RUN: %FileCheck %s < %t/UI.sil
 
 // REQUIRES: swift_in_compiler
+// REQUIRES: OS=macosx || OS=ios || OS=tvos || OS=watchos || OS=maccatalyst
+
+//--- Client.swift
+package import Utils
+
+package func clientFunc<T: PkgKlass>(_ list: [T]) {
+  // closure #1 in clientFunc<A>(_:)
+  // CHECK-CLIENT: sil private @$s6Client10clientFuncyySayxG5Utils8PkgKlassCRbzlFSo8NSObjectCxXEfU_ : $@convention(thin) <T where T : PkgKlass> (@in_guaranteed T) -> (@out NSObject, @error_indirect Never) {
+  // CHECK-CLIENT: class_method {{.*}} #PkgKlass.second!getter : (PkgKlass) -> () -> NSObject, $@convention(method) (@guaranteed PkgKlass) -> @owned NSObject
+  // CHECK-CLIENT: } // end sil function '$s6Client10clientFuncyySayxG5Utils8PkgKlassCRbzlFSo8NSObjectCxXEfU_'
+  let result = list.map { $0.second }
+  print(result)
+}
+
+
+//--- UtilsA.swift
+public import Foundation // public import to allow `NSObject` in API.
 
+package class PkgKlass: NSObject {
+  /// Serialized since it does _not_ reference a type from module imported as @_implementationOnly.
+  // PkgKlass.first.getter
+  // CHECK-UTILS-DAG: sil package [serialized_for_package] [canonical] @$s5Utils8PkgKlassC5firstSSvg : $@convention(method) (@guaranteed PkgKlass) -> @owned String {
+  package var first: String
+
+  /// NOT serialized since it does reference a type from module imported as @_implementationOnly.
+  // PkgKlass.second.getter
+  // CHECK-UTILS-DAG: sil package_external [canonical] @$s5Utils8PkgKlassC6secondSo8NSObjectCvg : $@convention(method) (@guaranteed PkgKlass) -> @owned NSObject
+  @objc package var second: NSObject
+
+  init(first: String, second: NSObject) {
+      self.first = first
+      self.second = second
+  }
+}
+
+//--- UtilsB.swift
+@_implementationOnly import Foundation
+
+public func utilsFunc() {
+  let x: NSString = "utilsfunc"
+  print(x)
+}
 
-//--- Lib.swift
+//--- UI.swift
 package import CoreA
 @_spiOnly public import CoreB
 
 /// PkgStruct is imported with `package import` and should be serialized.
-// CHECK-DAG: sil package [serialized_for_package] [canonical] @$s3Lib7libFuncyy5CoreA9PkgStructVF : $@convention(thin) (@in_guaranteed PkgStruct) -> () {
+// CHECK-DAG: sil package [serialized_for_package] [canonical] @$s2UI7libFuncyy5CoreA9PkgStructVF : $@convention(thin) (@in_guaranteed PkgStruct) -> () {
 package func libFunc(_ arg: PkgStruct) {
   print(arg.pkgVar)
 }
 
 /// PubStruct is imported with `@_spiOnly public import` and should be serialized.
-// CHECK-DAG: sil [serialized_for_package] [canonical] @$s3Lib7spiFuncyy5CoreB15PubStructForSPIVF : $@convention(thin) (@in_guaranteed PubStructForSPI) -> () {
-@_spi(InCoreB)
+// CHECK-DAG: sil [serialized_for_package] [canonical] @$s2UI7spiFuncyy5CoreB15PubStructForSPIVF : $@convention(thin) (@in_guaranteed PubStructForSPI) -> () {
+@_spi(GroupB)
 public func spiFunc(_ arg: PubStructForSPI) {
   print(arg.pubVarForSPI)
 }