Merge pull request #78166 from swiftlang/egorzhdan/derived-out-of-order

[cxx-interop] Fix memory layout for structs with out-of-order base types

Author: egorzhdan

lib/IRGen/GenClangDecl.cpp

@@ -22,6 +22,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/GlobalDecl.h"
+#include "clang/AST/RecordLayout.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/CodeGen/ModuleBuilder.h"
 #include "clang/Sema/Sema.h"
@@ -366,3 +367,41 @@ void IRGenModule::ensureImplicitCXXDestructorBodyIsDefined(
   auto &sema = Context.getClangModuleLoader()->getClangSema();
   sema.DefineImplicitDestructor(clang::SourceLocation(), destructor);
 }
+
+/// Retrieves the base classes of a C++ struct/class ordered by their offset in
+/// the derived type's memory layout.
+SmallVector<CXXBaseRecordLayout, 1>
+irgen::getBasesAndOffsets(const clang::CXXRecordDecl *decl) {
+  auto &layout = decl->getASTContext().getASTRecordLayout(decl);
+
+  // Collect the offsets and sizes of base classes within the memory layout
+  // of the derived class.
+  SmallVector<CXXBaseRecordLayout, 1> baseOffsetsAndSizes;
+  for (auto base : decl->bases()) {
+    if (base.isVirtual())
+      continue;
+
+    auto baseType = base.getType().getCanonicalType();
+    auto baseRecordType = cast<clang::RecordType>(baseType);
+    auto baseRecord = baseRecordType->getAsCXXRecordDecl();
+    assert(baseRecord && "expected a base C++ record");
+
+    if (baseRecord->isEmpty())
+      continue;
+
+    auto offset = Size(layout.getBaseClassOffset(baseRecord).getQuantity());
+    auto size =
+        Size(decl->getASTContext().getTypeSizeInChars(baseType).getQuantity());
+
+    baseOffsetsAndSizes.push_back({baseRecord, offset, size});
+  }
+
+  // In C++, base classes might get reordered if the primary base was not
+  // the first base type on the declaration of the class.
+  llvm::sort(baseOffsetsAndSizes, [](const CXXBaseRecordLayout &lhs,
+                                     const CXXBaseRecordLayout &rhs) {
+    return lhs.offset < rhs.offset;
+  });
+
+  return baseOffsetsAndSizes;
+}

lib/IRGen/GenClass.cpp

