[cxx-interop] Fix metadata mismatch regarding fields of structs

Author: susmonteiro

lib/IRGen/Field.h

@@ -96,12 +96,20 @@ struct Field {
   bool operator!=(Field other) const { return declOrKind != other.declOrKind; }
 };
 
+// Don't export private C++ fields that were imported as private Swift fields.
+// The type of a private field might not have all the type witness operations
+// that Swift requires, for instance, `std::unique_ptr<IncompleteType>` would
+// not have a destructor.
+bool isExportableField(Field field);
+
 /// Iterate all the fields of the given struct or class type, including
 /// any implicit fields that might be accounted for in
 /// getFieldVectorLength.
 void forEachField(IRGenModule &IGM, const NominalTypeDecl *typeDecl,
                   llvm::function_ref<void(Field field)> fn);
 
+unsigned countExportableFields(IRGenModule &IGM, const NominalTypeDecl *type);
+
 } // end namespace irgen
 } // end namespace swift
 

lib/IRGen/GenMeta.cpp

@@ -1840,7 +1840,7 @@ namespace {
 
     void addLayoutInfo() {
       // uint32_t NumFields;
-      B.addInt32(getNumFields(getType()));
+      B.addInt32(countExportableFields(IGM, getType()));
 
       // uint32_t FieldOffsetVectorOffset;
       B.addInt32(FieldVectorOffset / IGM.getPointerSize());

lib/IRGen/GenReflection.cpp

@@ -946,35 +946,13 @@ class FieldTypeMetadataBuilder : public ReflectionMetadataBuilder {
     B.addInt16(uint16_t(kind));
     B.addInt16(FieldRecordSize);
 
-    // Filter to select which fields we'll export FieldDescriptors for.
-    auto exportable_field =
-      [](Field field) {
-        // Don't export private C++ fields that were imported as private Swift fields.
-        // The type of a private field might not have all the type witness
-        // operations that Swift requires, for instance,
-        // `std::unique_ptr<IncompleteType>` would not have a destructor.
-        if (field.getKind() == Field::Kind::Var &&
-            field.getVarDecl()->getClangDecl() &&
-            field.getVarDecl()->getFormalAccess() == AccessLevel::Private)
-          return false;
-        // All other fields are exportable
-        return true;
-      };
-
-    // Count exportable fields
-    int exportableFieldCount = 0;
-    forEachField(IGM, NTD, [&](Field field) {
-      if (exportable_field(field)) {
-        ++exportableFieldCount;
-      }
-    });
-
     // Emit exportable fields, prefixed with a count
-    B.addInt32(exportableFieldCount);
+    B.addInt32(countExportableFields(IGM, NTD));
+
+    // Filter to select which fields we'll export FieldDescriptor for.
     forEachField(IGM, NTD, [&](Field field) {
-      if (exportable_field(field)) {
+      if (isExportableField(field))
         addField(field);
-      }
     });
   }
 

lib/IRGen/StructLayout.cpp

@@ -589,6 +589,15 @@ unsigned irgen::getNumFields(const NominalTypeDecl *target) {
   return numFields;
 }
 
+bool irgen::isExportableField(Field field) {
+  if (field.getKind() == Field::Kind::Var &&
+      field.getVarDecl()->getClangDecl() &&
+      field.getVarDecl()->getFormalAccess() == AccessLevel::Private)
+    return false;
+  // All other fields are exportable
+  return true;
+}
+
 void irgen::forEachField(IRGenModule &IGM, const NominalTypeDecl *typeDecl,
                          llvm::function_ref<void(Field field)> fn) {
   auto classDecl = dyn_cast<ClassDecl>(typeDecl);
@@ -610,6 +619,17 @@ void irgen::forEachField(IRGenModule &IGM, const NominalTypeDecl *typeDecl,
   }
 }
 
+unsigned irgen::countExportableFields(IRGenModule &IGM,
+                                      const NominalTypeDecl *type) {
+  // Don't count private C++ fields that were imported as private Swift fields
+  unsigned exportableFieldCount = 0;
+  forEachField(IGM, type, [&](Field field) {
+    if (isExportableField(field))
+      ++exportableFieldCount;
+  });
+  return exportableFieldCount;
+}
+
 SILType Field::getType(IRGenModule &IGM, SILType baseType) const {
   switch (getKind()) {
   case Field::Var:

lib/IRGen/StructMetadataVisitor.h

@@ -63,8 +63,11 @@ template <class Impl> class StructMetadataVisitor
 
     // Struct field offsets.
     asImpl().noteStartOfFieldOffsets();
-    for (VarDecl *prop : Target->getStoredProperties())
-      asImpl().addFieldOffset(prop);
+    for (VarDecl *prop : Target->getStoredProperties()) {
+      if (!(prop->getClangDecl() &&
+            prop->getFormalAccess() == AccessLevel::Private))
+        asImpl().addFieldOffset(prop);
+    }
 
     asImpl().noteEndOfFieldOffsets();
 

test/Interop/Cxx/class/Inputs/module.modulemap

@@ -152,3 +152,9 @@ module PIMPL {
   requires cplusplus
   export *
 }
+
+module SimpleStructs {
+  header "simple-structs.h"
+  requires cplusplus
+  export *
+}

test/Interop/Cxx/class/Inputs/simple-structs.h

@@ -0,0 +1,57 @@
+#ifndef TEST_INTEROP_CXX_CLASS_INPUTS_SIMPLE_STRUCTS_H
+#define TEST_INTEROP_CXX_CLASS_INPUTS_SIMPLE_STRUCTS_H
+
+#include <string>
+
+struct HasPrivateFieldsOnly {
+private:
+  int priv1;
+  int priv2;
+
+public:
+  HasPrivateFieldsOnly(int i1, int i2) : priv1(i1), priv2(i2) {}
+};
+
+struct HasPublicFieldsOnly {
+  int publ1;
+  int publ2;
+
+  HasPublicFieldsOnly(int i1, int i2) : publ1(i1), publ2(i2) {}
+};
+
+struct HasPrivatePublicProtectedFields {
+private:
+  int priv1;
+
+public:
+  int publ1;
+
+protected:
+  int prot1;
+
+protected:
+  int prot2;
+
+private:
+  int priv2;
+
+public:
+  int publ2;
+
+  HasPrivatePublicProtectedFields(int i1, int i2, int i3, int i4, int i5,
+                                  int i6)
+      : priv1(i1), publ1(i2), prot1(i3), prot2(i4), priv2(i5),
+        publ2(i6) {}
+};
+
+struct Outer {
+private:
+  HasPrivatePublicProtectedFields privStruct;
+
+public:
+  HasPrivatePublicProtectedFields publStruct;
+
+  Outer() : privStruct(1, 2, 3, 4, 5, 6), publStruct(7, 8, 9, 10, 11, 12) {}
+};
+
+#endif

test/Interop/Cxx/class/print-simple-structs.swift

@@ -0,0 +1,40 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-build-swift %s -o %t/a.out -Xfrontend -enable-experimental-cxx-interop -I %S/Inputs
+// RUN: %target-codesign %t/a.out
+// RUN: %target-run %t/a.out | %FileCheck %s
+
+// REQUIRES: executable_test
+
+import SimpleStructs
+
+func printCxxStructPrivateFields() {
+    let s = HasPrivateFieldsOnly(1, 2)
+    print(s)
+}
+
+func printCxxStructPublicFields() {
+    let s = HasPublicFieldsOnly(1, 2)
+    print(s)
+}
+
+func printCxxStructPrivatePublicProtectedFields() {
+    let s = HasPrivatePublicProtectedFields(1, 2, 3, 4, 5, 6)
+    print(s)
+}
+
+func printCxxStructNested() {
+    let s = Outer()
+    print(s)
+}
+
+printCxxStructPrivateFields() 
+// CHECK: HasPrivateFieldsOnly()
+
+printCxxStructPublicFields()
+// CHECK: HasPublicFieldsOnly(publ1: 1, publ2: 2)
+
+printCxxStructPrivatePublicProtectedFields()
+// CHECK: HasPrivatePublicProtectedFields(publ1: 2, publ2: 6)
+
+printCxxStructNested()
+// CHECK: Outer(publStruct: {{.*}}.HasPrivatePublicProtectedFields(publ1: 8, publ2: 12))

test/Interop/Cxx/stdlib/Inputs/std-string-and-vector.h

@@ -9,3 +9,28 @@ struct Item {
 inline Item get_item() {
   return {};
 }
+
+std::vector<int> makeVecOfInt() { return {1, 2, 3}; }
+std::vector<std::string> makeVecOfString() { return {"Hello", "World"}; }
+
+struct S {
+private:
+  std::string privStr;
+  std::vector<std::string> privVec;
+
+public:
+  std::string pubStr;
+  std::vector<std::string> pubVec;
+
+protected:
+  std::string protStr;
+  std::vector<std::string> protVec;
+
+public:
+  S() : privStr("private"), privVec({"private", "vector"}), 
+        pubStr("public"), pubVec({"a", "public", "vector"}), 
+        protStr("protected"), protVec({"protected", "vector"}) {}
+
+  std::vector<std::string> getPrivVec() const { return privVec; }
+  std::string getProtStr() const { return protStr; }
+};

test/Interop/Cxx/stdlib/Inputs/std-unique-ptr.h

@@ -2,6 +2,7 @@
 #define TEST_INTEROP_CXX_STDLIB_INPUTS_STD_UNIQUE_PTR_H
 
 #include <memory>
+#include <string>
 
 struct NonCopyable {
     NonCopyable(int x) : x(x) {}
@@ -29,6 +30,10 @@ std::unique_ptr<int> makeInt() {
   return std::make_unique<int>(42);
 }
 
+std::unique_ptr<std::string> makeString() {
+  return std::make_unique<std::string>("Unique string");
+}
+
 std::unique_ptr<int[]> makeArray() {
   int *array = new int[3];
   array[0] = 1;
@@ -55,4 +60,10 @@ std::unique_ptr<HasDtor> makeDtor() {
   return std::make_unique<HasDtor>();
 }
 
+std::shared_ptr<int> makeIntShared() { return std::make_unique<int>(42); }
+
+std::shared_ptr<std::string> makeStringShared() {
+  return std::make_unique<std::string>("Shared string");
+}
+
 #endif // TEST_INTEROP_CXX_STDLIB_INPUTS_STD_UNIQUE_PTR_H

test/Interop/Cxx/stdlib/print-stdlib-types.swift

@@ -0,0 +1,90 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-build-swift %s -o %t/a.out -Xfrontend -enable-experimental-cxx-interop -I %S/Inputs
+// RUN: %target-codesign %t/a.out
+// RUN: %target-run %t/a.out | %FileCheck %s
+
+// REQUIRES: executable_test
+// XFAIL: OS=windows-msvc
+// FIXME: We can't import std::unique_ptr or std::shared_ptr properly on Windows (https://github.com/apple/swift/issues/70226)
+
+import StdStringAndVector
+import StdUniquePtr
+
+func printStdString(s: std.string) {
+    print(s)
+    let swiftString = String(s)
+    print(swiftString)
+}
+
+func printStdUniquePtrOfInt() {
+    let uint = makeInt()
+    print(uint.pointee)
+}
+
+func printStdUniquePtrOfString() {
+    let ustring = makeString()
+    print(ustring.pointee)
+}
+
+func printStdSharedPtrOfInt() {
+    let sint = makeIntShared()
+    print(sint.pointee)
+    print(sint)
+}
+
+func printStdSharedPtrOfString() {
+    let sstring = makeStringShared()
+    print(sstring.pointee)
+    print(sstring)
+}
+
+func printStdVectorOfInt() {
+    let vecOfInt = makeVecOfInt()
+    print(vecOfInt[0])
+    print(vecOfInt)
+}
+
+func printStdVectorOfString() {
+    let vecOfString = makeVecOfString()
+    print(vecOfString[0])
+    print(vecOfString)
+}
+
+func printStruct() {
+    let s = S()
+    print(s.getPrivVec())
+    print(s.getProtStr())
+    print(s.pubStr)
+    print(s.pubVec)
+    print(s)
+}
+
+printStdString(s: "Hello")
+// CHECK: basic_string<CChar, {{.*}}, {{.*}}>()
+// CHECK: Hello
+
+printStdUniquePtrOfInt()
+// CHECK: 42
+printStdUniquePtrOfString()
+// CHECK: basic_string<CChar, {{.*}}, {{.*}}>()
+
+printStdSharedPtrOfInt()
+// CHECK: 42
+// CHECK: shared_ptr<CInt>()
+printStdSharedPtrOfString()
+// CHECK: basic_string<CChar, {{.*}}, {{.*}}>()
+// CHECK: shared_ptr<{{.*}}.basic_string<CChar, {{.*}}, {{.*}}>>
+
+printStdVectorOfInt()
+// CHECK: 1
+// CHECK: vector<CInt, {{.*}}>()
+printStdVectorOfString()
+// CHECK: basic_string<CChar, {{.*}}, {{.*}}>()
+// CHECK: vector<{{.*}}.basic_string<CChar, {{.*}}, {{.*}}>, {{.*}}>()
+
+printStruct()
+// CHECK: vector<{{.*}}.basic_string<CChar, {{.*}}, {{.*}}>, {{.*}}>()
+// CHECK: basic_string<CChar, {{.*}}, {{.*}}>()
+// CHECK: basic_string<CChar, {{.*}}, {{.*}}>()
+// CHECK: vector<{{.*}}.basic_string<CChar, {{.*}}, {{.*}}>, {{.*}}>()
+// CHECK: S(pubStr: {{.*}}.basic_string<CChar, {{.*}}, {{.*}}>(), pubVec: {{.*}}.vector<{{.*}}.basic_string<CChar, {{.*}}, {{.*}}>, {{.*}}>())