[Serialization] Report modularization breaks as proper diagnostics #65713

xymus · 2023-05-05T19:32:12Z

The Swift compiler expects the context to remain stable between when a module is built and loaded by a client. Usually the build system would rebuild a module if a dependency changes, or the compiler would rebuilt the module from a swiftinterface on a context change. However, such changes are not always detected and in that case the compiler may crash on an inconsistency in the context. We often see this when a clang module is poorly modularized, the headers are modified in the SDK, or some clang define change its API.

These are project issues that used to make the compiler crash, it provided a poor experience and doesn't encourage the developer to fix them by themselves. Instead, let's keep track of modularization issues encountered during deserialization and report them as proper errors when and if they trigger a fatal failure.

Next I'll look to add a mode to remark on any modularization issue detected, not only the fatal ones. And a mode to workaround some modules issues and recover when a type moves between modules.

xymus · 2023-05-05T19:32:41Z

@swift-ci Please smoke test

xymus · 2023-05-11T21:45:06Z

@swift-ci Please smoke test

xymus · 2023-05-11T21:52:59Z

include/swift/AST/DiagnosticsSema.def

+      "module reference to top-level %0 broken by a context change, "
+      "module '%2' references %0 in %1, but it cannot be found in this build. "
+      "Audit related headers or uninstall SDK roots.",
+      (DeclName, Identifier, StringRef, Identifier))


@tshortli @bnbarham I'd be interest in having your opinions on these three main diagnostics. You can also see them in the test.

Do you have suggestions on how to make them more actionable or simply more understandable by project owners?

I like the general direction of providing more complete diagnostics for these failures. My main criticism is that I'm not sure the specific calls to action here are applicable. Suggesting uninstallation of roots may not help because presumably the root is needed. Suggesting auditing headers is a bit vague; I'm not sure what I would personally do to follow through on that suggestion.

If the problem is that the .swiftmodule is stale but the build system/compiler is unable to detect that it ought to rebuild it, maybe what we should be suggesting is to remove the potentially stale .swiftmodule?

Suggesting to delete the stale swiftmodule makes perfect sense, it would fix it in most roots case.

I'm currently looking into pointing the diagnostics on the swiftmodule file with the broken reference instead of the current <unknown>:0:. By itself it should help to put into doubts the SDK or recently installed roots.

The case of auditing headers may be too general to be useful. I'm mostly thinking about the Xcc issue but describing that in a diagnostic is a challenge. I'm considerong moving the recommended solutions to separate notes, so we could make them longer if it helps.

Is deleting the swiftmodule actually always an option? It could be a distributed binary module without library evolution enabled couldn't it?

I'm considerong moving the recommended solutions to separate notes

This would be good IMO. I think it'd be better if the error should be a short summary, with the rest in notes.

Is deleting the swiftmodule actually always an option? It could be a distributed binary module without library evolution enabled couldn't it?

Yeah, we should only suggest it when the module is resilient I suppose.

I reworked those diagnostics to make them simpler and shorter. They are now reported on the swiftmodule file making it more intuitive to see where is the broken reference.

I removed the hints to solutions and potential problems from this PR. I plan on replacing them with notes and further analysis of the context, i.e. hint that the swiftmodule is outdated only when resilient and distributed.

xymus · 2023-05-11T23:23:30Z

@swift-ci Please smoke test

elsh · 2023-05-12T00:38:04Z

test/Serialization/modularization-error.swift

+// RUN: %target-swift-frontend %t/Empty.swift -emit-module-path %t/B.swiftmodule -module-name B
+// RUN: not %target-swift-frontend -emit-sil %t/Client.swiftmodule -module-name Client -I %t 2>&1 \
+// RUN:   | %FileCheck --check-prefixes CHECK,CHECK-TYPE-CHANGED %s
+// CHECK-TYPE-CHANGED: <unknown>:0: error: module reference to top-level 'MyType' broken by a context change, its type changed since building 'Client', it was in 'A' now found in 'A'. Ensure Swift language version match between modules or uninstall SDK roots.


Is it possible to show the type info here? e.g. it was a struct but now a func in module 'A'

I should be able to. Since it failed to deserialize we pretty much just know if it's a type or if it's a ValueDecl. In most cases it's a type so having that precision should help.

elsh · 2023-05-12T00:39:49Z

include/swift/AST/DiagnosticsSema.def

@@ -872,6 +872,31 @@ ERROR(need_hermetic_seal_to_import_module,none,
      "current compilation does not have -experimental-hermetic-seal-at-link",
      (Identifier))

