Merge pull request #75176 from swiftlang/gaborh/inheritance-2

[cxx-interop] Fix wrong field offsets with value types using inheritance

Author: Xazax-hun

lib/IRGen/GenStruct.cpp

@@ -29,15 +29,19 @@
 #include "swift/SIL/SILModule.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Attr.h"
+#include "clang/AST/CharUnits.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
 #include "clang/AST/GlobalDecl.h"
 #include "clang/AST/Mangle.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/CodeGen/CodeGenABITypes.h"
 #include "clang/CodeGen/SwiftCallingConv.h"
 #include "clang/Sema/Sema.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Function.h"
+#include <iterator>
 
 #include "GenDecl.h"
 #include "GenMeta.h"
@@ -1310,6 +1314,7 @@ class ClangRecordLowering {
   SmallVector<llvm::Type *, 8> LLVMFields;
   SmallVector<ClangFieldInfo, 8> FieldInfos;
   Size NextOffset = Size(0);
+  Size SubobjectAdjustment = Size(0);
   unsigned NextExplosionIndex = 0;
 public:
   ClangRecordLowering(IRGenModule &IGM, StructDecl *swiftDecl,
@@ -1327,8 +1332,8 @@ class ClangRecordLowering {
     if (ClangDecl->isUnion()) {
       collectUnionFields();
     } else {
-      collectBases();
-      collectStructFields();
+      collectBases(ClangDecl);
+      collectStructFields(ClangDecl);
     }
   }
 
@@ -1359,10 +1364,9 @@ class ClangRecordLowering {
     return (swiftField->getClangNode().castAsDecl() == clangField);
   }
 
-  void collectBases() {
-    auto &layout = ClangDecl->getASTContext().getASTRecordLayout(ClangDecl);
-
-    if (auto cxxRecord = dyn_cast<clang::CXXRecordDecl>(ClangDecl)) {
+  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;
@@ -1375,17 +1379,24 @@ class ClangRecordLowering {
         if (baseCxxRecord->isEmpty())
           continue;
 
-        auto offset = layout.getBaseClassOffset(baseCxxRecord);
-        auto size = ClangDecl->getASTContext().getTypeSizeInChars(baseType);
-        addOpaqueField(Size(offset.getQuantity()), Size(size.getQuantity()));
+        auto baseOffset = Size(layout.getBaseClassOffset(baseCxxRecord).getQuantity());
+        SubobjectAdjustment += baseOffset;
+        collectBases(baseCxxRecord);
+        collectStructFields(baseCxxRecord);
+        SubobjectAdjustment -= baseOffset;
       }
     }
   }
 
-  void collectStructFields() {
-    auto cfi = ClangDecl->field_begin(), cfe = ClangDecl->field_end();
+  void collectStructFields(const clang::RecordDecl *decl) {
+    auto cfi = decl->field_begin(), cfe = decl->field_end();
+    const auto &layout = ClangContext.getASTRecordLayout(decl);
     auto swiftProperties = SwiftDecl->getStoredProperties();
     auto sfi = swiftProperties.begin(), sfe = swiftProperties.end();
+    // When collecting fields from the base subobjects, we do not have corresponding swift
+    // stored properties.
+    if (decl != ClangDecl)
+      sfi = swiftProperties.end();
 
     while (cfi != cfe) {
       const clang::FieldDecl *clangField = *cfi++;
@@ -1395,13 +1406,13 @@ class ClangRecordLowering {
       if (clangField->isBitField()) {
         // Collect all of the following bitfields.
         unsigned bitStart =
-          ClangLayout.getFieldOffset(clangField->getFieldIndex());
+          layout.getFieldOffset(clangField->getFieldIndex());
         unsigned bitEnd = bitStart + clangField->getBitWidthValue(ClangContext);
 
         while (cfi != cfe && (*cfi)->isBitField()) {
           clangField = *cfi++;
           unsigned nextStart =
-            ClangLayout.getFieldOffset(clangField->getFieldIndex());
+            layout.getFieldOffset(clangField->getFieldIndex());
           assert(nextStart >= bitEnd && "laying out bit-fields out of order?");
 
           // In a heuristic effort to reduce the number of weird-sized
@@ -1419,41 +1430,41 @@ class ClangRecordLowering {
         continue;
       }
 
-      VarDecl *swiftField;
+      VarDecl *swiftField = nullptr;
       if (sfi != sfe) {
         swiftField = *sfi;
         if (isImportOfClangField(swiftField, clangField)) {
           ++sfi;
         } else {
           swiftField = nullptr;
         }
-      } else {
-        swiftField = nullptr;
-      }
+      } 
 
       // Try to position this field.  If this fails, it's because we
       // didn't lay out padding correctly.
-      addStructField(clangField, swiftField);
+      addStructField(clangField, swiftField, layout);
     }
 
     assert(sfi == sfe && "more Swift fields than there were Clang fields?");
 
+    Size objectTotalStride = Size(layout.getSize().getQuantity());
     // We never take advantage of tail padding, because that would prevent
     // us from passing the address of the object off to C, which is a pretty
     // likely scenario for imported C types.
-    assert(NextOffset <= TotalStride);
-    assert(SpareBits.size() <= TotalStride.getValueInBits());
-    if (NextOffset < TotalStride) {
-      addPaddingField(TotalStride);
+    assert(NextOffset <= SubobjectAdjustment + objectTotalStride);
+    assert(SpareBits.size() <= SubobjectAdjustment.getValueInBits() +
+                                   objectTotalStride.getValueInBits());
+    if (NextOffset < objectTotalStride) {
+      addPaddingField(objectTotalStride);
     }
   }
 
   /// Place the next struct field at its appropriate offset.
   void addStructField(const clang::FieldDecl *clangField,
-                      VarDecl *swiftField) {
-    unsigned fieldOffset = ClangLayout.getFieldOffset(clangField->getFieldIndex());
+                      VarDecl *swiftField, const clang::ASTRecordLayout &layout) {
+    unsigned fieldOffset = layout.getFieldOffset(clangField->getFieldIndex());
     assert(!clangField->isBitField());
-    Size offset(fieldOffset / 8);
+    Size offset( SubobjectAdjustment.getValue() + fieldOffset / 8);
 
     // If we have a Swift import of this type, use our lowered information.
     if (swiftField) {

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

@@ -100,3 +100,21 @@ class CopyTrackedDerivedDerivedClass: public NonEmptyBase, public CopyTrackedDer
 public:
     CopyTrackedDerivedDerivedClass(int x) : CopyTrackedDerivedClass(x) {}
 };
+
+// Types with virtual methods, make sure field offsets are right. rdar://126754931
+
+struct HasOneFieldWithVirtualMethod {
+  int a;
+  virtual ~HasOneFieldWithVirtualMethod() {}
+};
+
+struct HasTwoFieldsWithVirtualMethod {
+  bool b;
+  bool c;
+  virtual ~HasTwoFieldsWithVirtualMethod() = default;
+};
+
+struct InheritFromStructsWithVirtualMethod: HasOneFieldWithVirtualMethod, HasTwoFieldsWithVirtualMethod {
+  int d;
+  virtual ~InheritFromStructsWithVirtualMethod() = default;
+};

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

@@ -111,4 +111,10 @@ FieldsTestSuite.test("Get field without copying base in the getter accessor") {
   expectEqual(copyCounter, getCopyCounter().pointee - expectedCopyCountDiff)
 }
 
+FieldsTestSuite.test("Structs with virtual methods") {
+  var derived = InheritFromStructsWithVirtualMethod()
+  derived.d = 42
+  expectEqual(derived.d, 42)
+}
+
 runAllTests()