Skip to content

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

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.

Sign up for GitHub

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 6 commits into from
Dec 11, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
af7c46a
[lldb] Add ReferenceTypeInfo support to SwiftLanguageRuntimeImpl::Get…
kastiglione Dec 10, 2020
da1254e
fix early exit condition
kastiglione Dec 10, 2020
3ff1e49
Use TypeRefBuilder::lookupSuperclass
kastiglione Dec 10, 2020
5967cbc
fix TestLibraryIndirect.py
kastiglione Dec 10, 2020
3c4c00b
remove {}
kastiglione Dec 10, 2020
a76bd63
another fix to TestLibraryIndirect.py
kastiglione Dec 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 75 additions & 36 deletions lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -997,6 +997,38 @@ llvm::Optional<uint64_t> SwiftLanguageRuntimeImpl::GetMemberVariableOffset(
return offset;
}

static CompilerType GetWeakReferent(TypeSystemSwiftTypeRef &ts,
Copy link
Author

Choose a reason for hiding this comment

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

this whole function was moved up and nothing more.

CompilerType type) {
using namespace swift::Demangle;
Demangler dem;
auto mangled = type.GetMangledTypeName().GetStringRef();
NodePointer n = dem.demangleSymbol(mangled);
if (!n || n->getKind() != Node::Kind::Global || !n->hasChildren())
return {};
n = n->getFirstChild();
if (!n || n->getKind() != Node::Kind::TypeMangling || !n->hasChildren())
return {};
n = n->getFirstChild();
if (!n || n->getKind() != Node::Kind::Type || !n->hasChildren())
return {};
n = n->getFirstChild();
if (!n ||
(n->getKind() != Node::Kind::Weak &&
n->getKind() != Node::Kind::Unowned &&
n->getKind() != Node::Kind::Unmanaged) ||
!n->hasChildren())
return {};
n = n->getFirstChild();
if (!n || n->getKind() != Node::Kind::Type || !n->hasChildren())
return {};
// FIXME: We only need to canonicalize this node, not the entire type.
n = ts.CanonicalizeSugar(dem, n->getFirstChild());
if (!n || n->getKind() != Node::Kind::SugaredOptional || !n->hasChildren())
return {};
n = n->getFirstChild();
return ts.RemangleAsType(dem, n);
}

