[lldb] Add ReferenceTypeInfo support to SwiftLanguageRuntimeImpl::GetNumChildren #2218

kastiglione · 2020-12-10T01:52:08Z

Add ReferenceTypeInfo support to SwiftLanguageRuntimeImpl::GetNumChildren. This uses TypeConverter::getClassInstanceTypeInfo to get a RecordTypeInfo for a class. And uses TypeRefBuilder::lookupSuperclass to determine if a class has a superclass (base class).

rdar://68171335

…NumChildren

kastiglione · 2020-12-10T01:52:40Z

lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

@@ -997,6 +997,38 @@ llvm::Optional<uint64_t> SwiftLanguageRuntimeImpl::GetMemberVariableOffset(
  return offset;
 }

+static CompilerType GetWeakReferent(TypeSystemSwiftTypeRef &ts,


this whole function was moved up and nothing more.

kastiglione · 2020-12-10T01:53:27Z

lldb/source/Target/SwiftLanguageRuntimeImpl.h

+  GetTypeInfo(CompilerType type, ExecutionContextScope *exe_scope,
+              swift::reflection::TypeRef const **out_tr = nullptr);


Added an out parameter to return the typeref.

It might be cleaner to factor out a function

const swift::reflection::TypeInfo *GetTypeInfo(swift::reflection::TypeRef type_ref)

and call that directly where we need the type_ref? If it isn't, then returning either a std::pair<> or a struct {TypeInfo type_info; TypeRef type_ref} may be more elegant.

kastiglione · 2020-12-10T01:54:33Z

Initial draft for availability/review, haven't done tests yet.

kastiglione · 2020-12-10T01:57:29Z

TODO

Figure out what value to use for start parameter of getClassInstanceTypeInfo

kastiglione · 2020-12-10T01:58:02Z

lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

+    auto *reflection_ctx = GetReflectionContext();
+    auto tc = swift::reflection::TypeConverter(reflection_ctx->getBuilder());
+    LLDBTypeInfoProvider tip(*this, *ts);
+    auto *cti = tc.getClassInstanceTypeInfo(tr, 0, &tip);


I have a todo to figure out if other values should be used instead of 0 for start.

The tests pass, and no SwiftASTContext validations fail when using start = 0. I'll learn more about this field before merging, but I don't think it's an issue.

Sounds good.

adrian-prantl · 2020-12-10T02:00:29Z

lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

+    auto *cti = tc.getClassInstanceTypeInfo(tr, 0, &tip);
+    if (auto *rti =
+            llvm::dyn_cast_or_null<swift::reflection::RecordTypeInfo>(cti)) {
+      return rti->getNumFields();


Should this be a recursive case or will there only ever be a record type underneath a class? (Well, probably).

Currently it's only ever a RecordTypeInfo:

https://github.com/apple/swift/blob/4f3f68ea48286278b2c071a6ec1320cb220e1eea/stdlib/public/Reflection/TypeLowering.cpp#L2229-L2233

We could factor out a helper function GetNumFieldsFromTypeInfo() that calls itself here, but the savings are marginal.

I'll soon be implementing GetNumFields, and which may call for some refactoring.

I have a question, by "calls itself" do you mean recursive like in your top comment? If so, can you elaborate on what needs to be recursive?

adrian-prantl · 2020-12-10T02:02:32Z

lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

+    if (!tr)
+      return {};
+
+    auto *reflection_ctx = GetReflectionContext();


Would this fit nicely into one case in the switch above, or are there many different kinds for which this path makes sense?

good question, I'm not sure yet. Once this PR is working, I'll compare the paths then.

What do you think? There are a couple distinct conditions in each case that don't make sense to the other case, and so combing the two doesn't feel like a no-brainer to me.

I was just thinking of writing this as

switch (rti->getReferenceKind()) { case swift::reflection::ReferenceKind::Weak: case swift::reflection::ReferenceKind::Unowned: case swift::reflection::ReferenceKind::Unmanaged: // Weak references are implicitly Optionals, so report the one // child of Optional here. if (GetWeakReferent(*ts, type)) return 1; break; default: { if (!tr) return {}; auto tc = swift::reflection::TypeConverter(reflection_ctx->getBuilder()); ....

but that's purely cosmetic and not important at all...

thanks, I had misunderstood. I will make that change in a follow up.

kastiglione · 2020-12-10T16:16:32Z

Currently one kind of test failure is with subclasses: GetNumChildren is off by one because it's not taking into account the child that represents the superclass. I'll need to figure out how determine whether the ReferenceTypeInfo has a parent class.

Another test failure is something I mentioned in #2200, that TypeRef's can expose private fields even when compiling with library evolution and .swiftinterfaces. By implementing this, it seems to have unlocked GetCompilerChildAtIndex to return private variables, so those tests will need to be updated.

adrian-prantl · 2020-12-10T16:31:54Z

Currently one kind of test failure is with subclasses: GetNumChildren is off by one because it's not taking into account the child that represents the superclass. I'll need to figure out how determine whether the ReferenceTypeInfo has a parent class.

Maybe you can reuse some of the code used in GetChildCompilerTypeAtIndex for this (although it does start out from an instance pointer).

Another test failure is something I mentioned in #2200, that TypeRef's can expose private fields even when compiling with library evolution and .swiftinterfaces. By implementing this, it seems to have unlocked GetCompilerChildAtIndex to return private variables, so those tests will need to be updated.

That's actually a good thing and precisely what we need in order for Foundation value types to work!

kastiglione · 2020-12-10T18:04:21Z

That's actually a good thing and precisely what we need in order for Foundation value types to work!

yes I was excited to see it!

kastiglione · 2020-12-10T18:19:08Z

Maybe you can reuse some of the code used in GetChildCompilerTypeAtIndex for this (although it does start out from an instance pointer).

I figured I'd need a way to do it with just the reflection API (no instance), and I found NominalTypeRef::getParent, but it's null. I've asked some Swift folks to see if that's the right API, or if there's something else.

kastiglione · 2020-12-10T20:32:28Z

@swift-ci test

kastiglione · 2020-12-10T21:38:49Z

@swift-ci test

kastiglione · 2020-12-10T23:05:53Z

Tests pass locally, hopefully CI too. Ready for review.

kastiglione · 2020-12-10T23:07:01Z

Maybe you can reuse some of the code used in GetChildCompilerTypeAtIndex for this (although it does start out from an instance pointer).

To follow up here, TypeRefBuilder::lookupSuperclass was used to determine if there's a superclass. See 3ff1e49

adrian-prantl

Functionally this LGTM. I have only cosmetic comments and those can also be addressed later.

kastiglione · 2020-12-11T01:20:52Z

Thanks, I will do the cosmetic changes in a followup, primarily because I don't want to wait another full test cycle.

[lldb] Add ReferenceTypeInfo support to SwiftLanguageRuntimeImpl::Get…

af7c46a

…NumChildren

kastiglione requested a review from adrian-prantl December 10, 2020 01:52

kastiglione commented Dec 10, 2020

View reviewed changes

kastiglione marked this pull request as draft December 10, 2020 01:54

fix early exit condition

da1254e

kastiglione commented Dec 10, 2020

View reviewed changes

adrian-prantl reviewed Dec 10, 2020

View reviewed changes

kastiglione added 3 commits December 10, 2020 12:29

Use TypeRefBuilder::lookupSuperclass

3ff1e49

fix TestLibraryIndirect.py

5967cbc

remove {}

3c4c00b

kastiglione marked this pull request as ready for review December 10, 2020 20:32

another fix to TestLibraryIndirect.py

a76bd63

adrian-prantl approved these changes Dec 11, 2020

View reviewed changes

kastiglione mentioned this pull request Dec 11, 2020

[lldb] Add ReferenceTypeInfo support to SwiftLanguageRuntimeImpl::GetNumChildren #2224

Merged

kastiglione merged commit a72a24c into swift/main Dec 11, 2020

kastiglione deleted the lldb-Add-ReferenceTypeInfo-support-to-SwiftLanguageRuntimeImpl-GetNumChildren branch December 11, 2020 01:30

		GetTypeInfo(CompilerType type, ExecutionContextScope *exe_scope,
		swift::reflection::TypeRef const **out_tr = nullptr);

[lldb] Add ReferenceTypeInfo support to SwiftLanguageRuntimeImpl::GetNumChildren #2218

[lldb] Add ReferenceTypeInfo support to SwiftLanguageRuntimeImpl::GetNumChildren #2218

Uh oh!

Conversation

kastiglione commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kastiglione commented Dec 10, 2020

Uh oh!

kastiglione commented Dec 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kastiglione commented Dec 10, 2020

Uh oh!

adrian-prantl commented Dec 10, 2020

Uh oh!

kastiglione commented Dec 10, 2020

Uh oh!

kastiglione commented Dec 10, 2020

Uh oh!

kastiglione commented Dec 10, 2020

Uh oh!

kastiglione commented Dec 10, 2020

Uh oh!

kastiglione commented Dec 10, 2020

Uh oh!

kastiglione commented Dec 10, 2020

Uh oh!

adrian-prantl left a comment

Choose a reason for hiding this comment

Uh oh!

kastiglione commented Dec 11, 2020

Uh oh!

Uh oh!

kastiglione commented Dec 10, 2020 •

edited

Loading