[Serialization] Refactor normal conformance serialization #16023

xedin · 2018-04-19T01:59:44Z

Make it so requirement substitutions are serialized/deserialized independently from
synthetic context (because these two are not always related), also optimize storage
of the witness to avoid unnecessary allocation and remove requiresSubstitution
invariant which is not always correctly established and not really needed.

Resolves SR-6754.
Resolves: rdar://problem/36497404

xedin · 2018-04-19T02:00:44Z

@swift-ci please test

xedin · 2018-04-19T02:01:13Z

@swift-ci please test source compatibility

slavapestov

The commit message is not very clear. It should explain what the two conditions mean in terms of language features used and why the old one was insufficient. Right now you’re just paraphrasing the jargon in the diff.

slavapestov · 2018-04-19T02:05:55Z

validation-test/compiler_crashers_2_fixed/0153-rdar36497404.swift

@@ -0,0 +1,44 @@
+// RUN: %target-build-swift -emit-module -o %t %s


It should be possible to produce a smaller test case, and it should go in test/Serialization

I will check how serialization tests are set up but if there is not way to check layout of the binary the test i have is good enough becuase it exposes the problem through one of the substitutions being error type.

It should still be in test/Serialization since its testing serialization

I've put it into crashers because it was a crash, but I can put it where ever you tell me to put it.

slavapestov · 2018-04-19T02:06:29Z

include/swift/AST/Witness.h

@@ -168,6 +168,11 @@ class Witness {
    return storage.get<StoredWitness *>()->syntheticEnvironment;
  }

