Merge pull request #16057 from slavapestov/codable-vtable-layout

Fix Codable vtable layout

Author: slavapestov

include/swift/SIL/SILVTableVisitor.h

@@ -18,11 +18,55 @@
 #ifndef SWIFT_SIL_SILVTABLEVISITOR_H
 #define SWIFT_SIL_SILVTABLEVISITOR_H
 
+#include <string>
+
 #include "swift/AST/Decl.h"
 #include "swift/AST/Types.h"
+#include "swift/AST/ASTMangler.h"
 
 namespace swift {
 
+// Utility class for deterministically ordering vtable entries for
+// synthesized methods.
+struct SortedFuncList {
+  using Entry = std::pair<std::string, AbstractFunctionDecl *>;
+  SmallVector<Entry, 2> elts;
+  bool sorted = false;
+
+  void add(AbstractFunctionDecl *afd) {
+    Mangle::ASTMangler mangler;
+    std::string mangledName;
+    if (auto *cd = dyn_cast<ConstructorDecl>(afd))
+      mangledName = mangler.mangleConstructorEntity(cd, 0, 0);
+    else
+      mangledName = mangler.mangleEntity(afd, 0);
+
+    elts.push_back(std::make_pair(mangledName, afd));
+  }
+
+  bool empty() { return elts.empty(); }
+
+  void sort() {
+    assert(!sorted);
+    sorted = true;
+    std::sort(elts.begin(),
+              elts.end(),
+              [](const Entry &lhs, const Entry &rhs) -> bool {
+                return lhs.first < rhs.first;
+              });
+  }
+
+  decltype(elts)::const_iterator begin() const {
+    assert(sorted);
+    return elts.begin();
+  }
+
+  decltype(elts)::const_iterator end() const {
+    assert(sorted);
+    return elts.end();
+  }
+};
+
 /// A CRTP class for visiting virtually-dispatched methods of a class.
 ///
 /// You must override these two methods in your subclass:
@@ -76,19 +120,47 @@ template <class T> class SILVTableVisitor {
     }
   }
 
+  void maybeAddMember(Decl *member) {
+    if (auto *fd = dyn_cast<FuncDecl>(member))
+      maybeAddMethod(fd);
+    else if (auto *cd = dyn_cast<ConstructorDecl>(member))
+      maybeAddConstructor(cd);
+    else if (auto *placeholder = dyn_cast<MissingMemberDecl>(member))
+      asDerived().addPlaceholder(placeholder);
+  }
+
 protected:
   void addVTableEntries(ClassDecl *theClass) {
     // Imported classes do not have a vtable.
     if (!theClass->hasKnownSwiftImplementation())
       return;
 
+    // Note that while vtable order is not ABI, we still want it to be
+    // consistent between translation units.
+    //
+    // So, sort synthesized members by their mangled name, since they
+    // are added lazily during type checking, with the remaining ones
+    // forced at the end.
+    SortedFuncList synthesizedMembers;
+
     for (auto member : theClass->getMembers()) {
-      if (auto *fd = dyn_cast<FuncDecl>(member))
-        maybeAddMethod(fd);
-      else if (auto *cd = dyn_cast<ConstructorDecl>(member))
-        maybeAddConstructor(cd);
-      else if (auto *placeholder = dyn_cast<MissingMemberDecl>(member))
-        asDerived().addPlaceholder(placeholder);
+      if (auto *afd = dyn_cast<AbstractFunctionDecl>(member)) {
+        if (afd->isSynthesized()) {
+          synthesizedMembers.add(afd);
+          continue;
+        }
+      }
+
+      maybeAddMember(member);
+    }
+
+    if (synthesizedMembers.empty())
+      return;
+
+    synthesizedMembers.sort();
+
+    for (const auto &pair : synthesizedMembers) {
+      maybeAddMember(pair.second);
     }
   }
 };

lib/Sema/TypeCheckDecl.cpp

