Skip to content
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
27 changes: 23 additions & 4 deletions lib/Serialization/Deserialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7518,9 +7518,9 @@ ModuleFile::getClangType(ClangTypeID TID) {
return clangType;
}

Decl *handleErrorAndSupplyMissingClassMember(ASTContext &context,
llvm::Error &&error,
ClassDecl *containingClass) {
Decl *ModuleFile::handleErrorAndSupplyMissingClassMember(
ASTContext &context, llvm::Error &&error,
ClassDecl *containingClass) const {
Decl *suppliedMissingMember = nullptr;
auto handleMissingClassMember = [&](const DeclDeserializationError &error) {
if (error.isDesignatedInitializer())
Expand All @@ -7534,7 +7534,26 @@ Decl *handleErrorAndSupplyMissingClassMember(ASTContext &context,
error.getNumberOfVTableEntries(),
error.needsFieldOffsetVectorEntry());
};
llvm::handleAllErrors(std::move(error), handleMissingClassMember);

// Emit the diagnostics/remarks out of the ModularizationError
// wrapped in a TypeError (eg. coming from resolveCrossReference),
// which is otherwise just dropped but could help better understand
// C/C++ interop issues.
assert(context.LangOpts.EnableDeserializationRecovery);
auto handleModularizationError = [&](ModularizationError &error)
-> llvm::Error {
if (context.LangOpts.EnableModuleRecoveryRemarks)
Copy link
Contributor

@xymus xymus Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this as an expansion to EnableModuleRecoveryRemarks. Just so you know, we may turn it on by default in the future in some contexts. It currently reports both expected and unexpected errors, but when we start preventing the expected errors earlier we could start reporting only the unexpected errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know about the chance that the flag may be enabled by default.

error.diagnose(this, DiagnosticBehavior::Remark);
return llvm::Error::success();
};
auto handleTypeError = [&](TypeError &typeError) {
handleMissingClassMember(typeError);
typeError.diagnoseUnderlyingReason(handleModularizationError);
if (context.LangOpts.EnableModuleRecoveryRemarks)
typeError.diagnose(this);
};
llvm::handleAllErrors(std::move(error), handleTypeError,
handleMissingClassMember);
return suppliedMissingMember;
}

Expand Down
3 changes: 3 additions & 0 deletions lib/Serialization/ModuleFile.h
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,9 @@ class ModuleFile
Decl *handleErrorAndSupplyMissingMember(ASTContext &context,
Decl *container,
llvm::Error &&error) const;
Decl *handleErrorAndSupplyMissingClassMember(
ASTContext &context, llvm::Error &&error,
ClassDecl *containingClass) const;

public:
/// Change the status of the current module.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import CxxLib

open class Window {
open func queryInterface(_ iid: IID) -> Int32? {
return nil
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import A

public class CustomWindow: Window {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#ifndef GUID_DEFINED
#define GUID_DEFINED
typedef struct _GUID {
} GUID;
#endif

#ifndef __IID_DEFINED__
#define __IID_DEFINED__
typedef GUID IID;
#endif
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module CxxLib {
header "CxxLib.h"

export *
}
10 changes: 10 additions & 0 deletions test/Serialization/wrapped-modularization-error-remarks.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend -emit-module %S/Inputs/wrapped-modularization-error-remarks/A/A.swift -cxx-interoperability-mode=default -Xcc -fmodule-map-file=%S/Inputs/wrapped-modularization-error-remarks/CxxLib/include/module.modulemap -Xcc -I -Xcc %S/Inputs/wrapped-modularization-error-remarks/CxxLib/include -module-name A -o %t/A.swiftmodule
// RUN: not %target-swift-frontend -c -primary-file %S/Inputs/wrapped-modularization-error-remarks/B/B.swift -cxx-interoperability-mode=default -I %t -Xcc -fmodule-map-file=%S/Inputs/wrapped-modularization-error-remarks/CxxLib/include/module.modulemap -Xcc -I -Xcc %S/Inputs/wrapped-modularization-error-remarks/CxxLib/include -module-name B -o %t/B.swift.o -Rmodule-recovery 2>&1 | %FileCheck %s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the underlying problem here? I'm not very familiar with the C++ interior and can't tell what could be the context change between the two commands could lead to _GUID being lost.

I'm curious as I'm likely to screen issues when they are reported by serialization errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also like to know if this issue is more of a compiler bug that could be fixed in the future and break the test. It's always a challenge to test the deserialization recovery logic, to find bugs we don't intend to fix to trigger the crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying problem is referenced in #68362 which is an issue we had on windows. I'm not sure if this is a compiler bug or a modulemap file issue. The issue may be fixed in the future and the test may break as the situation seems to be something that may be expected to work. But I'm not sure if I can predict what would happen in reality. This PR intends to emit diagnostics around the issue that help with better diagnosis.

// REQUIRES: OS=windows-msvc

// Check that the diagnostics/remark from the wrapped ModularizationError is emitted.
// CHECK: remark: reference to type '_GUID' broken by a context change; '_GUID' is not found, it was expected to be in 'CxxLib'
// CHECK: note: could not deserialize type for 'queryInterface'
// CHECK: error: cannot inherit from class 'Window' because it has overridable members that could not be loaded
// CHECK: note: could not deserialize 'queryInterface'