@@ -294,32 +294,18 @@ namespace {
       if (!cxxRecord)
         return;
 
-      auto &layout = cxxRecord->getASTContext().getASTRecordLayout(cxxRecord);
-
-      for (auto base : cxxRecord->bases()) {
-        if (base.isVirtual())
-          continue;
-
-        auto baseType = base.getType().getCanonicalType();
-
-        auto baseRecord = cast<clang::RecordType>(baseType)->getDecl();
-        auto baseCxxRecord = cast<clang::CXXRecordDecl>(baseRecord);
-
-        if (baseCxxRecord->isEmpty())
-          continue;
-
-        auto offset = Size(layout.getBaseClassOffset(baseCxxRecord).getQuantity());
-        auto size = Size(cxxRecord->getASTContext().getTypeSizeInChars(baseType).getQuantity());
-
-        if (offset != CurSize) {
-          assert(offset > CurSize);
-          auto paddingSize = offset - CurSize;
-          auto &opaqueTI = IGM.getOpaqueStorageTypeInfo(paddingSize, Alignment(1));
+      auto bases = getBasesAndOffsets(cxxRecord);
+      for (auto base : bases) {
+        if (base.offset != CurSize) {
+          assert(base.offset > CurSize);
+          auto paddingSize = base.offset - CurSize;
+          auto &opaqueTI =
+              IGM.getOpaqueStorageTypeInfo(paddingSize, Alignment(1));
           auto element = ElementLayout::getIncomplete(opaqueTI);
           addField(element, LayoutStrategy::Universal);
         }
 
-        auto &opaqueTI = IGM.getOpaqueStorageTypeInfo(size, Alignment(1));
+        auto &opaqueTI = IGM.getOpaqueStorageTypeInfo(base.size, Alignment(1));
         auto element = ElementLayout::getIncomplete(opaqueTI);
         addField(element, LayoutStrategy::Universal);
       }

lib/IRGen/GenStruct.cpp

@@ -368,28 +368,6 @@ namespace {
                                   ClangFieldInfo> {
     const clang::RecordDecl *ClangDecl;
 
-    template <class Fn>
-    void forEachNonEmptyBase(Fn fn) const {
-      auto &layout = ClangDecl->getASTContext().getASTRecordLayout(ClangDecl);
-
-      if (auto cxxRecord = dyn_cast<clang::CXXRecordDecl>(ClangDecl)) {
-        for (auto base : cxxRecord->bases()) {
-          auto baseType = base.getType().getCanonicalType();
-
-          auto baseRecord = cast<clang::RecordType>(baseType)->getDecl();
-          auto baseCxxRecord = cast<clang::CXXRecordDecl>(baseRecord);
-
-          if (baseCxxRecord->isEmpty())
-            continue;
-
-          auto offset = layout.getBaseClassOffset(baseCxxRecord);
-          auto size =
-              ClangDecl->getASTContext().getTypeSizeInChars(baseType);
-          fn(baseType, offset, size);
-        }
-      }
-    }
-
   public:
     LoadableClangRecordTypeInfo(ArrayRef<ClangFieldInfo> fields,
                                 unsigned explosionSize, llvm::Type *storageType,
@@ -446,10 +424,11 @@ namespace {
 
     void addToAggLowering(IRGenModule &IGM, SwiftAggLowering &lowering,
                           Size offset) const override {
-      forEachNonEmptyBase([&](clang::QualType type, clang::CharUnits offset,
-                              clang::CharUnits) {
-        lowering.addTypedData(type, offset);
-      });
+      if (auto cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(ClangDecl)) {
+        for (auto base : getBasesAndOffsets(cxxRecordDecl)) {
+          lowering.addTypedData(base.decl, base.offset.asCharUnits());
+        }
+      }
 
       lowering.addTypedData(ClangDecl, offset.asCharUnits());
     }
@@ -1384,23 +1363,12 @@ class ClangRecordLowering {
   void collectBases(const clang::RecordDecl *decl) {
     auto &layout = decl->getASTContext().getASTRecordLayout(decl);
     if (auto cxxRecord = dyn_cast<clang::CXXRecordDecl>(decl)) {
-      for (auto base : cxxRecord->bases()) {
-        if (base.isVirtual())
-          continue;
-
-        auto baseType = base.getType().getCanonicalType();
-
-        auto baseRecord = cast<clang::RecordType>(baseType)->getDecl();
-        auto baseCxxRecord = cast<clang::CXXRecordDecl>(baseRecord);
-
-        if (baseCxxRecord->isEmpty())
-          continue;
-
-        auto baseOffset = Size(layout.getBaseClassOffset(baseCxxRecord).getQuantity());
-        SubobjectAdjustment += baseOffset;
-        collectBases(baseCxxRecord);
-        collectStructFields(baseCxxRecord);
-        SubobjectAdjustment -= baseOffset;
+      auto bases = getBasesAndOffsets(cxxRecord);
+      for (auto base : bases) {
+        SubobjectAdjustment += base.offset;
+        collectBases(base.decl);
+        collectStructFields(base.decl);
+        SubobjectAdjustment -= base.offset;
       }
     }
   }

lib/IRGen/IRGenModule.h

@@ -2029,6 +2029,17 @@ class CurrentIGMPtr {
 /// Workaround to disable thumb-mode until debugger support is there.
 bool shouldRemoveTargetFeature(StringRef);
 
+struct CXXBaseRecordLayout {
+  const clang::CXXRecordDecl *decl;
+  Size offset;
+  Size size;
+};
+
+/// Retrieves the base classes of a C++ struct/class ordered by their offset in
+/// the derived type's memory layout.
+SmallVector<CXXBaseRecordLayout, 1>
+getBasesAndOffsets(const clang::CXXRecordDecl *decl);
+
 } // end namespace irgen
 } // end namespace swift
 

test/Interop/Cxx/class/inheritance/Inputs/fields.h

@@ -132,3 +132,27 @@ struct DerivedHasTailPadding : public BaseAlign8 {
 struct DerivedUsesBaseTailPadding : public DerivedHasTailPadding {
   short field2 = 789;
 }; // sizeof=16, dsize=14, align=8
+
+// MARK: Types with an out-of-order inheritance.
+
+struct BaseWithVirtualDestructor {
+  int baseField = 123;
+
+  virtual ~BaseWithVirtualDestructor() {}
+};
+
+struct DerivedWithVirtualDestructor : public BaseWithVirtualDestructor {
+  int derivedField = 456;
+
+  ~DerivedWithVirtualDestructor() override {}
+};
+
+struct DerivedOutOfOrder : public HasOneField,
+                           public DerivedWithVirtualDestructor {
+  // DerivedWithVirtualDestructor is the primary base class despite being the
+  // second one the list.
+
+  int leafField = 789;
+
+  ~DerivedOutOfOrder() override {}
+};

test/Interop/Cxx/class/inheritance/fields.swift

@@ -124,4 +124,11 @@ FieldsTestSuite.test("Field in tail padding of base class") {
   expectEqual(usesBaseTailPadding.field8, 123)
 }
 
+FieldsTestSuite.test("Out-of-order inheritance") {
+  let d = DerivedOutOfOrder()
+  expectEqual(d.leafField, 789)
+  expectEqual(d.derivedField, 456)
+  expectEqual(d.baseField, 123)
+}
+
 runAllTests()

test/Interop/Cxx/foreign-reference/Inputs/inheritance.h

@@ -27,3 +27,44 @@ SubT : BaseT {
     SubT(const SubT &) = delete;
     static SubT &getSubT() { static SubT singleton; return singleton; }
 };
+
+struct
+__attribute__((swift_attr("import_reference")))
+__attribute__((swift_attr("retain:immortal")))
+__attribute__((swift_attr("release:immortal")))
+BaseWithVirtualDestructor {
+    int baseField = 123;
+
+    BaseWithVirtualDestructor() {}
+    virtual ~BaseWithVirtualDestructor() {}
+};
+
+struct
+__attribute__((swift_attr("import_reference")))
+__attribute__((swift_attr("retain:immortal")))
+__attribute__((swift_attr("release:immortal")))
+DerivedWithVirtualDestructor : public BaseWithVirtualDestructor {
+    int derivedField = 456;
+
+    DerivedWithVirtualDestructor() : BaseWithVirtualDestructor() {}
+    ~DerivedWithVirtualDestructor() override {}
+};
+
+struct
+__attribute__((swift_attr("import_reference")))
+__attribute__((swift_attr("retain:immortal")))
+__attribute__((swift_attr("release:immortal")))
+DerivedOutOfOrder : public BaseT, public DerivedWithVirtualDestructor {
+    // DerivedWithVirtualDestructor is the primary base class despite being the
+    // second one the list.
+
+    int leafField = 789; 
+
+    DerivedOutOfOrder() = default;
+    ~DerivedOutOfOrder() override {}
+
+    static DerivedOutOfOrder& getInstance() {
+      static DerivedOutOfOrder singleton;
+      return singleton;
+    }
+};

test/Interop/Cxx/foreign-reference/inheritance-irgen.swift

@@ -0,0 +1,14 @@
+// RUN: %target-swift-emit-ir %s -I %S/Inputs -enable-experimental-cxx-interop -Xcc -fignore-exceptions -disable-availability-checking
+
+// This ensured we do not crash during IRGen for inherited C++ foreign reference types.
+
+import Inheritance
+
+@inline(never)
+public func blackHole<T>(_ _: T) {}
+
+let x = DerivedOutOfOrder.getInstance()
+
+blackHole(x.baseField)
+blackHole(x.derivedField)
+blackHole(x.leafField)

test/Interop/Cxx/foreign-reference/inheritance-typechecker.swift

@@ -21,3 +21,8 @@ let b: BaseT = BaseT.getBaseT()
 assert(b.isBase)
 let bc: BaseT = cxxCast(b)      // should instantiate I and O both to BaseT
 assert(bc.isBase)
+
+let d = DerivedOutOfOrder.getInstance()
+let _ = d.baseField
+let _ = d.derivedField
+let _ = d.leafField

test/Interop/Cxx/foreign-reference/inheritance.swift

@@ -30,4 +30,11 @@ TemplatingTestSuite.test("BaseT") {
     expectTrue(bc.isBase)
 }
 
+TemplatingTestSuite.test("DerivedOutOfOrder") {
+    let d = DerivedOutOfOrder.getInstance()
+    expectEqual(123, d.baseField)
+    expectEqual(456, d.derivedField)
+    expectEqual(789, d.leafField)
+}
+
 runAllTests()