Skip to content

[cxx-interop] Make sure interop does not trigger TBD validation errors #64841

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
merged 1 commit into from
Apr 6, 2023

Conversation

egorzhdan
Copy link
Contributor

rdar://83405989 / #56458

@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label Apr 1, 2023
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan
Copy link
Contributor Author

@swift-ci please test

@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-tbd-validation branch from 9406511 to 43161fd Compare April 1, 2023 17:22
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan
Copy link
Contributor Author

@swift-ci please test

@egorzhdan egorzhdan marked this pull request as ready for review April 1, 2023 17:23
@egorzhdan egorzhdan requested review from zoecarver and hyp as code owners April 1, 2023 17:23
@hyp
Copy link
Contributor

hyp commented Apr 4, 2023

I think that's fine, but we should be mindful that it's not necessarily a fix, rather a workaround. We do have additional issues tracking this here:
#61453, but there are probably other cases.

@@ -410,6 +411,14 @@ void ArgsToFrontendOptionsConverter::computeTBDOptions() {
Diags.diagnose(SourceLoc(), diag::error_unsupported_option_argument,
A->getOption().getPrefixedName(), value);
}
} else if (Args.hasArg(OPT_enable_experimental_cxx_interop) ||
Args.hasArg(OPT_cxx_interoperability_mode)) {
// TBD validation currently emits erroneous diagnostics when C++ interop is
Copy link
Contributor

Choose a reason for hiding this comment

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

I would argue that the diagnostics are not erroneous, but rather that IRGen is incorrectly applying linkage and/or visibility to the symbols. I think we need to clarify in the comment that this is specifically a workaround for correct diagnostics, and that it also must be a FIXME.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that makes sense, I updated the comment.

@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-tbd-validation branch from 43161fd to 50910da Compare April 5, 2023 15:06
This disables TBD validation when C++ interop is enabled, unless an explicit `-validate-tbd-against-ir=` flag was passed.

rdar://83405989 / #56458
@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-tbd-validation branch from 50910da to 8f58eaf Compare April 5, 2023 15:06
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan egorzhdan merged commit 05bcc88 into main Apr 6, 2023
@egorzhdan egorzhdan deleted the egorzhdan/cxx-tbd-validation branch April 6, 2023 13:30
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.

2 participants