@@ -7992,9 +7992,6 @@ static void finalizeType(TypeChecker &TC, NominalTypeDecl *nominal) {
   assert(!nominal->hasClangNode());
   assert(isa<SourceFile>(nominal->getModuleScopeContext()));
 
-  if (auto *classDecl = dyn_cast<ClassDecl>(nominal))
-    TC.requestSuperclassLayout(classDecl);
-
   for (auto *D : nominal->getMembers()) {
     auto VD = dyn_cast<ValueDecl>(D);
     if (!VD)
@@ -8017,12 +8014,33 @@ static void finalizeType(TypeChecker &TC, NominalTypeDecl *nominal) {
     }
   }
 
-  // FIXME: We need to add implicit initializers and dtors when a decl is
-  // touched, because it affects vtable layout.  If you're not defining the
-  // class, you shouldn't have to know what the vtable layout is.
   if (auto *CD = dyn_cast<ClassDecl>(nominal)) {
+    // We need to add implicit initializers and dtors because it
+    // affects vtable layout.
     TC.addImplicitConstructors(CD);
     CD->addImplicitDestructor();
+
+    // We need the superclass vtable layout as well.
+    TC.requestSuperclassLayout(CD);
+
+    auto useConformance = [&](ProtocolDecl *protocol) {
+      if (auto ref = TC.conformsToProtocol(
+            CD->getDeclaredInterfaceType(), protocol, CD,
+            ConformanceCheckFlags::SkipConditionalRequirements,
+            SourceLoc())) {
+        if (ref->getConcrete()->getDeclContext() == CD)
+          TC.markConformanceUsed(*ref, CD);
+      }
+    };
+
+    // If the class is Encodable or Decodable, force those
+    // conformances to ensure that the init(from:) and
+    // encode(to:) members appear in the vtable.
+    //
+    // FIXME: Generalize this to other protocols for which
+    // we can derive conformances.
+    useConformance(TC.Context.getProtocol(KnownProtocolKind::Decodable));
+    useConformance(TC.Context.getProtocol(KnownProtocolKind::Encodable));
   }
 
   // validateDeclForNameLookup will not trigger an immediate full

test/stdlib/CodableMultifile.swift

@@ -0,0 +1,79 @@
+// RUN: %empty-directory(%t)
+// RUN: cp %s %t/main.swift
+// RUN: %target-build-swift %t/main.swift %S/Inputs/CodableMultifileOther.swift -module-name main -o %t/main
+// RUN: %target-run %t/main
+// REQUIRES: executable_test
+
+// FIXME: This test could run on Linux too, if we could either use
+// corelibs-foundation, or implement a mock Encoder for testing.
+
+// REQUIRES: objc_interop
+
+import StdlibUnittest
+import Foundation
+
+var CodableMultifileTestSuite = TestSuite("CodableMultifile")
+
+// The first test doesn't synthesize encode(to:) at all.
+CodableMultifileTestSuite.test("1") {
+  let derived = DerivedFirst()
+
+  // Make sure the vtable offset matches between this translation unit and
+  // the other file, which requires us to force the encode(to:) member when
+  // type checking this translation unit.
+  expectEqual(false, derived.derivedMember)
+}
+
+// The second test synthesizes init(from:) before encode(to:).
+
+// We define a wrapper so that the virtual method BaseSecond.encode(to:) method
+// is called from outside its module. If we use the conformance of BaseSecond
+// to Codable, we don't expose the bug because the virtual method call is
+// encapsulated in the conformance, which is emitted in the same translation unit
+// as BaseSecond.
+struct WrapperSecond : Encodable {
+  let base: BaseSecond
+
+  func encode(to encoder: Encoder) throws {
+    try base.encode(to: encoder)
+  }
+}
+
+CodableMultifileTestSuite.test("2") {
+  // Make sure we synthesize the init(from:) member before encode(to:) in
+  // this translation unit.
+  _ = BaseSecond.init(from:)
+
+  let base = WrapperSecond(base: BaseSecond())
+  let encoder = JSONEncoder()
+
+  expectEqual(
+    "{\"baseMember\":2}",
+    String(data: try! encoder.encode(base), encoding: .utf8)!)
+}
+
+// The third test synthesizes encode(to:) before init(from:).
+
+// See above.
+struct WrapperThird : Encodable {
+  let base: BaseThird
+
+  func encode(to encoder: Encoder) throws {
+    try base.encode(to: encoder)
+  }
+}
+
+CodableMultifileTestSuite.test("3") {
+  // Make sure we synthesize the encode(to:) member before init(from:) in
+  // this translation unit.
+  _ = BaseThird.encode(to:)
+
+  let base = WrapperThird(base: BaseThird())
+  let encoder = JSONEncoder()
+
+  expectEqual(
+    "{\"baseMember\":3}",
+    String(data: try! encoder.encode(base), encoding: .utf8)!)
+}
+
+runAllTests()

test/stdlib/Inputs/CodableMultifileOther.swift

@@ -0,0 +1,21 @@
+class BaseFirst: Codable {
+  var baseMember = 1
+
+  init() {}
+}
+
+class DerivedFirst: BaseFirst {
+  var derivedMember = false
+}
+
+class BaseSecond: Codable {
+  var baseMember = 2
+
+  init() {}
+}
+
+class BaseThird: Codable {
+  var baseMember = 3
+
+  init() {}
+}