-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Conversation
@swift-ci please test |
Explanation: Swift compiler was passing foreign reference types to runtime function |
if (IGF.IGM.Context.LangOpts.EnableObjCInterop) | ||
result = emitObjCRetainAutoreleasedReturnValue(IGF, result); | ||
else | ||
if (IGF.IGM.Context.LangOpts.EnableObjCInterop) { |
There was a problem hiding this comment.
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.
@swift-ci please test macOS |
@@ -2605,9 +2606,16 @@ class SyncCallEmission final : public CallEmission { | |||
if (fnConv.getNumDirectSILResults() == 1 | |||
&& (fnConv.getDirectSILResults().begin()->getConvention() | |||
== ResultConvention::Autoreleased)) { |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
I'm reverting all commits from main: #73965. |
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)