Skip to content

[lldb] Try Clang fallback in TSSwiftTypeRef::GetNumFields #6671

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

Conversation

kastiglione
Copy link

@kastiglione kastiglione commented Apr 18, 2023

For Clang types, avoid loading Swift ASTContexts in TypeSystemSwiftTypeRef::GetNumFields.

This new logic checks if a type is an imported Clang type. If it's imported, but no type info is available (ex private/internal types), then zero is returned. If a Clang type is available, then, depending on the type class, return the Clang type's GetNumFields.

Note that there's logic to avoid calling GetNumFields on ObjC types. This is to match the current behavior.

@kastiglione
Copy link
Author

@swift-ci test Linux

@kastiglione
Copy link
Author

@swift-ci test Windows

@kastiglione
Copy link
Author

@swift-ci test

@kastiglione kastiglione changed the title [lldb] Try clang fallback TSSSwiftTypeRef::GetNumFields [lldb] Try clang fallback TSSwiftTypeRef::GetNumFields Apr 19, 2023
@kastiglione kastiglione force-pushed the dl/lldb-try-clang-fallback-tssswifttyperef-getnumfields branch from aafc6cc to 02d5ada Compare April 19, 2023 20:28
@kastiglione
Copy link
Author

@swift-ci test

@kastiglione
Copy link
Author

@swift-ci test windows

// Imported ObjC types are treated as having no fields.
return 0;
default:
return clang_type.GetNumFields();

Choose a reason for hiding this comment

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

I suppose the validation macro should tell us if these assumptions are wrong?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, they did their job when I wrote this. At first I tried returning 0 for all clang types, but that broke some tests and allowed me to tweak this logic.

Choose a reason for hiding this comment

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

We already talked about this offline, but in case anyone else is reading this — it would be good to understand why this doesn't already work via the GetSwiftRuntimeTypeInfo -> LLDBTypeInfoProvider code path.

Copy link
Author

Choose a reason for hiding this comment

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

@adrian-prantl The typeref for imported objc classes is ObjCClassTypeRef, ex:

(swift::reflection::ObjCClassTypeRef *) tr = 0x00006000029d5a10 {
  swift::reflection::TypeRef = (Kind = ObjCClass)
  Name = "NSApplication"
}

However, the TypeInfoProvider is currently used from NominalTypeRef and BoundGenericTypeRef.

See visitAnyNominalTypeRef https://github.com/apple/swift/blob/022311e438e7703c46e400ba01882ccb5a1236ca/stdlib/public/RemoteInspection/TypeLowering.cpp#L2302-L2379.

@kastiglione kastiglione changed the title [lldb] Try clang fallback TSSwiftTypeRef::GetNumFields [lldb] Try Clang fallback in TSSwiftTypeRef::GetNumFields Apr 19, 2023
@kastiglione
Copy link
Author

@swift-ci test

@kastiglione
Copy link
Author

@adrian-prantl I've refactored this a bit, and handle the case where a type is known to be imported, but has no type info (such as for private/internal types).

@kastiglione
Copy link
Author

@swift-ci test windows

}
} else if (is_imported) {
// A known imported type, but where clang has no info. Return early to
// avoid loading Swift ASTContexts, only to return the same zero value.

Choose a reason for hiding this comment

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

Should this log something?

Copy link
Author

Choose a reason for hiding this comment

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

Good call, added a log.

@kastiglione
Copy link
Author

@swift-ci test

@kastiglione
Copy link
Author

@swift-ci test windows

1 similar comment
@kastiglione
Copy link
Author

@swift-ci test windows

@kastiglione kastiglione merged commit 80c77db into swift/release/5.9 Apr 22, 2023
@kastiglione kastiglione deleted the dl/lldb-try-clang-fallback-tssswifttyperef-getnumfields branch April 22, 2023 14:05
kastiglione added a commit that referenced this pull request May 5, 2023
For Clang types, avoid loading Swift ASTContexts in `TypeSystemSwiftTypeRef::GetNumFields`.

This new logic checks if a type is an imported Clang type. If it's imported, but no type info is available (ex private/internal types), then zero is returned. If a Clang type is available, then, depending on the type class, return the Clang type's `GetNumFields`.

Note that there's logic to avoid calling `GetNumFields` on ObjC types. This is to match the current behavior.

(cherry picked from commit 80c77db)
kastiglione added a commit that referenced this pull request May 5, 2023
)

For Clang types, avoid loading Swift ASTContexts in `TypeSystemSwiftTypeRef::GetNumFields`.

This new logic checks if a type is an imported Clang type. If it's imported, but no type info is available (ex private/internal types), then zero is returned. If a Clang type is available, then, depending on the type class, return the Clang type's `GetNumFields`.

Note that there's logic to avoid calling `GetNumFields` on ObjC types. This is to match the current behavior.

(cherry picked from commit 80c77db)
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.

2 participants