Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 3, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 18 additions & 13 deletions lib/IRGen/GenStruct.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1451,6 +1451,7 @@ class ClangRecordLowering {
/// Place the next struct field at its appropriate offset.
void addStructField(const clang::FieldDecl *clangField,
VarDecl *swiftField, const clang::ASTRecordLayout &layout) {
bool isZeroSized = clangField->isZeroSize(ClangContext);
unsigned fieldOffset = layout.getFieldOffset(clangField->getFieldIndex());
assert(!clangField->isBitField());
Size offset( SubobjectAdjustment.getValue() + fieldOffset / 8);
Expand All @@ -1460,12 +1461,12 @@ class ClangRecordLowering {
auto &fieldTI = cast<FixedTypeInfo>(IGM.getTypeInfo(
SwiftType.getFieldType(swiftField, IGM.getSILModule(),
IGM.getMaximalTypeExpansionContext())));
addField(swiftField, offset, fieldTI);
addField(swiftField, offset, fieldTI, isZeroSized);
return;
}

// Otherwise, add it as an opaque blob.
auto fieldSize = ClangContext.getTypeSizeInChars(clangField->getType());
auto fieldSize = isZeroSized ? clang::CharUnits::Zero() : ClangContext.getTypeSizeInChars(clangField->getType());
return addOpaqueField(offset, Size(fieldSize.getQuantity()));
}

Expand All @@ -1491,23 +1492,23 @@ class ClangRecordLowering {
if (fieldSize.isZero()) return;

auto &opaqueTI = IGM.getOpaqueStorageTypeInfo(fieldSize, Alignment(1));
addField(nullptr, offset, opaqueTI);
addField(nullptr, offset, opaqueTI, false);
}

/// Add storage for an (optional) Swift field at the given offset.
void addField(VarDecl *swiftField, Size offset,
const FixedTypeInfo &fieldType) {
assert(offset >= NextOffset && "adding fields out of order");
const FixedTypeInfo &fieldType, bool isZeroSized) {
assert(isZeroSized || offset >= NextOffset && "adding fields out of order");

// Add a padding field if required.
if (offset != NextOffset)
if (!isZeroSized && offset != NextOffset)
addPaddingField(offset);

addFieldInfo(swiftField, fieldType);
addFieldInfo(swiftField, fieldType, isZeroSized);
}

/// Add information to track a value field at the current offset.
void addFieldInfo(VarDecl *swiftField, const FixedTypeInfo &fieldType) {
void addFieldInfo(VarDecl *swiftField, const FixedTypeInfo &fieldType, bool isZeroSized) {
bool isLoadableField = isa<LoadableTypeInfo>(fieldType);
unsigned explosionSize = 0;
if (isLoadableField)
Expand All @@ -1517,11 +1518,15 @@ class ClangRecordLowering {
unsigned explosionEnd = NextExplosionIndex;

ElementLayout layout = ElementLayout::getIncomplete(fieldType);
auto isEmpty = fieldType.isKnownEmpty(ResilienceExpansion::Maximal);
if (isEmpty)
layout.completeEmptyTailAllocatedCType(
fieldType.isTriviallyDestroyable(ResilienceExpansion::Maximal), NextOffset);
else
auto isEmpty = isZeroSized || fieldType.isKnownEmpty(ResilienceExpansion::Maximal);
if (isEmpty) {
if (isZeroSized)
layout.completeEmpty(
Copy link
Contributor

@Michael137 Michael137 Apr 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

llvm/llvm-project#50766

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Michael137 Michael137 Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fieldType.isTriviallyDestroyable(ResilienceExpansion::Maximal), NextOffset);
else
layout.completeEmptyTailAllocatedCType(
fieldType.isTriviallyDestroyable(ResilienceExpansion::Maximal), NextOffset);
} else
layout.completeFixed(fieldType.isTriviallyDestroyable(ResilienceExpansion::Maximal),
NextOffset, LLVMFields.size());

Expand Down
22 changes: 22 additions & 0 deletions test/Interop/Cxx/class/Inputs/member-variables.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,26 @@ class MyClass {
const int const_member = 23;
};

struct Empty {
using type = int;
int getNum() const { return 42; }
};

struct HasZeroSizedField {
int a;
[[no_unique_address]] Empty b;
short c;
[[no_unique_address]] Empty d;
int* e;
[[no_unique_address]] Empty f;

int get_a() const { return a; }
short get_c() const { return c; }
void set_c(short c) { this->c = c; }
};

inline int takesZeroSizedInCpp(HasZeroSizedField x) {
return x.a;
}

#endif
27 changes: 27 additions & 0 deletions test/Interop/Cxx/class/zero-sized-field.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// RUN: %target-run-simple-swift(-I %S/Inputs/ -Xfrontend -enable-experimental-cxx-interop -Xcc -std=c++20)
//
// REQUIRES: executable_test

import StdlibUnittest
import MemberVariables

var FieldsTestSuite = TestSuite("Generating code with zero sized fields")

func takeTypeWithZeroSizedMember(_ x: HasZeroSizedField) {}

FieldsTestSuite.test("Zero sized field") {
var s = HasZeroSizedField()
s.a = 5
s.set_c(7)
takeTypeWithZeroSizedMember(s)
let s2 = s
let myInt : Empty.type = 6
expectEqual(s.a, 5)
expectEqual(s.a, s.get_a())
expectEqual(s2.c, 7)
expectEqual(s2.c, s2.get_c())
expectEqual(takesZeroSizedInCpp(s2), 5)
expectEqual(s.b.getNum(), 42)
}

runAllTests()