[cxx-interop] [IRGen] TypeInfo for address-only types. #32973

zoecarver · 2020-07-18T03:42:30Z

The current "ClangRecordTypeInfo" derives from "LoadableTypeInfo" and is only meant for loadable types. While we have not yet run into problems, this may cause issues in the future and as more logic is needed around copying, moving, and destroying C++ objects, this should be fixed.

I'm not aware of any behavior changes so, it's a bit difficult for me to come up with tests. But, that may just be a consequence of my lack of knowledge around this part of the codebase. Please chime in with test ideas if you have any :)

Refs #32291.

lib/IRGen/GenStruct.cpp

zoecarver · 2020-07-18T03:44:10Z

lib/IRGen/GenStruct.cpp

-
+
+  class ClangRecordAddressOnlyTypeInfo final
+      : public StructTypeInfoBase<ClangRecordAddressOnlyTypeInfo, FixedTypeInfo,


Can someone more familiar with this part of IRGen confirm that FixedTypeInfo is what should go here?

Should we use IndirectTypeInfo as the base type? Depending on the capabilities of the Clang type, we might need to use ImmovableTypeInfo (or imitate it appropriately). I'd suggest to try using IndirectTypeInfo in this PR, while leaving the immovable issue as a TODO for future.

You mean use IndirectTypeInfo instead of StructTypeInfoBase? Even address-only types should be imported as structs, so I think we should keep StructTypeInfoBase. IndirectTypeInfo takes two template parameters so, I think one of those would still have to be FixedTypeInfo or something.

lib/IRGen/GenStruct.cpp

gribozavr · 2020-07-22T17:43:41Z

lib/IRGen/GenStruct.cpp

+    if (SwiftType.getStructOrBoundGenericStruct()
+            ->isCxxNotTriviallyCopyable()) {
+      return ClangRecordAddressOnlyTypeInfo::create(
+          FieldInfos, llvmType, TotalStride, std::move(SpareBits),


The FieldInfos vector will contain private data members that we're not importing. It would be good to skip private data members in ClangRecordLowering (in a future PR).

Currently for skipping non-importable fields ClangRecordLowering has addOpaqueField(), which should form the correct layout but avoid informing the rest of the system about the specific type of fields that we can't ever access. However, it uses IRGenModule::getOpaqueStorageTypeInfo which returns LoadableTypeInfo which is not correct for some fields.

I can make a follow up PR for this.

lib/IRGen/GenStruct.cpp

gribozavr · 2020-07-27T12:28:40Z

lib/IRGen/GenStruct.cpp

-  void addFieldInfo(VarDecl *swiftField, const LoadableTypeInfo &fieldType) {
-    unsigned explosionSize = fieldType.getExplosionSize();
+  void addFieldInfo(VarDecl *swiftField, const FixedTypeInfo &fieldType) {
+    unsigned explosionSize = SwiftDecl->getStoredProperties().size();


If I understand it correctly, the concept of explosion does not make sense for address-only types (explosion is a list of LLVM values into which an aggregate value was loaded), so it is not a surprise that we don't have an appropriate API to call here to calculate the size of the explosion. Just counting the number of stored properties is not correct, since we decided to not import private member variables in C++ classes.

This information about explosion size is eventually written into RecordField::Begin / ::End. Note that only ClangFieldInfo is setting these fields. Even StructFieldInfo (the subclass for native Swift structs) is not setting them. Since this data is not correct for native Swift structs, I suspect that users of this data are dead code. Could you take a look? It is quite possible you would be able to delete some/all of the users, as well as this code that calculates this information.

If this data is used, I think we should change ClangFieldInfo to be like StructFieldInfo and not set the explosion begin/end, at least when we have an address-only type.

Yeah, you're right! With a small amount of refactoring, I was able to get this working without begin/end.

Turns out after running all the tests there are some things which rely on begin/end. At first, I was confused by this because, as you said, it doesn't appear that StructFieldInfo sets them. But, after further investigation, it appears they are set for StructFieldInfo. Just in a more convoluted way.

I'll have to add back the old path, too.

test/Interop/Cxx/class/Inputs/type-classification.h

test/Interop/Cxx/class/address-only.swift

gribozavr

Thanks for the fixes!

lib/IRGen/GenRecord.h

lib/IRGen/GenStruct.cpp

zoecarver · 2020-08-01T05:11:33Z

@swift-ci please smoke test

zoecarver · 2020-08-01T05:12:34Z

test/Interop/Cxx/class/type-classification-non-trivial-irgen.swift

+
+import TypeClassification
+
+// TODO: C++ objects with destructors should be tested here once we fully


This is unfortunate (but also unrelated to this change). I was meaning to submit a fix for this and forgot. Maybe I'll have time this weekend. Otherwise, I'll do it next week.

See #32591.

test/Interop/Cxx/class/type-classification-non-trivial-silgen.swift

gribozavr · 2020-08-07T14:36:22Z

test/Interop/Cxx/class/type-classification-non-trivial-irgen.swift

+// TODO: C++ objects with destructors should be tested here once we fully
+// support them.
+
+// CHECK-LABEL: define {{.*}}i1 @"$s4main37testStructWithCopyConstructorAndValueSbyF"


Could you add an explanation of what these tests are verifying, so that future when IRGen patterns change people can update the CHECK lines appropriately, and not accidentally eliminate the part that we're thinking we are testing here.

zoecarver · 2020-08-08T22:13:58Z

@swift-ci please test

swift-ci · 2020-08-08T22:16:56Z

Build failed
Swift Test Linux Platform
Git Sha - d921b11b14fac039a0a55a002f71c155c71feef8

swift-ci · 2020-08-08T22:17:04Z

Build failed
Swift Test OS X Platform
Git Sha - d921b11b14fac039a0a55a002f71c155c71feef8

zoecarver · 2020-08-09T17:05:09Z

@swift-ci please test

zoecarver · 2020-08-09T17:05:20Z

@swift-ci please test

zoecarver · 2020-08-09T17:18:24Z

@swift-ci please test Windows Platform

swift-ci · 2020-08-09T18:23:50Z

Build failed
Swift Test Linux Platform
Git Sha - 6b55bd765901d6fa1d68aba7fc131817aff7c18b

The current "ClangRecordTypeInfo" derives from "LoadableTypeInfo" and is only meant for loadable types. While we have not yet run into problems, this may cause issues in the future and as more logic is needed around copying, moving, and destroying C++ objects, this needs to be fixed.

zoecarver · 2020-08-10T04:08:52Z

@swift-ci please test

zoecarver · 2020-08-10T04:10:15Z

@swift-ci please test

gribozavr · 2020-08-10T17:45:30Z

@zoecarver Congrats!

meg-gupta · 2020-08-14T00:06:23Z

This is causing a Filecheck mismatch error with Interop/Cxx/class/type-classification-non-trivial-irgen.swift in https://ci.swift.org/job/oss-swift_tools-R_stdlib-RD_test-simulator/5307/
@zoecarver can you take a look ?

zoecarver · 2020-08-14T00:17:07Z

@meg-gupta thanks for bringing my attention to the issue. I have a fix, running the tests locally now. I'll make a PR shortly.

zoecarver · 2020-08-14T00:37:17Z

See #33474.

zoecarver requested a review from gribozavr July 18, 2020 03:42

zoecarver commented Jul 18, 2020

View reviewed changes