Skip to content
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.

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

[cxx-interop][IRGen] Do not pass a foreign reference type to objc_retainAutoreleasedReturnValue #73710

Closed
wants to merge 1 commit into from

Conversation

ahatanaka
Copy link
Contributor

If a foreign reference type has a custom retain function, emit a call to it instead of emitting a call to objc_retainAutoreleasedReturnValue.

rdar://117353222
(cherry picked from commit 335cec0)

…ainAutoreleasedReturnValue (#73630)

If a foreign reference type has a custom retain function, emit a call to
it instead of emitting a call to objc_retainAutoreleasedReturnValue.

rdar://117353222
(cherry picked from commit 335cec0)
@ahatanaka ahatanaka requested a review from a team as a code owner May 17, 2024 16:10
@ahatanaka
Copy link
Contributor Author

@swift-ci please test

@ahatanaka
Copy link
Contributor Author

Explanation: Swift compiler was passing foreign reference types to runtime function objc_retainAutoreleasedReturnValue, which was causing an app to crash. The patch fixes the bug by emitting calls to custom retain functions instead.
Scope: This can change IRGen when ObjC is enabled.
Original PR: #73630
Risk: Low.
Testing: Added a compiler test.
Reviewer: @egorzhdan

@ahatanaka ahatanaka requested a review from DougGregor May 17, 2024 17:50
if (IGF.IGM.Context.LangOpts.EnableObjCInterop)
result = emitObjCRetainAutoreleasedReturnValue(IGF, result);
else
if (IGF.IGM.Context.LangOpts.EnableObjCInterop) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the EnableObjCInterop check here? The fact that custom reference counting currently only kicks in with C++ (is that even true?) might not be correct in the future, and we should be able to rely on the ReferenceCounting::Custom check to correctly identify the cases.

@ahatanaka
Copy link
Contributor Author

@swift-ci please test macOS

@@ -2605,9 +2606,16 @@ class SyncCallEmission final : public CallEmission {
if (fnConv.getNumDirectSILResults() == 1
&& (fnConv.getDirectSILResults().begin()->getConvention()
== ResultConvention::Autoreleased)) {
Copy link
Member

Choose a reason for hiding this comment

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

This is the problem. This is not a valid convention to be using with a type that uses custom reference counting, and trying to use it will cause a lot of deep problems.

else
if (IGF.IGM.Context.LangOpts.EnableObjCInterop) {
auto ty = fnConv.getSILResultType(IGF.IGM.getMaximalTypeExpansionContext());
auto *classTypeInfo = dyn_cast<ClassTypeInfo>(&IGF.IGM.getTypeInfo(ty));
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI this will succeed for any ReferenceTypeInfo, I have a patch to fix crashes here: #73915

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the fix @hamishknight.

@ahatanaka
Copy link
Contributor Author

I'm reverting all commits from main: #73965.

@ahatanaka ahatanaka closed this May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants