[cxx-interop] Do not codegen zero-sized fields

[Xazax-hun]

Zero sized fields are messing up the offset calculations when we import C++ fields to Swift. We assume that the size of the field is determined by the type of the field. This is not true for fields marked with no_unique_address. Those fields can have 0 size while the sizeof(decltype(field)) is still 1.

rdar://143907490

[Xazax-hun]

@swift-ci please smoke test

[j-hui]

I think the actual code change itself seems fine as is, but I think it might be a good idea to add a bit more testing coverage since this may be prone to some subtle runtime bugs.

In addition to what I've suggested, I'm wondering if it would be worth introducing some operations that might be sensitive to the size and layout of HasZeroSizedField, e.g.,

var s = HasZeroSizedField()

let t = s    // copy construction/assignment

cppFunc(s)   // pass to C++ function
swiftFunc(s) // pass to a Swift function

// getting and setting fields, which you're already testing

Finally, just to double-check: since this change is only in IRGen, it shouldn't affect anything in SILGen or before that, right? I'm wondering if it's worth adding tests to check the module interface, or to look at the SILGen/IRGen output.

[rjmccall]

We really shouldn't be doing any "offset calculations" here at all — the only correct information comes from Clang. And I wouldn't expect that we'd need to suppress just adding an entry for the field to the TypeInfo. I can imagine that we'd need to specifically suppress adding a field to the LLVM IR type, if we're duplicating that logic rather than just using Clang.

[Xazax-hun]

@swift-ci please smoke test

[Xazax-hun]

I'm wondering if it would be worth introducing some operations that might be sensitive to the size and layout

Sure! I added more tests!

it shouldn't affect anything in SILGen or before that, right?

Yup!

adding tests to check the module interface, or to look at the SILGen/IRGen output.

I am a bit hesitant to do that. Checking the whole SIL would be really fragile without pointing out the source of the problem. I could check if the zero sized field is present but the accessibility of the zero sized field is already covered in the latest iteration of tests.

We really shouldn't be doing any "offset calculations" here at all — the only correct information comes from Clang.

We do use the offset information from Clang, but we also add padding on the Swift side manually to make sure the layout is the same and we use the Clang offsets to calculate these paddings. Unfortunately, this does not work for some zero sized types because Clang just returns zero as their offset (even if they are after some non-zero fields). Other reason to do offset calculations is how we handle inheritance. We don't have inheritance in Swift structs so we end up adding the bases' fields to the current type.

And I wouldn't expect that we'd need to suppress just adding an entry for the field to the TypeInfo.

I revised the patch to do what you suggest, keep adding the field to TypeInfo but avoid adding it to the LLVM IR type which matches what we do for other zero sized types like tail allocated C types.

[j-hui]

Looks good to me, thanks for addressing my comments!

[Michael137]

Don't have a lot of context about the interop side of things, but just wanted to point out that isZeroSized does not imply empty type (i.e., [[no_unique_address]] on non-empty fields allows Clang to fold other fields into the tail padding of that non-empty NUA field).

[Michael137]

llvm/llvm-project#50766

[Michael137]

Ah looks like Clang still doesn't currently make use of that (but it's a thing that could start happening, llvm/llvm-project#90462)

[Xazax-hun]

Thanks, that is a good point. Once Clang starts to reuse the tail padding we definitely should have some tests for that. I do remember us running into some trouble when a base class' tail padding was reused, Egor did some fixes in that space.

Here, isZeroSized comes from FieldDecl::isZeroSize and it is documented as "Determine if this field is a subobject of zero size, that is, either a zero-length bit-field or a field of empty class type with the [[no_unique_address]] attribute.". I think based on this, it should be false for non-empty fields.

[Michael137]

One quirk is for example:

struct E1 {};
struct E2 { [[no_unique_address]] E1 e; };
struct Foo { [[no_unique_address]] E2 e; };

where isZeroSize on Foo::e would also be true (I think). Just brought it up in case it brings some edge-cases to mind :)

Once Clang starts to reuse the tail padding we definitely should have some tests for that.

Sounds good! 👍

[Xazax-hun]

Ah, I see! Thanks a lot for the explanation! Now I see what you mean. I think this code path should be fine but this definitely worth having a test.