From 642980cbb1a2db0533d56408928123e4a134f76d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexis=20Laferri=C3=A8re?= Date: Tue, 9 May 2023 11:28:50 -0700 Subject: [PATCH 1/2] [Serialization] Add flag to force unsafe recovery from some xref failures Intro a deserialization mode controlled by the flag `-experimental-force-workaround-broken-modules` to attempt unsafe recovery from deserialization failures caused by project issues. The one issue handled at this time is when a type moves from one module to another. With this new mode the compiler may be able to pick a matching type in a different module. This is risky to use, but may help in a pinch for a client to fix and issue in a library over which they have no control. --- include/swift/AST/DiagnosticsSema.def | 4 ++ include/swift/Basic/LangOptions.h | 5 +++ include/swift/Option/FrontendOptions.td | 4 ++ lib/Frontend/CompilerInvocation.cpp | 2 + lib/Serialization/Deserialization.cpp | 40 ++++++++++++++----- test/Serialization/modularization-error.swift | 8 ++++ 6 files changed, 53 insertions(+), 10 deletions(-) diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index 81c4839089825..fd99d936d4cac 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -893,6 +893,10 @@ NOTE(modularization_issue_side_effect_type_error,none, "could not deserialize type for %0", (DeclName)) +NOTE(modularization_issue_worked_around,none, + "attempting forced recovery using top-level declaration from %0", + (Identifier)) + ERROR(reserved_member_name,none, "type member must not be named %0, since it would conflict with the" " 'foo.%1' expression", (DeclName, StringRef)) diff --git a/include/swift/Basic/LangOptions.h b/include/swift/Basic/LangOptions.h index 7e2402e145833..658f4104d8b4f 100644 --- a/include/swift/Basic/LangOptions.h +++ b/include/swift/Basic/LangOptions.h @@ -390,6 +390,11 @@ namespace swift { bool EnableDeserializationSafety = ::getenv("SWIFT_ENABLE_DESERIALIZATION_SAFETY"); + /// Attempt to recover for imported modules with broken modularization + /// in an unsafe way. Currently applies only to xrefs where the target + /// decl moved to a different module that is already loaded. + bool ForceWorkaroundBrokenModules = false; + /// Whether to enable the new operator decl and precedencegroup lookup /// behavior. This is a staging flag, and will be removed in the future. bool EnableNewOperatorLookup = false; diff --git a/include/swift/Option/FrontendOptions.td b/include/swift/Option/FrontendOptions.td index faf262f135315..a4f7f6faafc4c 100644 --- a/include/swift/Option/FrontendOptions.td +++ b/include/swift/Option/FrontendOptions.td @@ -588,6 +588,10 @@ def disable_deserialization_safety : Flag<["-"], "disable-deserialization-safety">, HelpText<"Don't avoid reading potentially unsafe decls in swiftmodules">; +def force_workaround_broken_modules : + Flag<["-"], "experimental-force-workaround-broken-modules">, + HelpText<"Attempt unsafe recovery for imported modules with broken modularization">; + def enable_experimental_string_processing : Flag<["-"], "enable-experimental-string-processing">, HelpText<"Enable experimental string processing">; diff --git a/lib/Frontend/CompilerInvocation.cpp b/lib/Frontend/CompilerInvocation.cpp index 6856112b062bc..ec49c49f0ff11 100644 --- a/lib/Frontend/CompilerInvocation.cpp +++ b/lib/Frontend/CompilerInvocation.cpp @@ -564,6 +564,8 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args, Opts.EnableDeserializationSafety = A->getOption().matches(OPT_enable_deserialization_safety); } + Opts.ForceWorkaroundBrokenModules + |= Args.hasArg(OPT_force_workaround_broken_modules); // Whether '/.../' regex literals are enabled. This implies experimental // string processing. diff --git a/lib/Serialization/Deserialization.cpp b/lib/Serialization/Deserialization.cpp index e14225ae81cff..a61f68a595b40 100644 --- a/lib/Serialization/Deserialization.cpp +++ b/lib/Serialization/Deserialization.cpp @@ -1803,8 +1803,10 @@ ModuleFile::resolveCrossReference(ModuleID MID, uint32_t pathLen) { } auto getXRefDeclNameForError = [&]() -> DeclName { + BCOffsetRAII restoreOffset(DeclTypeCursor); DeclName result = pathTrace.getLastName(); - while (--pathLen) { + uint32_t namePathLen = pathLen; + while (--namePathLen) { llvm::BitstreamEntry entry = fatalIfUnexpected(DeclTypeCursor.advance(AF_DontPopBlockAtEnd)); if (entry.Kind != llvm::BitstreamEntry::Record) @@ -1869,8 +1871,7 @@ ModuleFile::resolveCrossReference(ModuleID MID, uint32_t pathLen) { // up to the caller to recover if possible. // Look for types and value decls in other modules. This extra information - // is mostly for compiler engineers to understand a likely solution at a - // quick glance. + // will be used for diagnostics by the caller logic. SmallVector strScratch; auto errorKind = ModularizationError::Kind::DeclNotFound; @@ -1945,13 +1946,32 @@ ModuleFile::resolveCrossReference(ModuleID MID, uint32_t pathLen) { auto declName = getXRefDeclNameForError(); auto expectedIn = baseModule->getName(); auto referencedFrom = getName(); - return llvm::make_error(declName, - isType, - errorKind, - expectedIn, - referencedFrom, - foundIn, - pathTrace); + auto error = llvm::make_error(declName, + isType, + errorKind, + expectedIn, + referencedFrom, + foundIn, + pathTrace); + + // If we want to workaround broken modularization, we can keep going if + // we found a matching top-level decl in a different module. This is + // obviously dangerous as it could just be some other decl that happens to + // match. + if (getContext().LangOpts.ForceWorkaroundBrokenModules && + errorKind == ModularizationError::Kind::DeclMoved && + !values.empty()) { + // Print the error as a remark and notify of the recovery attempt. + llvm::handleAllErrors(std::move(error), + [&](const ModularizationError &modularError) { + modularError.diagnose(this, DiagnosticBehavior::Remark); + }); + getContext().Diags.diagnose(getSourceLoc(), + diag::modularization_issue_worked_around, + foundIn); + } else { + return error; + } } // Filters for values discovered in the remaining path pieces. diff --git a/test/Serialization/modularization-error.swift b/test/Serialization/modularization-error.swift index 2c0ae39b05bec..270811adffed1 100644 --- a/test/Serialization/modularization-error.swift +++ b/test/Serialization/modularization-error.swift @@ -14,6 +14,14 @@ // RUN: | %FileCheck --check-prefixes CHECK,CHECK-MOVED %s // CHECK-MOVED: LibWithXRef.swiftmodule:1:1: error: reference to type 'MyType' broken by a context change; 'MyType' was expected to be in 'A', but now a candidate is found only in 'B' +/// Force working around the broken modularization to get a result and no errors. +// RUN: %target-swift-frontend -emit-sil %t/LibWithXRef.swiftmodule -module-name LibWithXRef -I %t \ +// RUN: -experimental-force-workaround-broken-modules 2>&1 \ +// RUN: | %FileCheck --check-prefixes CHECK-WORKAROUND %s +// CHECK-WORKAROUND: remark: reference to type 'MyType' broken by a context change; 'MyType' was expected to be in 'A', but now a candidate is found only in 'B' +// CHECK-WORKAROUND: note: attempting forced recovery using top-level declaration from 'B' +// CHECK-WORKAROUND: func foo() -> some Proto + /// Change MyType into a function. // RUN: %target-swift-frontend %t/LibTypeChanged.swift -emit-module-path %t/A.swiftmodule -module-name A // RUN: %target-swift-frontend %t/Empty.swift -emit-module-path %t/B.swiftmodule -module-name B From d50f20e4a83024cee74f40b1470661d1cabac910 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexis=20Laferri=C3=A8re?= Date: Fri, 26 May 2023 15:24:18 -0700 Subject: [PATCH 2/2] [Serialization] Make a warning the diagnostic about attempting recovery Use the `attempting forced recovery` diagnostic as main warning to which we attach other messages as notes. Also mention the flag in the diagnostic to reinforce that the flag is active. --- include/swift/AST/DiagnosticsSema.def | 6 +++--- lib/Serialization/Deserialization.cpp | 9 ++++----- test/Serialization/modularization-error.swift | 4 ++-- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index fd99d936d4cac..ef8f5b895dc8f 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -893,9 +893,9 @@ NOTE(modularization_issue_side_effect_type_error,none, "could not deserialize type for %0", (DeclName)) -NOTE(modularization_issue_worked_around,none, - "attempting forced recovery using top-level declaration from %0", - (Identifier)) +WARNING(modularization_issue_worked_around,none, + "attempting forced recovery enabled by -experimental-force-workaround-broken-modules", + ()) ERROR(reserved_member_name,none, "type member must not be named %0, since it would conflict with the" diff --git a/lib/Serialization/Deserialization.cpp b/lib/Serialization/Deserialization.cpp index a61f68a595b40..781152625819c 100644 --- a/lib/Serialization/Deserialization.cpp +++ b/lib/Serialization/Deserialization.cpp @@ -1962,15 +1962,14 @@ ModuleFile::resolveCrossReference(ModuleID MID, uint32_t pathLen) { errorKind == ModularizationError::Kind::DeclMoved && !values.empty()) { // Print the error as a remark and notify of the recovery attempt. + getContext().Diags.diagnose(getSourceLoc(), + diag::modularization_issue_worked_around); llvm::handleAllErrors(std::move(error), [&](const ModularizationError &modularError) { - modularError.diagnose(this, DiagnosticBehavior::Remark); + modularError.diagnose(this, DiagnosticBehavior::Note); }); - getContext().Diags.diagnose(getSourceLoc(), - diag::modularization_issue_worked_around, - foundIn); } else { - return error; + return std::move(error); } } diff --git a/test/Serialization/modularization-error.swift b/test/Serialization/modularization-error.swift index 270811adffed1..9b0b0361c0c48 100644 --- a/test/Serialization/modularization-error.swift +++ b/test/Serialization/modularization-error.swift @@ -18,8 +18,8 @@ // RUN: %target-swift-frontend -emit-sil %t/LibWithXRef.swiftmodule -module-name LibWithXRef -I %t \ // RUN: -experimental-force-workaround-broken-modules 2>&1 \ // RUN: | %FileCheck --check-prefixes CHECK-WORKAROUND %s -// CHECK-WORKAROUND: remark: reference to type 'MyType' broken by a context change; 'MyType' was expected to be in 'A', but now a candidate is found only in 'B' -// CHECK-WORKAROUND: note: attempting forced recovery using top-level declaration from 'B' +// CHECK-WORKAROUND: warning: attempting forced recovery enabled by -experimental-force-workaround-broken-modules +// CHECK-WORKAROUND: note: reference to type 'MyType' broken by a context change; 'MyType' was expected to be in 'A', but now a candidate is found only in 'B' // CHECK-WORKAROUND: func foo() -> some Proto /// Change MyType into a function.