llvm::Optional<unsigned>
SwiftLanguageRuntimeImpl::GetNumChildren(CompilerType type,
ValueObject *valobj) {
Expand All @@ -1008,7 +1040,8 @@ SwiftLanguageRuntimeImpl::GetNumChildren(CompilerType type,
// Try the static type metadata.
auto frame =
valobj ? valobj->GetExecutionContextRef().GetFrameSP().get() : nullptr;
auto *ti = GetTypeInfo(type, frame);
const swift::reflection::TypeRef *tr = nullptr;
auto *ti = GetTypeInfo(type, frame, &tr);
// Structs and Tuples.
if (auto *rti =
llvm::dyn_cast_or_null<swift::reflection::RecordTypeInfo>(ti)) {
Expand All @@ -1030,6 +1063,41 @@ SwiftLanguageRuntimeImpl::GetNumChildren(CompilerType type,
if (auto *eti = llvm::dyn_cast_or_null<swift::reflection::EnumTypeInfo>(ti)) {
return eti->getNumPayloadCases();
}
// Objects.
if (auto *rti =
llvm::dyn_cast_or_null<swift::reflection::ReferenceTypeInfo>(ti)) {

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:
break;
}

if (!tr)
return {};

auto *reflection_ctx = GetReflectionContext();
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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...

Copy link
Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Sounds good.

if (auto *rti =
llvm::dyn_cast_or_null<swift::reflection::RecordTypeInfo>(cti)) {
// The superclass, if any, is an extra child.
if (builder.lookupSuperclass(tr))
return rti->getNumFields() + 1;
return rti->getNumFields();
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Currently it's only ever a RecordTypeInfo:

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

}

return {};
}
// FIXME: Implement more cases.
return {};
}
Expand Down Expand Up @@ -1064,38 +1132,6 @@ static llvm::StringRef GetBaseName(swift::Demangle::NodePointer node) {
}
}

static CompilerType GetWeakReferent(TypeSystemSwiftTypeRef &ts,
CompilerType type) {
using namespace swift::Demangle;
Demangler dem;
auto mangled = type.GetMangledTypeName().GetStringRef();
NodePointer n = dem.demangleSymbol(mangled);
if (!n || n->getKind() != Node::Kind::Global || !n->hasChildren())
return {};
n = n->getFirstChild();
if (!n || n->getKind() != Node::Kind::TypeMangling || !n->hasChildren())
return {};
n = n->getFirstChild();
if (!n || n->getKind() != Node::Kind::Type || !n->hasChildren())
return {};
n = n->getFirstChild();
if (!n ||
(n->getKind() != Node::Kind::Weak &&
n->getKind() != Node::Kind::Unowned &&
n->getKind() != Node::Kind::Unmanaged) ||
!n->hasChildren())
return {};
n = n->getFirstChild();
if (!n || n->getKind() != Node::Kind::Type || !n->hasChildren())
return {};
// FIXME: We only need to canonicalize this node, not the entire type.
n = ts.CanonicalizeSugar(dem, n->getFirstChild());
if (!n || n->getKind() != Node::Kind::SugaredOptional || !n->hasChildren())
return {};
n = n->getFirstChild();
return ts.RemangleAsType(dem, n);
}

static CompilerType
GetTypeFromTypeRef(TypeSystemSwiftTypeRef &ts,
const swift::reflection::TypeRef *type_ref) {
Expand Down Expand Up @@ -2510,9 +2546,9 @@ SwiftLanguageRuntimeImpl::GetTypeRef(CompilerType type,
return type_ref;
}

const swift::reflection::TypeInfo *
SwiftLanguageRuntimeImpl::GetTypeInfo(CompilerType type,
ExecutionContextScope *exe_scope) {
const swift::reflection::TypeInfo *SwiftLanguageRuntimeImpl::GetTypeInfo(
CompilerType type, ExecutionContextScope *exe_scope,
swift::reflection::TypeRef const **out_tr) {
auto *ts = llvm::dyn_cast_or_null<TypeSystemSwift>(type.GetTypeSystem());
if (!ts)
return nullptr;
Expand Down Expand Up @@ -2543,6 +2579,9 @@ SwiftLanguageRuntimeImpl::GetTypeInfo(CompilerType type,
if (!type_ref)
return nullptr;

if (out_tr)
*out_tr = type_ref;

auto *reflection_ctx = GetReflectionContext();
if (!reflection_ctx)
return nullptr;
Expand Down
3 changes: 2 additions & 1 deletion lldb/source/Target/SwiftLanguageRuntimeImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ class SwiftLanguageRuntimeImpl {

/// Ask Remote Mirrors for the type info about a Swift type.
const swift::reflection::TypeInfo *
GetTypeInfo(CompilerType type, ExecutionContextScope *exe_scope);
GetTypeInfo(CompilerType type, ExecutionContextScope *exe_scope,
swift::reflection::TypeRef const **out_tr = nullptr);
Comment on lines +81 to +82
Copy link
Author

Choose a reason for hiding this comment

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

Added an out parameter to return the typeref.

Copy link

Choose a reason for hiding this comment

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

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.


llvm::Optional<const swift::reflection::TypeInfo *>
lookupClangTypeInfo(CompilerType clang_type);
Expand Down
7 changes: 4 additions & 3 deletions lldb/test/API/lang/swift/implementation_only_imports/library_indirect/TestLibraryIndirect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,10 @@ def cleanup():
# This test is deliberately checking what the user will see, rather than
# the structure provided by the Python API, in order to test the recovery.
self.expect("fr var", substrs=[
"(SomeLibrary.ContainsTwoInts) container = (wrapped = 0x",
", other = 10)",
"(SomeLibrary.ContainsTwoInts) container = {",
"wrapped = 0x",
"other = 10",
"(Int) simple = 1"])
self.expect("e container", substrs=["(SomeLibrary.ContainsTwoInts)", "(wrapped = 0x", ", other = 10"])
self.expect("e container", substrs=["(SomeLibrary.ContainsTwoInts)", "wrapped = 0x", "other = 10"])
self.expect("e container.wrapped", substrs=["(SomeLibrary.BoxedTwoInts)", "0x", "{}"])
self.expect("e container.wrapped.value", error=True, substrs=["value of type 'BoxedTwoInts' has no member 'value'"])