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

Author: susmonteiro

lib/IRGen/Field.h

@@ -96,12 +96,22 @@ struct Field {
   bool operator!=(Field other) const { return declOrKind != other.declOrKind; }
 };
 
+bool isPrivateField(VarDecl *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.
+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

@@ -1845,7 +1845,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,18 @@ unsigned irgen::getNumFields(const NominalTypeDecl *target) {
   return numFields;
 }
 
+bool irgen::isPrivateField(VarDecl *decl) {
+  return (decl->getClangDecl() &&
+          decl->getFormalAccess() == AccessLevel::Private);
+}
+
+bool irgen::isExportableField(Field field) {
+  if (field.getKind() == Field::Kind::Var && isPrivateField(field.getVarDecl()))
+    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 +622,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
+  int 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

@@ -17,6 +17,7 @@
 #ifndef SWIFT_IRGEN_STRUCTMETADATALAYOUT_H
 #define SWIFT_IRGEN_STRUCTMETADATALAYOUT_H
 
+#include "Field.h"
 #include "NominalMetadataVisitor.h"
 #include "swift/AST/IRGenOptions.h"
 
@@ -64,7 +65,8 @@ template <class Impl> class StructMetadataVisitor
     // Struct field offsets.
     asImpl().noteStartOfFieldOffsets();
     for (VarDecl *prop : Target->getStoredProperties())
-      asImpl().addFieldOffset(prop);
+      if (!isPrivateField(prop))
+        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,53 @@
+#ifndef TEST_INTEROP_CXX_CLASS_INPUTS_SIMPLE_STRUCTS_H
+#define TEST_INTEROP_CXX_CLASS_INPUTS_SIMPLE_STRUCTS_H
+
+#include <string>
+
+struct HasPrivateFieldsOnly {
+private:
+  int privInt;
+  std::string privStr;
+
+public:
+  HasPrivateFieldsOnly(int i, std::string s) : privInt(i), privStr(s) {}
+};
+
+struct HasPublicFieldsOnly {
+  int pubInt;
+  std::string pubStr;
+
+  HasPublicFieldsOnly(int i, std::string s) : pubInt(i), pubStr(s) {}
+};
+
+struct HasIntFieldsOnly {
+private:
+  int a = 1;
+
+public:
+  int b = 2;
+
+private:
+  int c = 3;
+
+public:
+  int d = 4;
+};
+
+struct HasPrivateAndPublicFields {
+private:
+  std::string privStr;
+
+public:
+  int pubInt;
+
+private:
+  int privInt;
+
+public:
+  std::string pubStr;
+
+  HasPrivateAndPublicFields(int i1, int i2, std::string s1, std::string s2)
+      : privStr(s2), pubInt(i2), privInt(i1), pubStr(s1) {}
+};
+
+#endif

test/Interop/Cxx/class/print.swift

@@ -0,0 +1,43 @@
+// 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(42, "Hello")
+    print(s)
+}
+
+func printCxxStructPublicFields() {
+    let s = HasPublicFieldsOnly(42, "Hello")
+    print(s)
+}
+
+func printCxxStructIntFields() {
+    let s = HasIntFieldsOnly()
+    print(s)
+}
+
+func printCxxStructPrivateAndPublicFields() {
+    let s = HasPrivateAndPublicFields(24, 42, "Hello", "World")
+    print(s)
+}
+
+printCxxStructPrivateFields() 
+// CHECK: HasPrivateFieldsOnly()
+
+printCxxStructIntFields()
+// CHECK: HasIntFieldsOnly(b: 2, d: 4)
+
+#if os(macOS)
+// std::string has a different representation in other OSes
+
+printCxxStructPublicFields()
+// CHECK: HasPublicFieldsOnly(pubInt: 42, pubStr: __C.std.std::__1.basic_string<CChar, std.__1.char_traits<CChar>, std.__1.allocator<CChar>>())
+printCxxStructPrivateAndPublicFields()
+// CHECK: HasPrivateAndPublicFields(pubInt: 42, pubStr: __C.std.std::__1.basic_string<CChar, std.__1.char_traits<CChar>, std.__1.allocator<CChar>>())
+#endif

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

@@ -64,6 +64,12 @@ module MsvcUseVecIt {
   export *
 }
 
+module StdlibTypes {
+  header "stdlib-types.h"
+  requires cplusplus
+  export *
+}
+
 module StdUniquePtr {
   header "std-unique-ptr.h"
   requires cplusplus

test/Interop/Cxx/stdlib/Inputs/stdlib-types.h

@@ -0,0 +1,24 @@
+#ifndef TEST_INTEROP_CXX_STDLIB_INPUTS_STD_TYPES_H
+#define TEST_INTEROP_CXX_STDLIB_INPUTS_STD_TYPES_H
+
+#include <memory>
+#include <string>
+#include <vector>
+
+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::shared_ptr<int> makeIntShared() { return std::make_unique<int>(42); }
+
+std::shared_ptr<std::string> makeStringShared() {
+  return std::make_unique<std::string>("Shared string");
+}
+
+std::vector<int> makeVecOfInt() { return {1, 2, 3}; }
+
+std::vector<std::string> makeVecOfString() { return {"Hello", "World"}; }
+
+#endif

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

@@ -0,0 +1,72 @@
+// 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 StdlibTypes
+
+func printStdString(s: std.string) {
+    print(s)
+    let swiftString = String(s)
+    print(swiftString)
+}
+
+func printStdUniquePtrOfInt() {
+    let uint = makeInt()
+    print(uint.pointee)
+}
+
+func printStdSharedPtrOfInt() {
+    let sint = makeIntShared()
+    print(sint.pointee)
+    print(sint)
+}
+
+func printStdVectorOfInt() {
+    let vecOfInt = makeVecOfInt()
+    print(vecOfInt[0])
+    print(vecOfInt)
+}
+
+func printStdUniquePtrOfString() {
+    let ustring = makeString()
+    print(ustring.pointee)
+}
+
+func printStdSharedPtrOfString() {
+    let sstring = makeStringShared()
+    print(sstring.pointee)
+    print(sstring)
+}
+
+func printStdVectorOfString() {
+    let vecOfString = makeVecOfString()
+    print(vecOfString[0])
+    print(vecOfString)
+}
+
+#if !os(Windows)
+// FIXME: We can't import std::unique_ptr or std::shared_ptr properly on Windows (https://github.com/apple/swift/issues/70226)
+printStdUniquePtrOfInt()
+// CHECK: 42
+printStdSharedPtrOfInt()
+// CHECK: 42
+#endif
+printStdVectorOfInt()
+// CHECK: 1
+
+#if os(macOS)
+// std::string has a different representation in other OSes
+
+printStdString(s: "Hello")
+// CHECK: basic_string<CChar, std.__1.char_traits<CChar>, std.__1.allocator<CChar>>()
+// CHECK: Hello
+printStdUniquePtrOfString()
+// CHECK: basic_string<CChar, std.__1.char_traits<CChar>, std.__1.allocator<CChar>>()
+printStdSharedPtrOfString()
+// CHECK: basic_string<CChar, std.__1.char_traits<CChar>, std.__1.allocator<CChar>>()
+printStdVectorOfString()
+// CHECK: basic_string<CChar, std.__1.char_traits<CChar>, std.__1.allocator<CChar>>()
+#endif