[cxx-interop] Fix miscompilation for inferred shared references #86124
+271
−46
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Xazax-hun
requested review from
beccadax,
egorzhdan,
hborla,
hnrklssn,
ian-twilightcoder,
j-hui,
rjmccall,
slavapestov,
susmonteiro and
xedin
as code owners
Xazax-hun
force-pushed
the
fix-multiple-inheritance-refcounts
branch
from
59564f4 to
b0da742
Compare
egorzhdan
reviewed
Dec 18, 2025
Contributor
egorzhdan
left a comment
egorzhdan
left a comment
There was a problem hiding this comment.
Looks great! Just a couple minor comments.
When a base class is annotated as shared reference we can occasionally infer that the derived types also need to be shared references. Unfortunately, we did not generate the correct code for some of those scenarios. When the reference counted base is not at the offset zero we need to do offset adjustments before we pass the pointer to the reference counting functions. We did not do those offset calculations. I looked into implementing the codegen for the offset calculation directly in Swift but it needed significantly more work than I anticipated. We need to invoke the frontend to get the path to the base class and we also need to deal with virtual inheritance, alignment and some other considerations. This PR ends up generating a Clang shim instead and the derived to base conversion happens in this shim. As a result, we piggy-back on Clang making all the correct offset calculations. This patch also had to change how certain aspects of shared references are implemented to be compatible with this approach: * Instead of always looking at the base classes to querry the retain/release operations we now propagate the corresponding annotations once per types. This also has the beneficial effects that we traverse the inheritance hierarchy less often. * To generate the correct diagnostics, I reuse the result of the refcount operation query. * We do not want these generated functions to be inherited, so added a set to exempt them from cloning. * Tweaked the lookup logic for retain/release a bit as these generated clang methods are not found by lookup. We rely on looking up the imported methods instead. rdar://166227787 rdar://165635002
Xazax-hun
force-pushed
the
fix-multiple-inheritance-refcounts
branch
from
b0da742 to
35bcc99
Compare
Contributor
Author
|
@swift-ci please smoke test |
hnrklssn
approved these changes
Dec 18, 2025
Contributor
Author
|
@swift-ci please smoke test Windows |
Contributor
Author
|
@swift-ci please smoke test macOS |
egorzhdan
approved these changes
Dec 19, 2025
Contributor
|
@swift-ci please smoke test Windows |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a base class is annotated as shared reference we can occasionally infer that the derived types also need to be shared references. Unfortunately, we did not generate the correct code for some of those scenarios. When the reference counted base is not at the offset zero we need to do offset adjustments before we pass the pointer to the reference counting functions. We did not do those offset calculations.
I looked into implementing the codegen for the offset calculation directly in Swift but it needed significantly more work than I anticipated. We need to invoke the frontend to get the path to the base class and we also need to deal with virtual inheritance, alignment and some other considerations.
This PR ends up generating a Clang shim instead and the derived to base conversion happens in this shim. As a result, we piggy-back on Clang making all the correct offset calculations. This patch also had to change how certain aspects of shared references are implemented to be compatible with this approach:
rdar://166227787