[cxx-interop] Fix a crash with [[no_unique_address]]

[Xazax-hun]

Swift does not support storing fields in the padding of the previous fields just yet, so let's not import fields like that from C++. Represent them as opaque blobs instead.

Fixes #80764

rdar://149072458

[Xazax-hun]

@swift-ci please smoke test

[j-hui]

I'm wondering whether skipping this field is really the right approach, and whether there are other alternatives. Can you say more about why these overlapping fields are problematic for Swift at the moment?

[Xazax-hun]

I'm wondering whether skipping this field is really the right approach, and whether there are other alternatives. Can you say more about why these overlapping fields are problematic for Swift at the moment?

Absolutely, I would love to support this and that would be the right approach long term. We use FixedTypeInfo to represent the fields, and these classes and all the code using them have the underlying assumption that the sizeof(field) == sizeof(type). This invariant breaks with fields where the padding can be reused. So a significant portion of the code needs to be updated and tested for this to work. Should be doable, but it would be a significantly bigger change than this PR.

[rjmccall]

I would really prefer to just fix the IRGen bug here instead of declining to import a field just because it has this attribute on it. We should try to get Swift out of the business of making assumptions about C/C++ layout, which we should be able to do in three ways:

• We can completely get out of the business of constructing an LLVM struct type for the C struct by just using Clang's convertTypeForMemory API.
• We can build the FieldInfos by walking the Swift stored properties and just using Clang's getLLVMFieldNumber API to get the field number within the above.
• We can construct the spare bit vector by starting with all bits set and then clearing bits as appropriate for individual subobjects. We might need some new methods on ClusteredBitVector to support that kind of mutation, but that should be straightforward because APInt should directly support the operations we'd need.

[Xazax-hun]

I would really prefer to just fix the IRGen bug here

Absolutely! I think that is the correct thing to do here. Unfortunately, I believe it is a bit more involved so I'd like to land this workaround until we can get to the proper fix.

declining to import a field just because it has this attribute on it

Actually, we still can import zero sized fields or fields with this attribute that do not have their tail padding reused to other fields. Currently, it looks like it is not that easy to trigger this kind of layout optimization in Clang, so this might not be too restrictive for a short term workaround.

We should try to get Swift out of the business of making assumptions about C/C++ layout

I agree, but I think fixing the LLVM struct type and the spare bit vector might not be sufficient. Specifically, we generate FixedTypeInfo for the field types and that has the implicit assumption that sizeof(field) == sizeof(typeof(field)) but this is not true. My understanding is after using your proposed approach we would still look into trouble because we need to update the code paths that rely on getting the size from the FixedTypeInfo. At least, I attempted to do a quick fix that generated the correct LLVM struct type but I still run into some issues. I did not have time to go deeper with my investigations but wanted to address the crash until I have time to implement a better solution.

Would you be OK with this workaround temporarily?

[Xazax-hun]

@swift-ci please smoke test

[rjmccall]

Actually, we still can import zero sized fields or fields with this attribute that do not have their tail padding reused to other fields. Currently, it looks like it is not that easy to trigger this kind of layout optimization in Clang, so this might not be too restrictive for a short term workaround.

Yes, if you can change this to only apply to fields that actually have their tail padding used for something, that seems much less likely to have unwanted impact in the short term.

I agree, but I think fixing the LLVM struct type and the spare bit vector might not be sufficient. Specifically, we generate FixedTypeInfo for the field types and that has the implicit assumption that sizeof(field) == sizeof(typeof(field)) but this is not true.

Hmm. Yes, that's a much nastier problem; we need to make sure we're not generating code that writes beyond dsize for these types. I see why you want to simply not import them, then.

[Xazax-hun]

only apply to fields that actually have their tail padding used

I attempt to do something like that in this PR, I have fieldLayout.getDataSize() != fieldLayout.getSize() as a condition. That being said, I am not checking whether the following field is actually in the tail padding. Do you think that would be necessary?

[rjmccall]

only apply to fields that actually have their tail padding used

I attempt to do something like that in this PR, I have fieldLayout.getDataSize() != fieldLayout.getSize() as a condition. That being said, I am not checking whether the following field is actually in the tail padding. Do you think that would be necessary?

As it is, you're going to refuse to import in every field of non-POD type with tail padding. Well, hmm, except that it's only when they have the attribute; maybe that makes it okay in practice because people rarely use this attribute. Still, it would certainly be more conservative to only do this when it actually matters for layout.

[Xazax-hun]

Makes sense. I update the PR to check the offset of the next field (if there is one).

[Xazax-hun]

@swift-ci please smoke test

[j-hui]

Looks good to me, thanks for fleshing out the test!