+  /// Determine if the witness may have requirement substitutions.
+  bool mayHaveRequirementSubstitutions() const {


Does getSyntheticEnvironment not imply requiresSubstitution?

I think it does but not it would actually assert if requiresSubstitution is not set, so not really suitable for checks.

What's the specific case where requiresSubstitution() is true and getSyntheticEnvironment() is false? What about vice versa? I think it would help to understand what's actually going on here.

As far as I understand requiresSubstitution covers both requirement and witness substitutions but the latter doesn't require generic env to be present, what is happening in the example I have - a would not have generic environment but still requires substitutions because it's a static witness (I think). There is a single requirement substitution in the map from S to S (I think because it's static witness) but information about generic env and number of requirement substitutions is not going to be stored altough that single requirement substitution still would which offsets every other substitution by one.

I don't think we need this predicate per se, because I think the issue is in the construction of Witness not establishing invariants assumed by requiresSubstitution(). Specifically, that the storage is a StoredWitness * only when there are substitutions or a generic environment (or both), and a ValueDecl * for the simple case. That the invariant that requiresSubstitution() assumes, but the most-general Witness constructor doesn't establish that invariant. Oops.

More importantly, the two things that create Witness instances---the type checker and deserialization---do so differently: the type checker always goes through the most general Witness constructor, meaning that it always gets the StoredWitness * version (for which requiresSubstitution() will always return true), whereas deserialization uses the simpler Witness(ValueDecl *) constructor whenever it can---in which case it will have !requiresSubstitution() when deserializing something that the type checker built as requiresSubstitution(). Your change fixes the problem because it avoids relying on the invariant (so it checks the underlying condition), but it would be better to

(1) Teach the most-general Witness constructor to use the simpler ValueDecl * storage form when it can, and
(2) Eliminate the "optimization" from the deserialization code that tries to guess when to provide only the ValueDecl *, so the two Witness-constructing pieces use the same information.

I might not be understanding completely what is going on here but what you suggest doesn't actually fix the problem (I tried moving if block from deserialization to Witness general constructor) ... Because if you consider declaration of a in P3 it does return true for requiresSubstitution, doesn't have generic synthetic environment (which seems correct), does have a single synthetic requirement substitution S, and one witness substitution S when it's being originally written to partial module so checking just generic signature at one place and requiresSubstitution() when writing records would produce different results in serialization that's why deserialization produces incorrect results.

It looks like https://github.com/apple/swift/blob/master/lib/Sema/TypeCheckProtocol.cpp#L93-L101 is getting synthetic substitutions independent from presence of synthetic environment, that's why there could be requirement substitutions without synthetic environment, maybe the bug hides in that fact?

More concretely for static method a synthetic env ref would be 0 and # of requirement substitutions 0 (because synthetic env is absent), number of witness substitutions 1, but we'd still write one record for both requirement and witness substitutions later one because requiresSubstitution is true and size of reqToSyntheticSubs is 1 (since we got it from requirement signature in RequirementMatch::getWitness).

I think there are several bugs here, one of which is tripped up by the test case you've reduced. Here are the issues I see:

requiresSubstitution() is checking the kind of storage (StoredWitness *) as a proxy for "has interesting an synthetic environment", but nothing establishes that as an invariant. We can either establish that as an invariant or eliminate requiresSubstitution() outright. It's probably best to remove it---it's used incorrectly/inconsistently basically everywhere, mainly to dodge the assertions in getSyntheticEnvironment() and getRequirementToSyntheticSubs(). Let's make those functions do a dyn_cast to StoredWitness * and return its information when available, or nullptr/{} when not available, so we can clean up the callers. This is what my first big comment was about, but it's not the whole solution.

You've found that one of the assumptions that serialization is making is not an invariant in the AST, i.e., that one can only have a non-empty reqToSyntheticSubs if the requirement environment is non-null. You pull request fixes this by using a consistent predicate in both places, which is good, but...

When this happens, serialization (both before and after your PR) will drop the contents of reqToSyntheticSubs, so the deserialized Witness will be different from the originally-constructed form. This could potentially break SILGen's protocol witness emission from a deserialized Witness, which looks at and uses reqToSyntheticSubs always (without checking for a null requirement environment). To fix this, I recommend we always emit the size of reqToSyntheticSubs (e.g., take out this if on the serialization side and the corresponding one in deserialization).

Thanks @DougGregor! What you are saying totally makes sense to me, I don't think number of requirement substitution should be dependent on presence of synthetic environment!

DougGregor · 2018-04-19T05:20:44Z

include/swift/AST/Witness.h

@@ -168,6 +168,11 @@ class Witness {
    return storage.get<StoredWitness *>()->syntheticEnvironment;
  }

+  /// Determine if the witness may have requirement substitutions.
+  bool mayHaveRequirementSubstitutions() const {


I don't think we need this predicate per se, because I think the issue is in the construction of Witness not establishing invariants assumed by requiresSubstitution(). Specifically, that the storage is a StoredWitness * only when there are substitutions or a generic environment (or both), and a ValueDecl * for the simple case. That the invariant that requiresSubstitution() assumes, but the most-general Witness constructor doesn't establish that invariant. Oops.

More importantly, the two things that create Witness instances---the type checker and deserialization---do so differently: the type checker always goes through the most general Witness constructor, meaning that it always gets the StoredWitness * version (for which requiresSubstitution() will always return true), whereas deserialization uses the simpler Witness(ValueDecl *) constructor whenever it can---in which case it will have !requiresSubstitution() when deserializing something that the type checker built as requiresSubstitution(). Your change fixes the problem because it avoids relying on the invariant (so it checks the underlying condition), but it would be better to

(1) Teach the most-general Witness constructor to use the simpler ValueDecl * storage form when it can, and
(2) Eliminate the "optimization" from the deserialization code that tries to guess when to provide only the ValueDecl *, so the two Witness-constructing pieces use the same information.

Absence of synthetic generic environment should not affect serialization of the required substitutions because they can come from outer requirement context for static members. Resolves: rdar://problem/36497404

Avoid storing more information than necessary in witnesses and establishing the invariant that we only use out-of-line storage when we require substitutions.

xedin · 2018-04-20T02:17:49Z

@swift-ci please test

xedin · 2018-04-20T02:18:01Z

@swift-ci please test source compatibility

DougGregor

This looks fantastic, thank you!

slavapestov · 2018-04-20T09:16:16Z

@xedin Thank you for taking the time to get to the bottom of the problem and come up with this great fix!

xedin requested a review from DougGregor April 19, 2018 01:59

xedin changed the title ~~[Serialization] Don't serialize requirement substitutions records wit…~~ [Serialization] Don't serialize requirement substitution records wit… Apr 19, 2018

xedin force-pushed the rdar-36497404 branch from 4faae12 to 27b3927 Compare April 19, 2018 02:00

slavapestov reviewed Apr 19, 2018

View reviewed changes

DougGregor requested changes Apr 19, 2018

View reviewed changes

xedin added 4 commits April 19, 2018 17:58

[Serialization] Always serialize requirement substitutions

e1fab05

Absence of synthetic generic environment should not affect serialization of the required substitutions because they can come from outer requirement context for static members. Resolves: rdar://problem/36497404

[AST] Unify witness construction logic

f5db1df

Avoid storing more information than necessary in witnesses and establishing the invariant that we only use out-of-line storage when we require substitutions.

[AST] Remove requiresSubstitution invariant from Witness

01e8cbd

[Serialization] Update Module Format version

07b9170

xedin force-pushed the rdar-36497404 branch from 27b3927 to 07b9170 Compare April 20, 2018 02:13

xedin changed the title ~~[Serialization] Don't serialize requirement substitution records wit…~~ [Serialization] Refactor normal conformance serialization Apr 20, 2018

DougGregor approved these changes Apr 20, 2018

View reviewed changes

xedin merged commit 2690560 into swiftlang:master Apr 20, 2018

ianpartridge mentioned this pull request Apr 24, 2018

Serializaion error Kitura/Swift-Kuery-ORM#28

Closed

xedin mentioned this pull request Apr 19, 2018

[SR-6754] Swift Compiler Crash using Swift 4.1 toolchain with (!ty->hasError() && "Serializing error type"), function addTypeRef assertion failure #49303

Closed

		@@ -0,0 +1,44 @@
		// RUN: %target-build-swift -emit-module -o %t %s

[Serialization] Refactor normal conformance serialization #16023

[Serialization] Refactor normal conformance serialization #16023

Uh oh!

Conversation

xedin commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Apr 19, 2018

Uh oh!

xedin commented Apr 19, 2018

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xedin Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xedin commented Apr 20, 2018

Uh oh!

xedin commented Apr 20, 2018

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Apr 20, 2018

Uh oh!

Uh oh!

xedin commented Apr 19, 2018 •

edited

Loading

xedin Apr 19, 2018 •

edited

Loading