Skip to content

[cxx-interop] ensure that the body of implicit virtual destructor is … #74640

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

Merged

Conversation

hyp
Copy link
Contributor

@hyp hyp commented Jun 22, 2024

…defined before emitting the clang decl for it

This ensures that the destructor definition is emitted into the module's LLVM IR, fixing a linker error. The failure can be observed only on a Linux target, like Android, and Darwin/MSVC handle this case just fine.

…defined before emitting the clang decl for it

This ensures that the destructor definition is emitted into the module's LLVM IR, fixing a linker error.
@hyp hyp requested a review from egorzhdan as a code owner June 22, 2024 00:01
@hyp hyp requested review from compnerd and hjyamauchi June 22, 2024 00:01
@hyp
Copy link
Contributor Author

hyp commented Jun 22, 2024

@swift-ci please test

@egorzhdan egorzhdan requested review from Xazax-hun June 24, 2024 14:03
@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label Jun 24, 2024
Comment on lines 347 to 348
assert(!destructor->isDeleted() &&
"Swift cannot handle a type with no known destructor.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this assertion fail if struct Base has a deleted destructor but struct Derived : Base defines an explicit destructor? As far as I can see we would currently import Derived into Swift, and then fail with an assertion here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Let me test!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this was triggered. I fixed it by adding a check for a deleted destructor before the new invocation path, and added a test too.

@hyp hyp force-pushed the eng/virtual-destructor-vtable-ref-irgen-linux-android branch from 359565a to f4949bd Compare June 26, 2024 01:22
@hyp
Copy link
Contributor Author

hyp commented Jun 26, 2024

@swift-ci please test

@hyp hyp merged commit af36238 into swiftlang:main Jun 26, 2024
5 checks passed
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++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants