From bdacdd047c7f5327469aa51b980099ce2f6fdaff Mon Sep 17 00:00:00 2001 From: susmonteiro Date: Fri, 25 Apr 2025 16:42:46 +0100 Subject: [PATCH] [cxx-interop] Fix metadata mismatch regarding fields of structs --- lib/IRGen/Field.h | 10 ++ lib/IRGen/GenMeta.cpp | 2 +- lib/IRGen/GenReflection.cpp | 30 +----- lib/IRGen/StructLayout.cpp | 23 +++++ lib/IRGen/StructMetadataVisitor.h | 4 +- .../Interop/Cxx/class/Inputs/module.modulemap | 6 ++ .../Interop/Cxx/class/Inputs/simple-structs.h | 53 +++++++++++ test/Interop/Cxx/class/print.swift | 44 +++++++++ .../Cxx/stdlib/Inputs/module.modulemap | 6 ++ test/Interop/Cxx/stdlib/Inputs/stdlib-types.h | 24 +++++ .../Cxx/stdlib/print-stdlib-types.swift | 91 +++++++++++++++++++ 11 files changed, 265 insertions(+), 28 deletions(-) create mode 100644 test/Interop/Cxx/class/Inputs/simple-structs.h create mode 100644 test/Interop/Cxx/class/print.swift create mode 100644 test/Interop/Cxx/stdlib/Inputs/stdlib-types.h create mode 100644 test/Interop/Cxx/stdlib/print-stdlib-types.swift diff --git a/lib/IRGen/Field.h b/lib/IRGen/Field.h index 0b6eba19a2a53..e2b61f4614566 100644 --- a/lib/IRGen/Field.h +++ b/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` 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 fn); +unsigned countExportableFields(IRGenModule &IGM, const NominalTypeDecl *type); + } // end namespace irgen } // end namespace swift diff --git a/lib/IRGen/GenMeta.cpp b/lib/IRGen/GenMeta.cpp index 62facbd61f240..635cc8d3f2020 100644 --- a/lib/IRGen/GenMeta.cpp +++ b/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()); diff --git a/lib/IRGen/GenReflection.cpp b/lib/IRGen/GenReflection.cpp index 50b71a54e2ab0..ff46ed1681410 100644 --- a/lib/IRGen/GenReflection.cpp +++ b/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` 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); - } }); } diff --git a/lib/IRGen/StructLayout.cpp b/lib/IRGen/StructLayout.cpp index 96759e1be4631..e75be0fb429e6 100644 --- a/lib/IRGen/StructLayout.cpp +++ b/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 fn) { auto classDecl = dyn_cast(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: diff --git a/lib/IRGen/StructMetadataVisitor.h b/lib/IRGen/StructMetadataVisitor.h index 4e2aff2f100ae..2c942adbb9679 100644 --- a/lib/IRGen/StructMetadataVisitor.h +++ b/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 StructMetadataVisitor // Struct field offsets. asImpl().noteStartOfFieldOffsets(); for (VarDecl *prop : Target->getStoredProperties()) - asImpl().addFieldOffset(prop); + if (!isPrivateField(prop)) + asImpl().addFieldOffset(prop); asImpl().noteEndOfFieldOffsets(); diff --git a/test/Interop/Cxx/class/Inputs/module.modulemap b/test/Interop/Cxx/class/Inputs/module.modulemap index fca54e3ce2814..46b4c801295b3 100644 --- a/test/Interop/Cxx/class/Inputs/module.modulemap +++ b/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 * +} diff --git a/test/Interop/Cxx/class/Inputs/simple-structs.h b/test/Interop/Cxx/class/Inputs/simple-structs.h new file mode 100644 index 0000000000000..084436b901f62 --- /dev/null +++ b/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 + +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 diff --git a/test/Interop/Cxx/class/print.swift b/test/Interop/Cxx/class/print.swift new file mode 100644 index 0000000000000..306ab9f745218 --- /dev/null +++ b/test/Interop/Cxx/class/print.swift @@ -0,0 +1,44 @@ +// 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 --check-prefix=CHECK --check-prefix=CHECK-%target-os + +// 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) + +printCxxStructPublicFields() +// CHECK-macosx: HasPublicFieldsOnly(pubInt: 42, pubStr: __C.std.std::__1.basic_string, std.__1.allocator>()) +// CHECK-linux-gnu: HasPublicFieldsOnly(pubInt: 42, pubStr: __C.std.std::__cxx11.basic_string, std.allocator>()) +// CHECK-windows-msvc: HasPublicFieldsOnly(pubInt: 42, pubStr: __C.std.basic_string, std.allocator>()) + +printCxxStructPrivateAndPublicFields() +// CHECK-macosx: HasPrivateAndPublicFields(pubInt: 42, pubStr: __C.std.std::__1.basic_string, std.__1.allocator>()) +// CHECK-linux-gnu: HasPrivateAndPublicFields(pubInt: 42, pubStr: __C.std.std::__cxx11.basic_string, std.allocator>()) +// CHECK-windows-msvc: HasPrivateAndPublicFields(pubInt: 42, pubStr: __C.std.basic_string, std.allocator>()) diff --git a/test/Interop/Cxx/stdlib/Inputs/module.modulemap b/test/Interop/Cxx/stdlib/Inputs/module.modulemap index 57f7eae3bb670..8edbde8df1f10 100644 --- a/test/Interop/Cxx/stdlib/Inputs/module.modulemap +++ b/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 diff --git a/test/Interop/Cxx/stdlib/Inputs/stdlib-types.h b/test/Interop/Cxx/stdlib/Inputs/stdlib-types.h new file mode 100644 index 0000000000000..b6c2099d9d493 --- /dev/null +++ b/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 +#include +#include + +std::unique_ptr makeInt() { return std::make_unique(42); } + +std::unique_ptr makeString() { + return std::make_unique("Unique string"); +} + +std::shared_ptr makeIntShared() { return std::make_unique(42); } + +std::shared_ptr makeStringShared() { + return std::make_unique("Shared string"); +} + +std::vector makeVecOfInt() { return {1, 2, 3}; } + +std::vector makeVecOfString() { return {"Hello", "World"}; } + +#endif diff --git a/test/Interop/Cxx/stdlib/print-stdlib-types.swift b/test/Interop/Cxx/stdlib/print-stdlib-types.swift new file mode 100644 index 0000000000000..d8b02ea2d0355 --- /dev/null +++ b/test/Interop/Cxx/stdlib/print-stdlib-types.swift @@ -0,0 +1,91 @@ +// 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 --check-prefix=CHECK --check-prefix=CHECK-%target-os + +// REQUIRES: executable_test + +import StdlibTypes + +func printStdString(s: std.string) { + print(s) + let swiftString = String(s) + print(swiftString) +} + +#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) +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) +} +#endif + +func printStdVectorOfInt() { + let vecOfInt = makeVecOfInt() + print(vecOfInt[0]) + print(vecOfInt) +} + +func printStdVectorOfString() { + let vecOfString = makeVecOfString() + print(vecOfString[0]) + print(vecOfString) +} + +printStdString(s: "Hello") +// CHECK-macosx: basic_string, std.__1.allocator>() +// CHECK-linux-gnu: basic_string, std.allocator>() +// CHECK-windows-msvc: basic_string, std.allocator>() +// CHECK: Hello + +#if !os(Windows) +printStdUniquePtrOfInt() +// CHECK-macosx: 42 +// CHECK-linux-gnu: 42 +printStdUniquePtrOfString() +// CHECK-macosx: basic_string, std.__1.allocator>() +// CHECK-linux-gnu: basic_string, std.allocator>() +printStdSharedPtrOfInt() +// CHECK-macosx: 42 +// CHECK-macosx: shared_ptr() +// CHECK-linux-gnu: 42 +// CHECK-linux-gnu: shared_ptr() +printStdSharedPtrOfString() +// CHECK-macosx: basic_string, std.__1.allocator>() +// CHECK-macosx: shared_ptr, std.__1.allocator>>() +// CHECK-linux-gnu: basic_string, std.allocator>() +// CHECK-linux-gnu: shared_ptr, std.allocator>>() + +#endif + +printStdVectorOfInt() +// CHECK: 1 +// CHECK-macosx: vector>() +// CHECK-linux-gnu: vector>() +// CHECK-windows-msvc: vector>() +printStdVectorOfString() +// CHECK-macosx: basic_string, std.__1.allocator>() +// CHECK-macosx: vector, std.__1.allocator>, std.__1.allocator, std.__1.allocator>>>() +// CHECK-linux-gnu: basic_string, std.allocator>() +// CHECK-linux-gnu: vector, std.allocator>, std.allocator, std.allocator>>>() +// CHECK-windows-msvc: basic_string, std.allocator>() +// CHECK-windows-msvc: vector, std.allocator>, std.allocator, std.allocator>>>() +