[cxx-interop] [IRGen] TypeInfo for address-only types. #32973
Conversation
lib/IRGen/GenStruct.cpp
Outdated
There was a problem hiding this comment.
Can someone more familiar with this part of IRGen confirm that FixedTypeInfo is what should go here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I can make a follow up PR for this.
zoecarver
force-pushed
the
cxx/address-only-type-info
branch
3 times, most recently
from
2f3b266 to
798f0f9
Compare
lib/IRGen/GenStruct.cpp
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, you're right! With a small amount of refactoring, I was able to get this working without begin/end.
There was a problem hiding this comment.
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.
zoecarver
force-pushed
the
cxx/address-only-type-info
branch
from
798f0f9 to
0fcd3e9
Compare
zoecarver
force-pushed
the
cxx/address-only-type-info
branch
2 times, most recently
from
0548e7c to
d921b11
Compare
|
@swift-ci please smoke test |
There was a problem hiding this comment.
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.
test/Interop/Cxx/class/type-classification-non-trivial-silgen.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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
force-pushed
the
cxx/address-only-type-info
branch
2 times, most recently
from
49482b0 to
064d9ff
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
zoecarver
force-pushed
the
cxx/address-only-type-info
branch
from
064d9ff to
6b55bd7
Compare
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
@swift-ci please test Windows Platform |
|
Build failed |
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.
|
@swift-ci please test |
zoecarver
force-pushed
the
cxx/address-only-type-info
branch
from
6b55bd7 to
0cef2ce
Compare
|
@swift-ci please test |
|
@zoecarver Congrats! |
|
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/ |
|
@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. |
|
See #33474. |
4 participants
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.