Fix ASTMangler mangling NS_OPTION differently in C++ mode #64043
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
NuriAmari
force-pushed
the
ns-option-linkage-conflict
branch
from
7244342 to
b5eb7b7
Compare
NuriAmari
commented
Mar 2, 2023
NuriAmari
commented
Mar 2, 2023
lib/AST/ASTMangler.cpp
Outdated
| // Use an anonymous enum's enclosing typedef for the mangled name, if | ||
| // present. This matches C++'s rules for linkage names of tag declarations. | ||
| if (namedDecl->getDeclName().isEmpty()) | ||
| if (auto *tagDecl = dyn_cast<clang::TagDecl>(namedDecl)) | ||
| if (auto *typedefDecl = tagDecl->getTypedefNameForAnonDecl()) |
There was a problem hiding this comment.
This is a little weird. I want to make the Clang AST look like the Objective-C definition so that the mangled name will be the same, but I don't want the mutation to persist beyond after the mangler is done with it. More specifically, I want the anonymous enum to no longer be anonymous and instead have the backing typedef's name.
I considered making a copy of the node, but that seems pretty wasteful. I'd have to copy at least once per NS_OPTION use and keep them around somewhere. Instead I've made this RAII container that just clears the anonymous enum's name after the client is done with it.
NuriAmari
commented
Mar 2, 2023
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility suite |
|
@swift-ci Please Test Source Compatibility |
NuriAmari
requested review from
zoecarver,
hyp,
egorzhdan,
hborla,
slavapestov and
xedin
as code owners
NuriAmari
force-pushed
the
ns-option-linkage-conflict
branch
from
b5eb7b7 to
e3bf0ab
Compare
|
@swift-ci Please test |
|
@swift-ci Please Test Source Compatibility |
|
@swift-ci Please test macOS platform |
|
@swift-ci Please test Windows platform |
plotfi
reviewed
Mar 5, 2023
|
LGTM. I will merge once we get a chance to chat about this 1:1 this week. |
NuriAmari
force-pushed
the
ns-option-linkage-conflict
branch
from
e3bf0ab to
6a6b4f6
Compare
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
Compat suite failures are unrelated. The change passed compat suite before, and I've only made non-functional changes since to address PR comments. |
NuriAmari
force-pushed
the
ns-option-linkage-conflict
branch
from
6a6b4f6 to
508d5d6
Compare
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
RAII AST meddling no more -- teach the mangler to handle it instead. |
|
@swift-ci Please test source compatibility |
drodriguez
approved these changes
Mar 9, 2023
CF_OPTIONS is defined differently in the SDK based on a __cplusplus preprocessor branch. As a result, declarations referencing CF_OPTIONS are mangled differently depending on if C++ interop is enabled. This meant a module compiled with cxx interop on could not be linked with a module compiled without and vice versa. This patch modifies the mangler such that the mangled names are consistent. This is achieved by feeding the mangler a modified AST node that looks like the Objective-C definition of CF_OPTIONS, even when we have cxx interop enabled.
NuriAmari
force-pushed
the
ns-option-linkage-conflict
branch
from
508d5d6 to
709321b
Compare
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please test macOS platform |
|
@swift-ci Please Test Source Compatibility Release |
hyp
approved these changes
Mar 17, 2023
plotfi
added a commit
to plotfi/swift
that referenced
this pull request
Jun 14, 2023
… be desugared This patch is an add-on to swiftlang#64043. Essentially when encountering NS_OPTIONS enums, in C++-Interop mode if they are not specially handled then they can mangle differently than they do without C++-Interop. This patch adds logic to handle when a typedef and enum have additional clang::ElaboratedType sugar, but otherwise it does the same as the existing 64043 patch. The test case provided was encountered in a real app build. The problem came from when two modules are each compiled one with and one without C++-Interop. For the test case code provided the mangling of the protocol conformance is not consistent and the code in SILGenLazyConformance.cpp crashes on an invalid conformance with reason "Invalid conformance in type-checked AST".
plotfi
added a commit
to plotfi/swift
that referenced
this pull request
Jun 15, 2023
… be desugared This patch is an add-on to swiftlang#64043. Essentially when encountering NS_OPTIONS enums, in C++-Interop mode if they are not specially handled then they can mangle differently than they do without C++-Interop. This patch adds logic to handle when a typedef and enum have additional clang::ElaboratedType sugar, but otherwise it does the same as the existing 64043 patch. The test case provided was encountered in a real app build. The problem came from when two modules are each compiled one with and one without C++-Interop. For the test case code provided the mangling of the protocol conformance is not consistent and the code in SILGenLazyConformance.cpp crashes on an invalid conformance with reason "Invalid conformance in type-checked AST".
plotfi
added a commit
to plotfi/swift
that referenced
this pull request
Jun 15, 2023
… be desugared This patch is an add-on to swiftlang#64043. Essentially when encountering NS_OPTIONS enums, in C++-Interop mode if they are not specially handled then they can mangle differently than they do without C++-Interop. This patch adds logic to handle when a typedef and enum have additional clang::ElaboratedType sugar, but otherwise it does the same as the existing 64043 patch. The test case provided was encountered in a real app build. The problem came from when two modules are each compiled one with and one without C++-Interop. For the test case code provided the mangling of the protocol conformance is not consistent and the code in SILGenLazyConformance.cpp crashes on an invalid conformance with reason "Invalid conformance in type-checked AST".
plotfi
added a commit
to plotfi/swift
that referenced
this pull request
Jun 16, 2023
… be desugared This patch is an add-on to swiftlang#64043. Essentially when encountering NS_OPTIONS enums, in C++-Interop mode if they are not specially handled then they can mangle differently than they do without C++-Interop. This patch adds logic to handle when a typedef and enum have additional clang::ElaboratedType sugar, but otherwise it does the same as the existing 64043 patch. The test case provided was encountered in a real app build. The problem came from when two modules are each compiled one with and one without C++-Interop. For the test case code provided the mangling of the protocol conformance is not consistent and the code in SILGenLazyConformance.cpp crashes on an invalid conformance with reason "Invalid conformance in type-checked AST". (cherry picked from commit fe6ccd7)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
CF_OPTIONS is defined differently in the SDK based on a __cplusplus preprocessor branch. As a result, declarations referencing CF_OPTIONS are mangled differently depending on if C++ interop is enabled.
This meant a module compiled with cxx interop on could not be linked with a module compiled without and vice versa. This patch modifies the mangler such that the mangled names are consistent. This is achieved by feeding the mangler a modified AST node that looks like the Objective-C definition of CF_OPTIONS, even when we have cxx interop enabled.