Merge pull request #79157 from swiftlang/elsh/pcmo-bypass-res-check-mods

Package CMO: Skip deserialization error checks for same-module decls.

Author: elsh

include/swift/AST/DiagnosticsSema.def

@@ -4745,7 +4745,10 @@ NOTE(ambiguous_because_of_trailing_closure,none,
      (bool, const ValueDecl *))
 
 // In-package resilience bypassing
-WARNING(cannot_bypass_resilience_due_to_missing_member,none,
+WARNING(cannot_bypass_resilience_due_to_missing_member_warn,none,
+      "cannot bypass resilience due to member deserialization failure while attempting to access %select{member %0|missing member}1 of %2 in module %3 from module %4",
+      (Identifier, bool, Identifier, Identifier, Identifier))
+ERROR(cannot_bypass_resilience_due_to_missing_member,none,
       "cannot bypass resilience due to member deserialization failure while attempting to access %select{member %0|missing member}1 of %2 in module %3 from module %4",
       (Identifier, bool, Identifier, Identifier, Identifier))
 

include/swift/Basic/LangOptions.h

@@ -603,12 +603,11 @@ namespace swift {
     /// from source.
     bool AllowNonResilientAccess = false;
 
-    /// When Package CMO is enabled, deserialization checks are done to
-    /// ensure that the members of a decl are correctly deserialized to maintain
-    /// proper layout. This ensures that bypassing resilience is safe. Accessing
-    /// an incorrectly laid-out decl directly can lead to runtime crashes. This flag
-    /// should only be used temporarily during migration to enable Package CMO.
-    bool SkipDeserializationChecksForPackageCMO = false;
+    /// When Package CMO is enabled, deserialization checks ensure that a decl's
+    /// members are correctly deserialized to maintain the proper layout—a prerequisite
+    /// for bypassing resilience when accessing the decl. By default, a warning is issued
+    /// if a deserialization failure is found; this flag causes the build to fail fast instead.
+    bool AbortOnDeserializationFailForPackageCMO = false;
 
     /// Enables dumping type witness systems from associated type inference.
     bool DumpTypeWitnessSystems = false;

include/swift/Option/Options.td

@@ -1139,9 +1139,9 @@ def Oplayground : Flag<["-"], "Oplayground">, Group<O_Group>,
   Flags<[HelpHidden, FrontendOption, ModuleInterfaceOption]>,
   HelpText<"Compile with optimizations appropriate for a playground">;
 
-def ExperimentalSkipDeserializationChecksForPackageCMO : Flag<["-"], "experimental-skip-deserialization-checks-for-package-cmo">,
+def ExperimentalPackageCMOAbortOnDeserializationFail : Flag<["-"], "experimental-package-cmo-abort-on-deserialization-fail">,
   Flags<[FrontendOption]>,
-  HelpText<"Skip deserialization checks for package-cmo; use only for experimental purposes">;
+  HelpText<"Abort if a deserialization error is found while package optimization is enabled">;
 
 def ExperimentalPackageCMO : Flag<["-"], "experimental-package-cmo">,
   Flags<[FrontendOption]>,

lib/AST/Decl.cpp

@@ -4811,20 +4811,10 @@ bool ValueDecl::bypassResilienceInPackage(ModuleDecl *accessingModule) const {
   if (declModule->isResilient() &&
       declModule->allowNonResilientAccess() &&
       declModule->serializePackageEnabled()) {
-    // Fail and diagnose if there is a member deserialization error,
-    // with an option to skip for temporary/migration purposes.
-    if (!getASTContext().LangOpts.SkipDeserializationChecksForPackageCMO) {
-      // Since we're checking for deserialization errors, make sure the
-      // accessing module is different from this decl's module.
-      if (accessingModule &&
-          accessingModule != declModule) {
-        if (auto IDC = dyn_cast<IterableDeclContext>(this)) {
-          // Recursively check if members and their members have failing
-          // deserialization, and emit a diagnostic.
-          // FIXME: It throws a warning for now; need to upgrade to an error.
-          IDC->checkDeserializeMemberErrorInPackage(accessingModule);
-        }
-      }
+    if (auto IDC = dyn_cast<IterableDeclContext>(this)) {
+      // Recursively check if this decl's members and their members have
+      // been deserialized correctly, and emit a diagnostic if not.
+      IDC->checkDeserializeMemberErrorInPackage(accessingModule);
     }
     return true;
   }

lib/AST/DeclContext.cpp

@@ -1180,12 +1180,15 @@ void IterableDeclContext::loadAllMembers() const {
     --s->getFrontendCounters().NumUnloadedLazyIterableDeclContexts;
 }
 
-// Checks whether members of this decl and their respective members
-// (recursively) were deserialized correctly and emits a diagnostic
-// if deserialization failed. Requires accessing module and this decl's
-// module are in the same package, and this decl's module has package
-// optimization enabled.
+// Checks whether members of this decl and their respective member decls
+// (recursively) were deserialized into another module correctly, and
+// emits a diagnostic if deserialization failed. Requires the other module
+// module and this decl's module are in the same package, and this
+// decl's module has package optimization enabled.
 void IterableDeclContext::checkDeserializeMemberErrorInPackage(ModuleDecl *accessingModule) {
+  // For decls in the same module, we can skip deserialization error checks.
+  if (getDecl()->getModuleContext() == accessingModule)
+    return;
   // Only check if accessing module is in the same package as this
   // decl's module, which has package optimization enabled.
   if (!getDecl()->getModuleContext()->inSamePackage(accessingModule) ||
@@ -1220,6 +1223,10 @@ void IterableDeclContext::checkDeserializeMemberErrorInPackage(ModuleDecl *acces
       containerID = container->getBaseIdentifier();
     }
 
+    auto diagID = diag::cannot_bypass_resilience_due_to_missing_member_warn;
+    if (getASTContext().LangOpts.AbortOnDeserializationFailForPackageCMO)
+      diagID = diag::cannot_bypass_resilience_due_to_missing_member;
+
     auto foundMissing = false;
     for (auto member: memberList) {
       // Only storage vars can affect memory layout so
@@ -1237,7 +1244,7 @@ void IterableDeclContext::checkDeserializeMemberErrorInPackage(ModuleDecl *acces
           foundMissing = false;
           auto missingMemberID = missingMember->getName().getBaseIdentifier();
           getASTContext().Diags.diagnose(member->getLoc(),
-                                         diag::cannot_bypass_resilience_due_to_missing_member,
+                                         diagID,
                                          missingMemberID,
                                          missingMemberID.empty(),
                                          containerID,
@@ -1249,7 +1256,7 @@ void IterableDeclContext::checkDeserializeMemberErrorInPackage(ModuleDecl *acces
       // If not handled above, emit a diag here.
       if (foundMissing) {
         getASTContext().Diags.diagnose(getDecl()->getLoc(),
-                                       diag::cannot_bypass_resilience_due_to_missing_member,
+                                       diagID,
                                        Identifier(),
                                        true,
                                        containerID,

lib/Frontend/CompilerInvocation.cpp

@@ -1349,7 +1349,7 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
     }
   }
 
-  Opts.SkipDeserializationChecksForPackageCMO = Args.hasArg(OPT_ExperimentalSkipDeserializationChecksForPackageCMO);
+  Opts.AbortOnDeserializationFailForPackageCMO = Args.hasArg(OPT_ExperimentalPackageCMOAbortOnDeserializationFail);
   Opts.AllowNonResilientAccess =
       Args.hasArg(OPT_experimental_allow_non_resilient_access) ||
       Args.hasArg(OPT_allow_non_resilient_access) ||

test/SILOptimizer/package-cmo-disallow-bypass-resilience-on-deserialization-fail.swift

@@ -20,6 +20,18 @@
 // RUN: -I %t/artifacts/ObjcBuilds -L %t/artifacts/ObjcBuilds \
 // RUN: -lObjCAPI -Xcc -DINCLUDE_FOO
 
+/// Build a swift module that depends on the above Core swift module without `INCLUDE_FOO`;
+/// it should fail and diagnose that there was a deserialization failure.
+// RUN: not %target-build-swift-dylib(%t/artifacts/SwiftBuilds/%target-library-name(MyUIA)) %t/src/UIA.swift \
+// RUN: -module-name MyUIA -emit-module -package-name pkg \
+// RUN: -enable-library-evolution -O -wmo \
+// RUN: -Xfrontend -experimental-package-cmo-abort-on-deserialization-fail \
+// RUN: -I %t/artifacts/SwiftBuilds -L %t/artifacts/SwiftBuilds \
+// RUN: -I %t/artifacts/ObjcBuilds -L %t/artifacts/ObjcBuilds \
+// RUN: -lMyCore -lObjCAPI -Rmodule-loading 2>&1 | tee %t/MyUIA-build.txt 
+// RUN: %FileCheck %s --check-prefix=CHECK-ERROR < %t/MyUIA-build.txt
+// CHECK-ERROR: error: cannot bypass resilience due to member deserialization failure while attempting to access missing member of 'PkgStructA' in module 'MyCore' from module 'MyUIA'
+
 /// Build another swift module that depends on Core; since FooObjc is not referenced
 /// in the call chain, there's no deserialization failure, and bypassing resilience is allowed.
 // RUN: %target-build-swift-dylib(%t/artifacts/SwiftBuilds/%target-library-name(MyUIB)) %t/src/UIB.swift \
@@ -35,10 +47,12 @@
 
 //--- UIA.swift
 package import MyCore
-package import ObjCAPI
+import ObjCAPI
 
 package func testWrapperA(_ arg: WrapperA) {
-  print(arg.varA, arg.varAdict, arg.varAarray)
+  if let x = arg.varA, let y = arg.varAdict, let z = arg.varAarray {
+      print(x, y, z)
+  }
 }
 
 package func testKlass(_ arg: Klass) {
@@ -47,11 +61,12 @@ package func testKlass(_ arg: Klass) {
 
 //--- UIB.swift
 public import MyCore
-public import ObjCAPI
+import ObjCAPI
 
 package func testWrapperB(_ arg: WrapperB) {
-  let v = arg.varB
-  print(v)
+  if let v = arg.varB {
+    print(v)
+  }
 }
 
 package func testKlass(_ arg: Klass) {