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][5.9] windows ABI fixes #67442

Merged
merged 3 commits into from
Jul 22, 2023

Conversation

hyp
Copy link
Contributor

@hyp hyp commented Jul 20, 2023

[cxx-interop] Fix the windows ABI for returning indirect values out of methods

Fixes https://github.com/apple/swift/issues/66326

This allows us to reneable Windows method tests. Note that Windows still has
a broken convention for non-trivial record with non-trivial destructor but
trivial copy-constructor, so classes in the methods.swift test need an explicit
copy constructor.

Fixes rdar://88391102

[cxx-interop] Windows: unify address-only logic and mark non-trivial loadable C++ types as unavailable

Windows logic for determining address-only type layout for a C++ type is now unified with other platforms.
However, this means that on Windows, a C++ type with a custom destructor, but a default copy constructor
is now loadable, even though it's non-trivial. Since Swift does not support such type operations at the
moment (it can't be yet destroyed), mark such type as unavailable in Swift instead, when building for
the Windows target.

This fixes the Windows miscompilation related to such types when they were passed indirectly to C++
functions even though they're actually passed directly.
  • Explanation:
    Windows MSVC ABI had two problems with IRGen for calls to C++:
  1. Methods returning values indirectly had incorrect order between this and the returned value. This is now fixed, and we get the ordering right for the MSVC ABI.
  2. Non-trivial but passed in registers types (MSVC ABI example - C++ class with destructor but default copy constructor) were incorrectly passed indirectly as we marked them as address-only. This patch now marks them loadable, which fixes that issue. However, we cannot yet support such types correctly semantically, as they can't be yet destroyed from Swift. Therefore, mark such types as unavailable on Windows to avoid missing C++ destructor calls from Swift.
  • Scope: C++ interop, Clang importer, IRGen.
  • Risk: Low, the affected code changes only affect the MSVC windows target, and only affect C++ imported types, not C.
  • Testing: Unit tests.
  • Original PR: [cxx-interop] windows methods fixes #67268

hyp added 2 commits July 20, 2023 15:27
…f methods

Fixes swiftlang#66326

This allows us to reneable Windows method tests. Note that Windows still has
a broken convention for non-trivial record with non-trivial destructor but
trivial copy-constructor, so classes in the methods.swift test need an explicit
copy constructor.

Fixes rdar://88391102
…loadable C++ types as unavailable

Windows logic for determining address-only type layout for a C++ type is now unified with other platforms.
However, this means that on Windows, a C++ type with a custom destructor, but a default copy constructor
is now loadable, even though it's non-trivial. Since Swift does not support such type operations at the
moment (it can't be yet destroyed), mark such type as unavailable in Swift instead, when building for
the Windows target.

This fixes the Windows miscompilation related to such types when they were passed indirectly to C++
functions even though they're actually passed directly.
@hyp hyp added c++ interop Feature: Interoperability with C++ swift 5.9 labels Jul 20, 2023
@hyp hyp requested a review from a team as a code owner July 20, 2023 22:34
@hyp
Copy link
Contributor Author

hyp commented Jul 20, 2023

@swift-ci please test

@hyp hyp changed the title [cxx-interop] windows ABI fixes [cxx-interop][5.9] windows ABI fixes Jul 20, 2023
@hyp
Copy link
Contributor Author

hyp commented Jul 21, 2023

@swift-ci please test windows platform

@hyp
Copy link
Contributor Author

hyp commented Jul 21, 2023

@swift-ci please test linux platform

@hyp hyp force-pushed the eng/5.9/windows-almighty branch from aa1906c to 9b721b2 Compare July 21, 2023 22:21
@hyp
Copy link
Contributor Author

hyp commented Jul 21, 2023

@swift-ci please test

@hyp
Copy link
Contributor Author

hyp commented Jul 21, 2023

@swift-ci please test source compatibility

@hyp hyp merged commit 3671202 into swiftlang:release/5.9 Jul 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++ swift 5.9
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cxx-interop] fix the C++ methods dispatch for the Windows target
2 participants