[Serialization] Handle XREFs to private types (swiftlang#14352)

jrose-apple · web-flow · commit af67204b51a2 · 2018-02-07T16:42:16.000-08:00
We can encounter these when the compiler modifies an inlinable function to break apart a struct and the struct uses a private type for one of its fields. It's questionable whether we /should/ handle this, but meanwhile this /is/ a non-intrusive fix that preserves the performance of non-resilient libraries. (That is, it appears this worked in Swift 4.0, though perhaps not all of the same optimizations kicked in.) https://bugs.swift.org/browse/SR-6874
diff --git a/include/swift/Serialization/ModuleFormat.h b/include/swift/Serialization/ModuleFormat.h
@@ -54,7 +54,7 @@ const uint16_t VERSION_MAJOR = 0;
 /// in source control, you should also update the comment to briefly
 /// describe what change you made. The content of this comment isn't important;
 /// it just ensures a conflict if two people change the module format.
-const uint16_t VERSION_MINOR = 397; // No resilience expansion in SILDeclRef
+const uint16_t VERSION_MINOR = 398; // Private discriminators for type xrefs
 
 using DeclIDField = BCFixed<31>;
 
@@ -1288,6 +1288,7 @@ namespace decls_block {
   using XRefTypePathPieceLayout = BCRecordLayout<
     XREF_TYPE_PATH_PIECE,
     IdentifierIDField, // name
+    IdentifierIDField, // private discriminator
     BCFixed<1>         // restrict to protocol extension
   >;
 
diff --git a/lib/Serialization/Deserialization.cpp b/lib/Serialization/Deserialization.cpp
@@ -1264,18 +1264,22 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
   case XREF_TYPE_PATH_PIECE:
   case XREF_VALUE_PATH_PIECE: {
     IdentifierID IID;
+    IdentifierID privateDiscriminator = 0;
     TypeID TID = 0;
     bool isType = (recordID == XREF_TYPE_PATH_PIECE);
     bool inProtocolExt = false;
     bool isStatic = false;
     if (isType)
-      XRefTypePathPieceLayout::readRecord(scratch, IID, inProtocolExt);
+      XRefTypePathPieceLayout::readRecord(scratch, IID, privateDiscriminator,
+                                          inProtocolExt);
     else
       XRefValuePathPieceLayout::readRecord(scratch, TID, IID, inProtocolExt,
                                            isStatic);
 
     DeclBaseName name = getDeclBaseName(IID);
     pathTrace.addValue(name);
+    if (privateDiscriminator)
+      pathTrace.addValue(getIdentifier(privateDiscriminator));
 
     Type filterTy;
     if (!isType) {
@@ -1290,9 +1294,14 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
       pathTrace.addType(filterTy);
     }
 
-    baseModule->lookupQualified(ModuleType::get(baseModule), name,
-                                NL_QualifiedDefault | NL_KnownNoDependency,
-                                /*typeResolver=*/nullptr, values);
+    if (privateDiscriminator) {
+      baseModule->lookupMember(values, baseModule, name,
+                               getIdentifier(privateDiscriminator));
+    } else {
+      baseModule->lookupQualified(ModuleType::get(baseModule), name,
+                                  NL_QualifiedDefault | NL_KnownNoDependency,
+                                  /*typeResolver=*/nullptr, values);
+    }
     filterValues(filterTy, nullptr, nullptr, isType, inProtocolExt, isStatic,
                  None, values);
     break;
@@ -1348,7 +1357,7 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
       switch (recordID) {
       case XREF_TYPE_PATH_PIECE: {
         IdentifierID IID;
-        XRefTypePathPieceLayout::readRecord(scratch, IID, None);
+        XRefTypePathPieceLayout::readRecord(scratch, IID, None, None);
         result = getIdentifier(IID);
         break;
       }
@@ -1405,8 +1414,13 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
         // Fast path for nested types that avoids deserializing all
         // members of the parent type.
         IdentifierID IID;
+        IdentifierID privateDiscriminator;
         bool onlyInNominal = false;
-        XRefTypePathPieceLayout::readRecord(scratch, IID, onlyInNominal);
+        XRefTypePathPieceLayout::readRecord(scratch, IID, privateDiscriminator,
+                                            onlyInNominal);
+        if (privateDiscriminator)
+          goto giveUpFastPath;
+
         Identifier memberName = getIdentifier(IID);
         pathTrace.addValue(memberName);
 
@@ -1449,21 +1463,25 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
 
         pathTrace.removeLast();
       }
+giveUpFastPath:
       LLVM_FALLTHROUGH;
     }
     case XREF_VALUE_PATH_PIECE:
     case XREF_INITIALIZER_PATH_PIECE: {
       TypeID TID = 0;
       DeclBaseName memberName;
+      Identifier privateDiscriminator;
       Optional<swift::CtorInitializerKind> ctorInit;
       bool isType = false;
       bool inProtocolExt = false;
       bool isStatic = false;
       switch (recordID) {
       case XREF_TYPE_PATH_PIECE: {
-        IdentifierID IID;
-        XRefTypePathPieceLayout::readRecord(scratch, IID, inProtocolExt);
+        IdentifierID IID, discriminatorID;
+        XRefTypePathPieceLayout::readRecord(scratch, IID, discriminatorID,
+                                            inProtocolExt);
         memberName = getDeclBaseName(IID);
+        privateDiscriminator = getIdentifier(discriminatorID);
         isType = true;
         break;
       }
@@ -1490,6 +1508,8 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
       }
 
       pathTrace.addValue(memberName);
+      if (!privateDiscriminator.empty())
+        pathTrace.addPrivateDiscriminator(privateDiscriminator);
 
       Type filterTy;
       if (!isType) {
@@ -1519,8 +1539,17 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
                                            getXRefDeclNameForError());
       }
 
-      auto members = nominal->lookupDirect(memberName);
-      values.append(members.begin(), members.end());
+      if (!privateDiscriminator.empty()) {
+        ModuleDecl *searchModule = M;
+        if (!searchModule)
+          searchModule = nominal->getModuleContext();
+        searchModule->lookupMember(values, nominal, memberName,
+                                   privateDiscriminator);
+
+      } else {
+        auto members = nominal->lookupDirect(memberName);
+        values.append(members.begin(), members.end());
+      }
       filterValues(filterTy, M, genericSig, isType, inProtocolExt, isStatic,
                    ctorInit, values);
       break;
diff --git a/lib/Serialization/DeserializationErrors.h b/lib/Serialization/DeserializationErrors.h
@@ -37,6 +37,7 @@ class XRefTracePath {
       Accessor,
       Extension,
       GenericParam,
+      PrivateDiscriminator,
       Unknown
     };
 
@@ -55,11 +56,12 @@ class XRefTracePath {
       : kind(K),
         data(llvm::PointerLikeTypeTraits<T>::getAsVoidPointer(value)) {}
 
-    Identifier getAsIdentifier() const {
+    DeclBaseName getAsBaseName() const {
       switch (kind) {
       case Kind::Value:
       case Kind::Operator:
-        return getDataAs<Identifier>();
+      case Kind::PrivateDiscriminator:
+        return getDataAs<DeclBaseName>();
       case Kind::Type:
       case Kind::OperatorFilter:
       case Kind::Accessor:
@@ -73,7 +75,7 @@ class XRefTracePath {
     void print(raw_ostream &os) const {
       switch (kind) {
       case Kind::Value:
-        os << getDataAs<Identifier>();
+        os << getDataAs<DeclBaseName>();
         break;
       case Kind::Type:
         os << "with type " << getDataAs<Type>();
@@ -137,6 +139,9 @@ class XRefTracePath {
       case Kind::GenericParam:
         os << "generic param #" << getDataAs<uintptr_t>();
         break;
+      case Kind::PrivateDiscriminator:
+        os << "(in " << getDataAs<Identifier>() << ")";
+        break;
       case Kind::Unknown:
         os << "unknown xref kind " << getDataAs<uintptr_t>();
         break;
@@ -180,17 +185,21 @@ class XRefTracePath {
     path.push_back({ PathPiece::Kind::GenericParam, index });
   }
 
+  void addPrivateDiscriminator(Identifier name) {
+    path.push_back({ PathPiece::Kind::PrivateDiscriminator, name });
+  }
+
   void addUnknown(uintptr_t kind) {
     path.push_back({ PathPiece::Kind::Unknown, kind });
   }
 
-  Identifier getLastName() const {
+  DeclBaseName getLastName() const {
     for (auto &piece : reversed(path)) {
-      Identifier result = piece.getAsIdentifier();
+      DeclBaseName result = piece.getAsBaseName();
       if (!result.empty())
         return result;
     }
-    return Identifier();
+    return DeclBaseName();
   }
 
   void removeLast() {
diff --git a/lib/Serialization/Serialization.cpp b/lib/Serialization/Serialization.cpp
@@ -1868,8 +1868,16 @@ void Serializer::writeCrossReference(const DeclContext *DC, uint32_t pathLen) {
 
     auto generic = cast<GenericTypeDecl>(DC);
     abbrCode = DeclTypeAbbrCodes[XRefTypePathPieceLayout::Code];
+
+    Identifier discriminator;
+    if (generic->isOutermostPrivateOrFilePrivateScope()) {
+      auto *containingFile = cast<FileUnit>(generic->getModuleScopeContext());
+      discriminator = containingFile->getDiscriminatorForPrivateValue(generic);
+    }
+
     XRefTypePathPieceLayout::emitRecord(Out, ScratchRecord, abbrCode,
                                         addDeclBaseNameRef(generic->getName()),
+                                        addDeclBaseNameRef(discriminator),
                                         false);
     break;
   }
@@ -2019,8 +2027,17 @@ void Serializer::writeCrossReference(const Decl *D) {
   bool isProtocolExt = D->getDeclContext()->getAsProtocolExtensionContext();
   if (auto type = dyn_cast<TypeDecl>(D)) {
     abbrCode = DeclTypeAbbrCodes[XRefTypePathPieceLayout::Code];
+
+    Identifier discriminator;
+    if (type->isOutermostPrivateOrFilePrivateScope()) {
+      auto *containingFile =
+         cast<FileUnit>(type->getDeclContext()->getModuleScopeContext());
+      discriminator = containingFile->getDiscriminatorForPrivateValue(type);
+    }
+
     XRefTypePathPieceLayout::emitRecord(Out, ScratchRecord, abbrCode,
                                         addDeclBaseNameRef(type->getName()),
+                                        addDeclBaseNameRef(discriminator),
                                         isProtocolExt);
     return;
   }
diff --git a/test/Serialization/Inputs/xref-private-type/Lib.swift b/test/Serialization/Inputs/xref-private-type/Lib.swift
@@ -0,0 +1,8 @@
+import LibCore
+
+public let lazyFoo = Foo()
+public func testFoo(_: Foo = lazyFoo) {}
+public let lazyBar = Bar()
+public func testBar(_: Bar = lazyBar) {}
+public let lazyBaz = Baz()
+public func testBaz(_: Baz = lazyBaz) {}
diff --git a/test/Serialization/Inputs/xref-private-type/LibCore.swift b/test/Serialization/Inputs/xref-private-type/LibCore.swift
@@ -0,0 +1,24 @@
+private class TopLevelInternalClass {}
+public struct Foo {
+  private var ref: TopLevelInternalClass
+  
+  public init() { self.ref = .init() }
+}
+
+public struct Bar {
+  private class NestedInternalClass {}
+
+  private var ref: NestedInternalClass
+  
+  public init() { self.ref = .init() }
+}
+
+public struct Baz {
+  fileprivate class NestedInternalClass {
+    fileprivate class DoublyNestedInternalClass {}
+  }
+
+  private var ref: NestedInternalClass.DoublyNestedInternalClass
+  
+  public init() { self.ref = .init() }
+}
diff --git a/test/Serialization/xref-private-type.swift b/test/Serialization/xref-private-type.swift
@@ -0,0 +1,51 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-build-swift -swift-version 4 -O  -force-single-frontend-invocation -emit-module-path %t/LibCore.swiftmodule %S/Inputs/xref-private-type/LibCore.swift
+// RUN: %target-build-swift -swift-version 4 -O -I %t  -force-single-frontend-invocation -emit-module-path %t/Lib.swiftmodule %S/Inputs/xref-private-type/Lib.swift 
+// RUN: %target-build-swift -swift-version 4 -O -I %t -emit-sil %s | %FileCheck %s
+
+import Lib
+
+// CHECK: sil{{.*}} @[[TESTSR6874:[^ ]+10testSR6874[^ ]+]] :
+func testSR6874() {
+  // The important lines in this test are the strong_retains, which refer to
+  // private types defined in LibCore. Normally we shouldn't have references to
+  // non-public declarations in inlinable code, but because SIL passes can break
+  // apart non-resilient structs and enums we can end up in that situation.
+  // Worse, this can happen *across module boundaries.*
+  //
+  // In this test, the addressor for each global defined in Lib ends up
+  // referencing private types defined in LibCore. Using those globals in
+  // default argument position leads to the addressor getting inlined into
+  // calling code in Swift 4 and later. This results in an attempt to not just
+  // reference a private type, but to *resolve a cross-reference to a private
+  // type.*
+  //
+  // This is the situation in SR-6874 (simplified). I'm not sure of a simpler
+  // way to reliably trigger the issue. But if this test breaks, please try to
+  // find one.
+  //
+  // (We may want to revisit this whole thing later, as it violates the model.
+  // But it's also useful for performance in non-resilient code.)
+
+  // CHECK: [[ADDR:%.+]] = global_addr @{{[^ ]+}}3Lib7lazyFoo
+  // CHECK: [[LOADED:%.+]] = load [[ADDR]] : $*Foo
+  // CHECK: [[REF:%.+]] = struct_extract [[LOADED]] : $Foo, #Foo.ref
+  // CHECK: strong_retain [[REF]] : $TopLevelInternalClass
+  // CHECK: apply {{%.+}}([[LOADED]])
+  testFoo()
+
+  // CHECK: [[ADDR:%.+]] = global_addr @{{[^ ]+}}3Lib7lazyBar
+  // CHECK: [[LOADED:%.+]] = load [[ADDR]] : $*Bar
+  // CHECK: [[REF:%.+]] = struct_extract [[LOADED]] : $Bar, #Bar.ref
+  // CHECK: strong_retain [[REF]] : $Bar.NestedInternalClass
+  // CHECK: apply {{%.+}}([[LOADED]])
+  testBar()
+
+  // CHECK: [[ADDR:%.+]] = global_addr @{{[^ ]+}}3Lib7lazyBaz
+  // CHECK: [[LOADED:%.+]] = load [[ADDR]] : $*Baz
+  // CHECK: [[REF:%.+]] = struct_extract [[LOADED]] : $Baz, #Baz.ref
+  // CHECK: strong_retain [[REF]] : $Baz.NestedInternalClass.DoublyNestedInternalClass
+  // CHECK: apply {{%.+}}([[LOADED]])
+  testBaz()
+} // CHECK: end sil function '[[TESTSR6874]]'
+
