Skip to content

[lldb] Check if C++ interop and embedded Swift are enabled on CU instead of whole module #10279

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

augusto2112
Copy link

Checking if C++ interop is enabled can be extremely slow if the project is big enough. We have changed this to checking only the compile unit instead, but one usage of the old API slipped by.

rdar://147009063

@augusto2112 augusto2112 requested a review from a team as a code owner March 17, 2025 23:31
@augusto2112
Copy link
Author

@swift-ci test

Copy link

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

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

That makes a lot more sense than the copy&paste error that was there before.

@augusto2112
Copy link
Author

@swift-ci test

Checking if C++ interop is enabled can be extremely slow if the project
is big enough. We have changed this to checking only the compile unit
instead, but one usage of the old API slipped by.

rdar://147009063
When deciding whether to format a C++ type as a Swiftified type or not,
checking if Swift/C++ interop is enabled on the entire target can be
very expensive for big projects, and may not swiftify types that should
be.

rdar://117708944
(cherry picked from commit fe06ebc)
Reading debug info of an entire module to check whether embedded Swift
is enabled or not can be very slow. This patch replaces that check with
only checking the compile unit instead.

rdar://147009063
@augusto2112 augusto2112 force-pushed the fix-interop-checking-mod branch from c2c8cdb to 03ee42c Compare March 18, 2025 21:58
@augusto2112
Copy link
Author

@swift-ci test

@augusto2112 augusto2112 changed the title [lldb] Check if C++ interop is enabled on CU instead of whole module [lldb] Check if C++ interop and embedded Swift are enabled on CU instead of whole module Mar 18, 2025
Copy link

@kastiglione kastiglione left a comment

Choose a reason for hiding this comment

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

The code looks reasonable to me, but I don't have the knowledge to know whether this prevents the expensive flag checks.

@augusto2112
Copy link
Author

@swift-ci test macOS

@augusto2112
Copy link
Author

@swift-ci test macos

@augusto2112 augusto2112 merged commit 0685fea into swiftlang:stable/20240723 Mar 19, 2025
3 checks passed
lldb::SymbolContextItem::eSymbolContextModule);

// If there is a compile unit, use that to check if C++ interop should be
// enabled. If there is no compiler unit, use the module. If neither

Choose a reason for hiding this comment

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

compiler -> compile

@@ -2653,7 +2650,7 @@ static bool ShouldEnableCXXInterop(CompileUnit *cu) {
if (unit.get() == cu) {
if (cu->GetLanguage() == eLanguageTypeSwift)
for (const char *arg : args.GetArgumentArrayRef())
if (strcmp(arg, "-enable-experimental-cxx-interop") == 0)
if (strcmp(arg, flag) == 0)

Choose a reason for hiding this comment

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

Do we not have a DWARF attribute for this?

Copy link
Author

Choose a reason for hiding this comment

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

We don't

@@ -2663,6 +2660,15 @@ static bool ShouldEnableCXXInterop(CompileUnit *cu) {
return false;
}

/// Determine whether this CU was compiled with C++ interop enabled.

Choose a reason for hiding this comment

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

this comment should be in the header

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants