Skip to content

[Serialization] Add flag to force unsafe recovery from some xref failures #66186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,10 @@ NOTE(modularization_issue_side_effect_type_error,none,
"could not deserialize type for %0",
(DeclName))

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"
" 'foo.%1' expression", (DeclName, StringRef))
Expand Down
5 changes: 5 additions & 0 deletions include/swift/Basic/LangOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 4 additions & 0 deletions include/swift/Option/FrontendOptions.td
Original file line number Diff line number Diff line change
Expand Up @@ -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">;
Expand Down
2 changes: 2 additions & 0 deletions lib/Frontend/CompilerInvocation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
39 changes: 29 additions & 10 deletions lib/Serialization/Deserialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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<char, 64> strScratch;

auto errorKind = ModularizationError::Kind::DeclNotFound;
Expand Down Expand Up @@ -1945,13 +1946,31 @@ ModuleFile::resolveCrossReference(ModuleID MID, uint32_t pathLen) {
auto declName = getXRefDeclNameForError();
auto expectedIn = baseModule->getName();
auto referencedFrom = getName();
return llvm::make_error<ModularizationError>(declName,
isType,
errorKind,
expectedIn,
referencedFrom,
foundIn,
pathTrace);
auto error = llvm::make_error<ModularizationError>(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.
getContext().Diags.diagnose(getSourceLoc(),
diag::modularization_issue_worked_around);
llvm::handleAllErrors(std::move(error),
[&](const ModularizationError &modularError) {
modularError.diagnose(this, DiagnosticBehavior::Note);
});
} else {
return std::move(error);
}
}

// Filters for values discovered in the remaining path pieces.
Expand Down
8 changes: 8 additions & 0 deletions test/Serialization/modularization-error.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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: 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.
// 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
Expand Down