Skip to content

[cxx-interop][Runtime] Initialize metadata of a Swift array of C++ references correctly #73615

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
Merged
merged 1 commit into from
May 16, 2024
Merged

[cxx-interop][Runtime] Initialize metadata of a Swift array of C++ references correctly #73615

merged 1 commit into from
May 16, 2024

Conversation

egorzhdan
Copy link
Contributor

@egorzhdan egorzhdan commented May 14, 2024 •
edited
Loading

This fixes a crash at runtime when destroying a Swift array of values of a C++ foreign reference type.

Swift optimizes the amount of metadata emitted for _ContiguousArrayStorage<Element> by reusing _ContiguousArrayStorage<AnyObject> whenever possible (see getContiguousArrayStorageType). However, C++ foreign reference types are not AnyObjects, since they have custom retain/release operations.

This change disables the _ContiguousArrayStorage metadata optimization for C++ reference types, which makes sure that swift_arrayDestroy will call the correct release operation for elements of [MyCxxRefType].

rdar://127154770 / resolves #72827

@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label May 14, 2024
@egorzhdan egorzhdan requested review from zoecarver and hyp as code owners May 14, 2024 15:15
@egorzhdan
Copy link
Contributor Author

@swift-ci please test

mikeash
mikeash approved these changes May 14, 2024
View reviewed changes
Copy link
Contributor

@mikeash mikeash left a comment

Choose a reason for hiding this comment

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

A delightful test:change ratio.

@egorzhdan egorzhdan force-pushed the egorzhdan/frt-array-metadata branch from 065be95 to b75cb51 Compare May 14, 2024 16:19
@egorzhdan
Copy link
Contributor Author

@swift-ci please test

zoecarver
zoecarver approved these changes May 14, 2024
View reviewed changes
Copy link
Contributor

@zoecarver zoecarver left a comment

Choose a reason for hiding this comment

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

Assuming we don't care about ABI, LGTM.

@ravikandhadai
Copy link
Contributor

ravikandhadai commented May 14, 2024 •
edited
Loading

Assuming we don't care about ABI, LGTM.

Wondering what kind of ABI change do we expect for foreign reference types from this PR?

@zoecarver
Copy link
Contributor

Assuming we don't care about ABI, LGTM.

Wondering what kind of ABI change do we expect for foreign reference types from this PR?

Someone who knows more about the Swift runtime and ABI should comment, but I think this change might cause FRTs to lose some protocol witnesses. There might be some other edge cases too beyond the runtime implications.

@mikeash
Copy link
Contributor

mikeash commented May 15, 2024

As far as I can tell, this function is only used for runtime casting decisions. This shouldn't be an ABI break, exactly, but some casts that previously succeeded will now fail, and casts to AnyObject will presumably produce a boxed value now instead of just producing the value verbatim. This should be OK, because almost any real-world use of such a cast would have crashed when it tried to retain/release the C++ reference.

@egorzhdan
[cxx-interop][Runtime] Initialize metadata of a Swift array of C++ re…
fd04cc3 
…ferences correctly

This fixes a crash at runtime when destroying a Swift array of values of a C++ foreign reference type.

Swift optimizes the amount of metadata emitted for `_ContiguousArrayStorage<Element>` by reusing `_ContiguousArrayStorage<AnyObject>` whenever possible (see `getContiguousArrayStorageType`). However, C++ foreign reference types are not `AnyObject`s, since they have custom retain/release operations.

This change disables the `_ContiguousArrayStorage` metadata optimization for C++ reference types, which makes sure that `swift_arrayDestroy` will call the correct release operation for elements of `[MyCxxRefType]`.

rdar://127154770
@egorzhdan egorzhdan force-pushed the egorzhdan/frt-array-metadata branch from b75cb51 to fd04cc3 Compare May 15, 2024 18:21
@egorzhdan
Copy link
Contributor Author

@swift-ci please test

@egorzhdan egorzhdan requested a review from aschwaighofer May 15, 2024 18:21
@egorzhdan
Copy link
Contributor Author

@aschwaighofer I had to tweak the getContiguousArrayStorageType<AnyObject> logic in SILOptimizer to make this work for optimized builds, could you please look over to check if I did it right?

aschwaighofer
aschwaighofer approved these changes May 16, 2024
View reviewed changes
Copy link
Contributor

@aschwaighofer aschwaighofer left a comment

Choose a reason for hiding this comment

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

The SIL optimizer stuff looks good to me.

I don't presume to be able to predict what will happen due to the change in Metadata.h

@egorzhdan egorzhdan merged commit 764ba94 into main May 16, 2024
5 checks passed
@egorzhdan egorzhdan deleted the egorzhdan/frt-array-metadata branch May 16, 2024 15:44
This was referenced May 16, 2024
Closed
Merged
egorzhdan added a commit that referenced this pull request Jun 10, 2024
@egorzhdan
[cxx-interop] Only run tests for reference types against a recent Swi…
0f420fe 
…ft runtime

Having a Swift array of C++ reference types without crashing at runtime only became possible recently (#73615). When building against an older runtime, one would still see runtime crashes. This is expected because part of the fix is in the Swift runtime.

This makes sure we don't try to run tests for C++ reference types with an older Swift runtime.

rdar://128681137
@egorzhdan egorzhdan mentioned this pull request Jun 10, 2024
Merged
egorzhdan added a commit that referenced this pull request Jun 11, 2024
@egorzhdan
[cxx-interop] Only run tests for reference types against a recent Swi…
a58feb3 
…ft runtime

Having a Swift array of C++ reference types without crashing at runtime only became possible recently (#73615). When building against an older runtime, one would still see runtime crashes. This is expected because part of the fix is in the Swift runtime.

This makes sure we don't try to run tests for C++ reference types with an older Swift runtime.

rdar://128681137
(cherry picked from commit 0f420fe)
@egorzhdan egorzhdan mentioned this pull request Jun 11, 2024
Merged
ktoso pushed a commit to ktoso/swift that referenced this pull request Jun 14, 2024
@egorzhdan @ktoso
[cxx-interop] Only run tests for reference types against a recent Swi…
611971f 
…ft runtime

Having a Swift array of C++ reference types without crashing at runtime only became possible recently (swiftlang#73615). When building against an older runtime, one would still see runtime crashes. This is expected because part of the fix is in the Swift runtime.

This makes sure we don't try to run tests for C++ reference types with an older Swift runtime.

rdar://128681137
(cherry picked from commit 0f420fe)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikeash mikeash mikeash approved these changes

@zoecarver zoecarver zoecarver approved these changes

@aschwaighofer aschwaighofer aschwaighofer approved these changes

@ahatanaka ahatanaka Awaiting requested review from ahatanaka

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

@ravikandhadai ravikandhadai Awaiting requested review from ravikandhadai

@hyp hyp Awaiting requested review from hyp

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.

crash with array of C++ objects exposed via SWIFT_SHARED_REFERENCE
5 participants
@egorzhdan @ravikandhadai @zoecarver @mikeash @aschwaighofer