From dc6a4bfd90d93dae315a74bad87b91b810f772ab Mon Sep 17 00:00:00 2001 From: elsh Date: Fri, 17 Jan 2025 18:03:51 -0800 Subject: [PATCH] PackageCMO: fix serializability check for keypath When a keypath instruction was checked for serializability, its referenced function was sometimes incorrectly deemed serializable when its referenced method had package or public access level. This resulted in incorrectly serializing a function that dynamically accesses a property on a generic type using key path. This PR fixes the issue by skipping the access level check if the referenced function is determined to be un-serializable. Resolves rdar://142950306 --- .../IPO/CrossModuleOptimization.cpp | 2 + .../package-cmo-cast-internal-dst | 29 ----- .../package-cmo-check-cast-dst.swift | 123 ++++++++++++++++++ test/SILOptimizer/package-cmo-keypath.swift | 61 +++++++++ 4 files changed, 186 insertions(+), 29 deletions(-) delete mode 100644 test/SILOptimizer/package-cmo-cast-internal-dst create mode 100644 test/SILOptimizer/package-cmo-check-cast-dst.swift create mode 100644 test/SILOptimizer/package-cmo-keypath.swift diff --git a/lib/SILOptimizer/IPO/CrossModuleOptimization.cpp b/lib/SILOptimizer/IPO/CrossModuleOptimization.cpp index 03d4917f93f13..7a3dbcc5cfea4 100644 --- a/lib/SILOptimizer/IPO/CrossModuleOptimization.cpp +++ b/lib/SILOptimizer/IPO/CrossModuleOptimization.cpp @@ -623,6 +623,8 @@ bool CrossModuleOptimization::canSerializeFieldsByInstructionKind( canUse = false; }, [&](SILDeclRef method) { + if (!canUse) // If already set to false in the above lambda, return + return; if (method.isForeign) canUse = false; else if (isPackageCMOEnabled(method.getModuleContext())) { diff --git a/test/SILOptimizer/package-cmo-cast-internal-dst b/test/SILOptimizer/package-cmo-cast-internal-dst deleted file mode 100644 index 2281532f4aaa0..0000000000000 --- a/test/SILOptimizer/package-cmo-cast-internal-dst +++ /dev/null @@ -1,29 +0,0 @@ -// RUN: %empty-directory(%t) -// RUN: split-file %s %t - -// RUN: %target-swift-frontend -emit-sil %t/Lib.swift -package-name pkg \ -// RUN: -wmo -allow-non-resilient-access -package-cmo \ -// RUN: -enable-library-evolution -swift-version 5 \ -// RUN: -Xllvm -sil-print-function=topFunc -o %t/Lib.sil - -// RUN: %FileCheck %s < %t/Lib.sil - -/// Verify that `InternalKlass` is visited and the instruction containing it is not serialized. -// CHECK: sil @$s3Lib7topFuncySiAA3PubCF : $@convention(thin) (@guaranteed Pub) -> Int { -// CHECK: checked_cast_br Pub in %0 : $Pub to InternalKlass - - -//--- Lib.swift -public class Pub { - public var pubVar: Int - public init(_ arg: Int) { - pubVar = arg - } -} - -class InternalKlass: Pub {} - -public func topFunc(_ arg: Pub) -> Int { - let x = arg as? InternalKlass - return x != nil ? 1 : 0 -} diff --git a/test/SILOptimizer/package-cmo-check-cast-dst.swift b/test/SILOptimizer/package-cmo-check-cast-dst.swift new file mode 100644 index 0000000000000..87b52ab00f5e5 --- /dev/null +++ b/test/SILOptimizer/package-cmo-check-cast-dst.swift @@ -0,0 +1,123 @@ +// RUN: %empty-directory(%t) +// RUN: split-file %s %t + +/// Verify a function with a cast instruction with an internal decl as dst +/// does not get serialized with PackageCMO in both scenarios below. + +/// Scenario 1. +// RUN: %target-swift-frontend -emit-sil %t/Lib.swift -package-name pkg \ +// RUN: -wmo -allow-non-resilient-access -package-cmo \ +// RUN: -enable-library-evolution -swift-version 5 \ +// RUN: -Xllvm -sil-print-function=topFunc -o %t/Lib.sil +// RUN: %FileCheck %s < %t/Lib.sil + +/// Scenario 2. +// RUN: %target-build-swift %t/Mod.swift \ +// RUN: -module-name=Mod -package-name pkg \ +// RUN: -parse-as-library -emit-module -emit-module-path %t/Mod.swiftmodule -I%t \ +// RUN: -Xfrontend -experimental-package-cmo -Xfrontend -experimental-allow-non-resilient-access \ +// RUN: -O -wmo -enable-library-evolution +// RUN: %target-sil-opt -enable-sil-verify-all %t/Mod.swiftmodule -o %t/Mod.sil +// RUN: %FileCheck %s --check-prefix=CHECK-MOD < %t/Mod.sil + +//--- Lib.swift +public class Pub { + public var pubVar: Int + public init(_ arg: Int) { + pubVar = arg + } +} + +class InternalKlass: Pub {} + +/// Verify that `InternalKlass` is visited and the instruction containing it is not serialized. +// CHECK: sil @$s3Lib7topFuncySiAA3PubCF : $@convention(thin) (@guaranteed Pub) -> Int { +// CHECK: checked_cast_br Pub in {{.*}} to InternalKlass +public func topFunc(_ arg: Pub) -> Int { + let x = arg as? InternalKlass + return x != nil ? 1 : 0 +} + + +//--- Mod.swift +struct SymmetricTextChildQuery { + var source: Text + init(_ arg: Text) { + source = arg + } + /// This function references isCollapsible(), which contains an internal decl. + /// If isCollapsible() were serialized, building a client of this module would fail + /// due to a linker error: undefined symbol `CollapsibleTextModifier`. + mutating func updateValue() { + let resolvedSource = ResolvedStyledText.styledText(canCollapse: source.isCollapsible()) + } +} + +@frozen +public struct Text: Equatable, Sendable { + public init() {} + @frozen + @usableFromInline + package enum Modifier: Equatable { + case font + case anyTextModifier(AnyTextModifier) + + @usableFromInline + package static func ==(lhs: Modifier, rhs: Modifier) -> Bool { + return true + } + } + + @usableFromInline + package var modifiers = [Modifier]() + +} + +extension Text { + /// Verify that function containing an internal decl CollapsibleTextModifier is + /// not serialized with Package CMO. + // CHECK-MOD-NOT: sil package [serialized_for_package] [canonical] @$s3Mod4TextV13isCollapsibleSbyF : $@convention(method) (@guaranteed Text) -> Bool { + // CHECK-MOD-NOT: checked_cast_br AnyTextModifier in {{.*}} : $AnyTextModifier to CollapsibleTextModifier + package func isCollapsible() -> Bool { + modifiers.contains { modifier in + guard case .anyTextModifier(let anyModifier) = modifier + else { return false } + return anyModifier is CollapsibleTextModifier + } + } +} + +final class CollapsibleTextModifier: AnyTextModifier { + override func isEqual(to other: AnyTextModifier) -> Bool { + other is CollapsibleTextModifier + } +} + +public protocol PubProto { + var pubVar: String { get set } +} + +public struct PubStruct { + public static func makeView(_ type: P.Type, _ arg: Text) { + var child = SymmetricTextChildQuery

(arg) + child.updateValue() + } +} + +public class ResolvedStyledText { + package var canCollapse: Bool + package init(_ arg: Bool) { + canCollapse = arg + } +} + +extension ResolvedStyledText { + package static func styledText(canCollapse: Bool) -> ResolvedStyledText { + return ResolvedStyledText(canCollapse) + } +} + +@usableFromInline +package class AnyTextModifier { + func isEqual(to other: AnyTextModifier) -> Bool { fatalError() } +} diff --git a/test/SILOptimizer/package-cmo-keypath.swift b/test/SILOptimizer/package-cmo-keypath.swift new file mode 100644 index 0000000000000..53f090d7344ea --- /dev/null +++ b/test/SILOptimizer/package-cmo-keypath.swift @@ -0,0 +1,61 @@ +// RUN: %empty-directory(%t) +// RUN: split-file %s %t + +// RUN: %target-build-swift %t/Lib.swift \ +// RUN: -module-name=Lib -package-name Pkg \ +// RUN: -parse-as-library -emit-module -emit-module-path %t/Lib.swiftmodule -I%t \ +// RUN: -Xfrontend -experimental-package-cmo -Xfrontend -experimental-allow-non-resilient-access \ +// RUN: -O -wmo -enable-library-evolution +// RUN: %target-sil-opt -enable-sil-verify-all %t/Lib.swiftmodule -o %t/Lib.sil + +// RUN: %FileCheck %s < %t/Lib.sil + +// REQUIRES: swift_in_compiler + +//--- Lib.swift +package protocol PkgProto { + /// Key path getter for a protocol property has a shared linkage as below, and a function referencing + /// this getter (as well as this getter) should not be serialized in Package CMO, even if the `witness_method` + /// in this getter has package or public access level. + // key path getter for PkgProto.pkgVar : A + // sil shared [thunk] @$s3Lib8PkgProtoP6pkgVarSSvpAaBRzlxTK : $@convention(keypath_accessor_getter) (@in_guaranteed T) -> @out String { + // [%0: noescape **, write v**] + // [%1: read v**, write v**, copy v**, destroy v**] + // [global: read,write,copy,destroy,allocate,deinit_barrier] + // // %0 // user: %4 + // // %1 // user: %3 + // bb0(%0 : $*String, %1 : $*T): + // %2 = witness_method $T, #PkgProto.pkgVar!getter : (Self) -> () -> String : $@convention(witness_method: PkgProto) <τ_0_0 where τ_0_0 : PkgProto> (@in_guaranteed τ_0_0) -> @owned String // user: %3 + // %3 = apply %2(%1) : $@convention(witness_method: PkgProto) <τ_0_0 where τ_0_0 : PkgProto> (@in_guaranteed τ_0_0) -> @owned String // user: %4 + // store %3 to %0 : $*String // id: %4 + // %5 = tuple () // user: %6 + // return %5 : $() // id: %6 + // } // end sil function '$s3Lib8PkgProtoP6pkgVarSSvpAaBRzlxTK' + var pkgVar: String { get } + + // key path getter for PkgProto.pkgVar : A + // CHECK-NOT: sil [serialized_for_package] [thunk] [canonical] @$s3Lib8PkgProtoP6pkgVarSSvpAaBRzlxTK : $@convention(keypath_accessor_getter) (@in_guaranteed T) -> @out String { + // CHECK-NOT: witness_method $T, #PkgProto.pkgVar!getter : (Self) -> () -> String : $@convention(witness_method: PkgProto) <τ_0_0 where τ_0_0 : PkgProto> (@in_guaranteed τ_0_0) -> @owned String // user: %3 + // CHECK-NOT: // end sil function '$s3Lib8PkgProtoP6pkgVarSSvpAaBRzlxTK' +} + +package struct Foo: PkgProto { + package var pkgVar: String { + return "Foo pkgVar" + } +} + +/// testKeyPath dynamically accesses pkgVar property on a generic type using keypath, +/// referencing the getter above, s3Lib8PkgProtoP6pkgVarSSvpAaBRzlxTK, +/// and should not be serialized. +// testKeyPath(_:) +// CHECK-NOT: sil package [serialized_for_package] [canonical] @$s3Lib11testKeyPathys0cD0CyxSSGSayxGAA8PkgProtoRzlF : $@convention(thin) (@guaranteed Array) -> @owned KeyPath { +// CHECK-NOT: keypath $KeyPath, <τ_0_0 where τ_0_0 : PkgProto> (root $τ_0_0; gettable_property $String, id #PkgProto.pkgVar!getter : (Self) -> () -> String, getter @$s3Lib8PkgProtoP6pkgVarSSvpAaBRzlxTK : $@convention(keypath_accessor_getter) <τ_0_0 where τ_0_0 : PkgProto> (@in_guaranteed τ_0_0) -> @out String) +// CHECK-NOT: } // end sil function '$s3Lib11testKeyPathys0cD0CyxSSGSayxGAA8PkgProtoRzlF' +package func testKeyPath(_ array: [T]) -> KeyPath { + return \T.pkgVar +} + +// CHECK: sil_witness_table package [serialized_for_package] Foo: PkgProto module Lib { +// CHECK: method #PkgProto.pkgVar!getter: (Self) -> () -> String : @$s3Lib3FooVAA8PkgProtoA2aDP6pkgVarSSvgTW +