+ERROR(modularization_issue_moved,Fatal,
+      "module reference to top-level %0 broken by a context change, "


would read better with ; instead of , after 'context change'

elsh · 2023-05-12T00:40:31Z

include/swift/AST/DiagnosticsSema.def

@@ -872,6 +872,31 @@ ERROR(need_hermetic_seal_to_import_module,none,
      "current compilation does not have -experimental-hermetic-seal-at-link",
      (Identifier))

+ERROR(modularization_issue_moved,Fatal,


modularization_issue_decl_moved might be more clear

Added decl_ to all of them and the related diagnostics.

elsh · 2023-05-12T00:41:06Z

lib/Serialization/DeserializationErrors.h

+                DiagnosticBehavior limit = DiagnosticBehavior::Fatal) const;
+
+  void log(raw_ostream &OS) const override {
+    OS << "modularization issue on '" << name << "', reference from '";


referenced from

I left this as is because it reads reference from X not resolvable. In theory nobody should see this error as it is replaced by the proper diagnostic.

elsh · 2023-05-12T00:42:08Z

include/swift/AST/DiagnosticsSema.def

+      (DeclName, Identifier, StringRef, Identifier))
+ERROR(modularization_issue_not_found,Fatal,
+      "module reference to top-level %0 broken by a context change, "
+      "module '%2' references %0 in %1, but it cannot be found in this build. "


%0 cannot be found in this build

elsh · 2023-05-12T00:43:54Z

lib/Serialization/DeserializationErrors.h

+
+public:
+  enum class Kind {
+    Moved,


elsh · 2023-05-12T00:44:18Z

lib/Serialization/DeserializationErrors.h

+  enum class Kind {
+    Moved,
+    TypeChanged,
+    NotFound


DeclNotFound

elsh · 2023-05-12T00:44:52Z

include/swift/AST/DiagnosticsSema.def

+      "Audit related headers or uninstall SDK roots.",
+      (DeclName, Identifier, StringRef, Identifier))
+
+NOTE(modularization_issue_effect_extension_error,none,


what does 'effect' here mean?

Replaced it with side_effect, hopefully it's clearer.

elsh · 2023-05-12T00:45:39Z

lib/Serialization/DeserializationErrors.h

@@ -359,82 +429,111 @@ class OverrideError : public llvm::ErrorInfo<OverrideError,
  }
 };

-class TypeError : public llvm::ErrorInfo<TypeError, DeclDeserializationError> {
+// Service class for errors with an underlying cause.
+class CausedError {


ErrorCause might be clearer to read

Changed it to ErrorWithUnderlyingReason.

Intro a new class ErrorWithUnderlyingReason to refactor duplicated logic from three classes. It can be used as super class to any error with an underlying error.

bnbarham · 2023-05-17T16:24:37Z

lib/AST/DiagnosticEngine.cpp

@@ -32,6 +32,7 @@
 #include "swift/Config.h"
 #include "swift/Localization/LocalizationFormat.h"
 #include "swift/Parse/Lexer.h" // bad dependency
+#include "../Serialization/ModuleFile.h"


I think it would be good to avoid this:

It's a private header in Serialization. If we wanted to use it we really should move it out into the top level include.

This introduces a circular dependency - Serialization depends on ClangImporter depends on AST. And AST depends on Serialization now.

Could we just add the new diagnose in Serialization and have it do the ModuleFile -> SourceLoc mapping instead? That would also avoid all the {} -> SourceLoc() changes.

Good point, I'm looking into it.

I added a ModuleFile::getSourceLoc() instead, it cleans up the PR nicely.

bnbarham · 2023-05-17T16:28:29Z

lib/AST/DiagnosticEngine.cpp

+    auto filename = file->getModuleFilename();
+    auto bufferID = SourceMgr.getIDForBufferIdentifier(filename);
+    if (!bufferID)
+      bufferID = SourceMgr.addMemBufferCopy(filename, filename);


Any particular reason to have the filename in the fake buffer? For debugging? Doesn't really hurt I suppose.

Replaced it with an empty string.

Generate a fake empty buffer to return a SourceLoc pointing to the beginning of a swiftmodule file.

The Swift compiler expects the context to remain stable between when a module is built and loaded by a client. Usually the build system would rebuild a module if a dependency changes, or the compiler would rebuilt the module from a swiftinterface on a context change. However, such changes are not always detected and in that case the compiler may crash on an inconsistency in the context. We often see this when a clang module is poorly modularized, the headers are modified in the SDK, or some clang define change its API. These are project issues that used to make the compiler crash, it provided a poor experience and doesn't encourage the developer to fix them by themselves. Instead, let's keep track of modularization issues encountered during deserialization and report them as proper errors when they trigger a fatal failure preventing compilation.

xymus · 2023-05-17T17:27:08Z

@swift-ci Please smoke test

tshortli · 2023-05-17T22:36:45Z

include/swift/AST/DiagnosticsSema.def

+      (bool, DeclName, Identifier, Identifier))
+ERROR(modularization_issue_decl_type_changed,Fatal,
+      "reference to %select{top-level|type}0 %1 broken by a context change; "
+      "the details of %1 %select{from %2|}5 changed since building '%3'"


Reading this diagnostic in context in the test case made me feel like the details of was a bit vague. Would the declaration kind of make sense? It would be especially helpful if we could print what kind of declaration we expected it to be and what it actually is but that seems like a stretch goal for another time.

That's an improvement, I'll apply it in my next PR.

I took another look and we could print more details here. That diagnostics actually points out if there were candidate decls but all of them were filtered out. There's about a dozen reasons why a decl could be filtered out, most of them are simply to differentiate decls sharing a same name. However, we could modify that logic to collect the specific reasons that are triggered and show them in the diagnostics. I've seen that issue very rarely, about once a year, but when it does hit having more details could be critical. If time permits I'll come back to this diagnostic.

xymus requested review from hborla, slavapestov and xedin as code owners May 5, 2023 19:32

xymus marked this pull request as draft May 5, 2023 19:32

xymus force-pushed the report-modularization-breaks branch 3 times, most recently from 0829c68 to 2c43e2b Compare May 11, 2023 21:43

xymus changed the title ~~[Serialization] Report detected modularization breaks~~ [Serialization] Report modularization breaks as proper diagnostics May 11, 2023

xymus marked this pull request as ready for review May 11, 2023 21:47

xymus requested review from bnbarham and tshortli May 11, 2023 21:47

xymus commented May 11, 2023

View reviewed changes

xymus force-pushed the report-modularization-breaks branch from 2c43e2b to 4fd7d8e Compare May 11, 2023 23:23

elsh reviewed May 12, 2023

View reviewed changes

lib/Serialization/DeserializationErrors.h Outdated

public:

enum class Kind {

Moved,

Copy link

Contributor

elsh May 12, 2023

Choose a reason for hiding this comment

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

DeclMoved

xymus reacted with thumbs up emoji

elsh reviewed May 12, 2023

View reviewed changes

xymus added 2 commits May 16, 2023 15:58

[Serialization] Refactor some services on errors

f67f7e7

Intro a new class ErrorWithUnderlyingReason to refactor duplicated logic from three classes. It can be used as super class to any error with an underlying error.

[Serialization] Keep going after fatal errors in getAllWitnessTables

478c653

xymus force-pushed the report-modularization-breaks branch from 4fd7d8e to f62af93 Compare May 16, 2023 23:15

xymus requested review from artemcm and zoecarver as code owners May 16, 2023 23:15

xymus requested review from hyp and egorzhdan as code owners May 16, 2023 23:15

xymus force-pushed the report-modularization-breaks branch from f62af93 to 7f8bf0b Compare May 17, 2023 16:14

bnbarham reviewed May 17, 2023

View reviewed changes

xymus added 2 commits May 17, 2023 10:21

[Serialization] Intro ModuleFile::getSourceLoc()

008047f

Generate a fake empty buffer to return a SourceLoc pointing to the beginning of a swiftmodule file.

xymus force-pushed the report-modularization-breaks branch from 7f8bf0b to 144d7eb Compare May 17, 2023 17:25

tshortli approved these changes May 17, 2023

View reviewed changes

xymus merged commit 74fd209 into swiftlang:main May 18, 2023

xymus deleted the report-modularization-breaks branch May 18, 2023 20:57

This was referenced May 25, 2023

[Serialization] Intro -Rmodule-recovery to remark on inconsistencies in swiftmodule files #66139

Merged

[Serialization] Display contextual notes on deserialization errors and misconfigurations #66227

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serialization] Report modularization breaks as proper diagnostics #65713

[Serialization] Report modularization breaks as proper diagnostics #65713

xymus commented May 5, 2023 •

edited

Loading

xymus commented May 5, 2023

xymus commented May 11, 2023

xymus May 11, 2023

tshortli May 12, 2023

xymus May 12, 2023

bnbarham May 12, 2023 •

edited

Loading

tshortli May 12, 2023

xymus May 16, 2023

xymus commented May 11, 2023

elsh May 12, 2023

xymus May 12, 2023

elsh May 12, 2023

elsh May 12, 2023 •

edited

Loading

xymus May 16, 2023

elsh May 12, 2023

xymus May 16, 2023

elsh May 12, 2023

elsh May 12, 2023

elsh May 12, 2023

elsh May 12, 2023

xymus May 16, 2023

elsh May 12, 2023 •

edited

Loading

xymus May 16, 2023

bnbarham May 17, 2023

xymus May 17, 2023

xymus May 17, 2023

bnbarham May 17, 2023

xymus May 17, 2023

xymus commented May 17, 2023

tshortli May 17, 2023

xymus May 18, 2023

[Serialization] Report modularization breaks as proper diagnostics #65713

[Serialization] Report modularization breaks as proper diagnostics #65713

Conversation

xymus commented May 5, 2023 • edited Loading

xymus commented May 5, 2023

xymus commented May 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnbarham May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xymus commented May 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elsh May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elsh May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xymus commented May 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xymus commented May 5, 2023 •

edited

Loading

bnbarham May 12, 2023 •

edited

Loading

elsh May 12, 2023 •

edited

Loading

elsh May 12, 2023 •

edited

Loading