Skip to content

[cxx-interop] Fix metadata mismatch regarding fields of structs #81035

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[cxx-interop] Fix metadata mismatch regarding fields of structs #81035

wants to merge 1 commit into from
+265 −28

Conversation

susmonteiro
Copy link
Contributor

@susmonteiro susmonteiro commented Apr 23, 2025 •
edited
Loading

In #78467 and #78961, we stopped emitting metadata for private C++ fields. However, this created a mismatch between the fields emitted and the number of fields + their offsets in the StructDescriptor.

rdar://147263490

@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

@susmonteiro susmonteiro added the c++ interop Feature: Interoperability with C++ label Apr 23, 2025
@susmonteiro susmonteiro force-pushed the susmonteiro/metadata-private-fields branch from f36ff56 to a39010b Compare April 24, 2025 10:59
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

@susmonteiro susmonteiro marked this pull request as ready for review April 24, 2025 11:03
Xazax-hun
Xazax-hun reviewed Apr 24, 2025
View reviewed changes
Copy link
Contributor

@Xazax-hun Xazax-hun left a comment

Choose a reason for hiding this comment

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

This is great, thanks for tracking this down! I am not super confident about some details, asked some questions inline. Maybe we should get someone more familiar with type metadata reviewed this PR.

lib/IRGen/GenMeta.cpp
@@ -1845,7 +1845,7 @@ namespace {

void addLayoutInfo() {
// uint32_t NumFields;
B.addInt32(getNumFields(getType()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we need to do something similar for ClassContextDescriptorBuilder too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am less sure about emitInitializeFieldOffsetVector and co. Maybe those are good as is but maybe we want to ask someone familiar with type metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if we need to do something similar for ClassContextDescriptorBuilder too.

Yes I think you're right. I intend to check if we need a similar fix for classes and add those changes in a future PR

Regarding emitInitializeFieldOffsetVector, I'm also not sure. I'll investigate this further

lib/IRGen/StructMetadataVisitor.h
@@ -64,7 +65,8 @@ template <class Impl> class StructMetadataVisitor
// Struct field offsets.
asImpl().noteStartOfFieldOffsets();
for (VarDecl *prop : Target->getStoredProperties())
asImpl().addFieldOffset(prop);
if (!isPrivateField(prop))
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can get out of sync with emitInitializeFieldOffsetVector here.

@susmonteiro susmonteiro force-pushed the susmonteiro/metadata-private-fields branch from a39010b to a49738e Compare April 24, 2025 14:49
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

@susmonteiro susmonteiro force-pushed the susmonteiro/metadata-private-fields branch from a49738e to abd2549 Compare April 25, 2025 11:52
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

Xazax-hun
Xazax-hun reviewed Apr 25, 2025
View reviewed changes
test/Interop/Cxx/stdlib/print-stdlib-types.swift
// CHECK-linux-gnu: 42
// CHECK-linux-gnu: shared_ptr<CInt>()
printStdSharedPtrOfString()
// CHECK-macosx: basic_string<CChar, std.__1.char_traits<CChar>, std.__1.allocator<CChar>>()
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, you could add regexes that match on all platforms. I don't have a preference.

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 for the suggestion!
Hopefully we will soon fix the conformance of std::string to CustomStringConvertible. Then we can remove all these os options, since the value of the string will be printed instead of the type name.
That said, I don't mind switching to regex

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, the string one is temporary (maybe not the shared_ptr one though). Let's leave it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right!

@susmonteiro susmonteiro force-pushed the susmonteiro/metadata-private-fields branch from abd2549 to 2a99a7d Compare April 25, 2025 15:25
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

Xazax-hun
Xazax-hun previously approved these changes Apr 25, 2025
View reviewed changes
test/Interop/Cxx/stdlib/print-stdlib-types.swift
// CHECK-linux-gnu: 42
// CHECK-linux-gnu: shared_ptr<CInt>()
printStdSharedPtrOfString()
// CHECK-macosx: basic_string<CChar, std.__1.char_traits<CChar>, std.__1.allocator<CChar>>()
Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, the string one is temporary (maybe not the shared_ptr one though). Let's leave it as is.

@Xazax-hun Xazax-hun dismissed their stale review April 25, 2025 15:32

Misclicked.

@susmonteiro susmonteiro force-pushed the susmonteiro/metadata-private-fields branch from 2a99a7d to bdacdd0 Compare April 25, 2025 15:42
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

@susmonteiro
Copy link
Contributor Author

@swift-ci Please test macOS platform

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xazax-hun Xazax-hun Xazax-hun left review comments

@zoecarver zoecarver Awaiting requested review from zoecarver zoecarver is a code owner

@egorzhdan egorzhdan Awaiting requested review from egorzhdan egorzhdan is a code owner

@j-hui j-hui Awaiting requested review from j-hui j-hui is a code owner

@fahadnayyar fahadnayyar Awaiting requested review from fahadnayyar fahadnayyar is a code owner

@hnrklssn hnrklssn Awaiting requested review from hnrklssn hnrklssn is a code owner

@rjmccall rjmccall Awaiting requested review from rjmccall rjmccall is a code owner

Assignees
No one assigned
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@susmonteiro @Xazax-hun