Skip to content

Conversation

Xazax-hun
Copy link
Contributor

Add a null pointer to the value witness table instead of completely skipping emitting the pointer.

@Xazax-hun Xazax-hun requested a review from DougGregor September 8, 2025 10:48
@Xazax-hun Xazax-hun requested a review from rjmccall as a code owner September 8, 2025 10:48
@Xazax-hun Xazax-hun added the c++ interop Feature: Interoperability with C++ label Sep 8, 2025
Add a null pointer to the value witness table instead of completely
skipping emitting the pointer.
@Xazax-hun Xazax-hun force-pushed the add-nullptr-to-metadata branch from 1e9eb9f to cae23fd Compare September 8, 2025 10:51
@Xazax-hun
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This will avoid the assertion, thank you. Are we certain that we can't dynamically get to this value witness table? Should we instead stub out the entries in the table?

@Xazax-hun
Copy link
Contributor Author

Are we certain that we can't dynamically get to this value witness table? Should we instead stub out the entries in the table?

I am not 100% positive yet if there is a way to get to these. Maybe one could with reflection? That being said, I think such code is inherently faulty, the user should only rely on the enclosing non-anonymous type's value witness table.

In case we stub the entries in the table, what would you expect those stubs to do? Trap?

@Xazax-hun
Copy link
Contributor Author

Merging this for now to fix the size issue and will follow up with a separate PR for the stubs if we decide we should have them.

@Xazax-hun Xazax-hun merged commit 56a525b into swiftlang:main Sep 8, 2025
3 checks passed
@Xazax-hun Xazax-hun changed the title [cxx-interop] Make the size of anonymous types metadata is unchanged [cxx-interop] Make sure the size of anonymous types metadata is unchanged Sep 8, 2025
@rjmccall
Copy link
Contributor

rjmccall commented Sep 8, 2025

Can we figure out why we're emitting this type metadata in the first place? It's not a nameable type, and I feel like eagerly emitting type metadata for imported field types for reflective purposes is probably a mistake.

Xazax-hun added a commit to Xazax-hun/swift that referenced this pull request Sep 9, 2025
…data

[cxx-interop] Make the size of anonymous types metadata is unchanged
@Xazax-hun
Copy link
Contributor Author

Can we figure out why we're emitting this type metadata in the first place?

The way we currently import anonymous types to Swift we generate a field like __Unnamed_struct___Anonymous_field0 in the enclosing type on the Swift side. Users are not expected to refer to this field ever. We introduce accessors in the enclosing type and users are expected to use those accessors to access the fields of the anonymous structs.

When we generate lazy metadata for the enclosing (non-anonymous type), we will generate field descriptors for all the fields including the field representing the anonymous structure itself. The generation of the field descriptor will trigger the emission of the metadata for this anonymous type.

@Xazax-hun
Copy link
Contributor Author

eagerly emitting type metadata for imported field types for reflective purposes is probably a mistake.

Are you suggesting that we should not emit type metadata for any field when the parent is imported? I think we might have some facilities that might rely on this behavior. E.g., in Swift we can print a C struct and see the values of all the members.

Or do you mean we should be able to tell when we call something that relies on reflection so we should be more lazy with emitting this metadata?

@rjmccall
Copy link
Contributor

rjmccall commented Sep 9, 2025

I believe that printing can use several different sources of information. We could also potentially just extend the format of field descriptors to express the things we want to express without having to emit full type metadata records. Type metadata has a lot of overhead specifically related to its use in the runtime type system, so it would be reasonable to put some engineering effort into avoiding that for that types that we have no intention of modeling in Swift.

Also, there are a lot of C/C++ types for which printing their direct recursive structure is not actually useful to users. We're probably spending a lot of time and code size creating type metadata for the implementation details of STL types. We should really only be doing this for aggregates.

@Xazax-hun
Copy link
Contributor Author

Hmm, it looks like we actually will not generate the reflection related data for private field! So we already avoid generating metadata for some of the implementation details.

I put up a PR to avoid generating reflection metadata for anonymous types too: #84199

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants