[lldb] Wire-up TypeSystemSwiftTypeRef::GetNumChildren #2200

kastiglione · 2020-12-08T01:37:57Z

Make TypeSystemSwiftTypeRef::GetNumChildren use reflection via SwiftLanguageRuntimeImpl instead of using SwiftASTContext.

This required changes to SwiftLanguageRuntimeImpl::GetNumChildren, to handle EnumTypeInfo and OpaqueExistential.

Other notes:

The Equivalent helper was tweaked to allow GetNumChildren to use >= when validating. This is because TypeRefs can, in some cases, expose more children than SwiftASTContext (for example when using library evolution and .swiftinterface files)
Function types are hard code to zero to match SwiftASTContext (and makes sense). TypeRefs for function types can contain a non-zero number of children.

rdar://68171335

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp

kastiglione · 2020-12-08T01:46:07Z

EDIT: Resolved by 79f7869.

When running the foundation_value_types tests, the validation asserts on TestSwiftFoundationTypeNotification:

https://github.com/apple/llvm-project/blob/1f4717b8bb8999ac9ff50e68583009bd7e00caaf/lldb/test/API/lang/swift/foundation_value_types/notification/main.swift#L22

The issue is the userInfo dictionary is optional. Its type info is an instance of SinglePayloadEnumTypeInfo (a subtype of EnumTypeInfo), which is not yet supported by SwiftLanguageRuntimeImpl::GetNumChildren. I'll implement that next.

kastiglione · 2020-12-08T07:43:21Z

@swift-ci test

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp

kastiglione · 2020-12-08T18:04:41Z

TODO:

~~Support clang imported types~~
Fallback to SwiftASTContext for unsupported cases

kastiglione · 2020-12-08T19:49:06Z

With the fallback to SwiftASTContext (3331e29), the following GetNumChildren validation assertions are logged during a full test run:

1. Possible Bugs in SwiftASTContext

It seems that for these types, TypeRefs provide the correct value, and SwiftASTContext is not. All I've done is look at the type definitions in the source and seen that they do have two (2) children, which matches the TypeRef value.

TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 2
r = 1
failing type was $s1a18SwiftStructCMemberVD
--
TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 2
r = 1
failing type was $s11SomeLibrary15ContainsTwoIntsVD
--
TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 2
r = 1
failing type was $s2AA7MyPointVD

2. Function Types "false positive"

These cases are function types. TypeRefs say there are children, while SwiftASTContext says there are zero children. The latter seems right to me, so I'll add a case to handle function types.

TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 2
r = 0
failing type was $sSSSiSbIggyl_D
--
TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 2
r = 0
failing type was $sSiycD

3. Protocol Type implementation differences

I don't have any insight to these. Both values seem weird to me. For example sypD is Any, and my intuition is that statically it should have neither 1 child nor 4, unless there's some runtime dynamic type determination happening, even though the logging reports the static type.

TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 1
r = 4
failing type was $sypD
--
TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 2
r = 5
failing type was $s1a10PointUtils_pD

4. TBD

TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 0
r = 1
failing type was $s10Foundation3URLVD
--
TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 0
r = 1
failing type was $s4main1CCD

kastiglione · 2020-12-08T19:50:24Z

@swift-ci test

kastiglione · 2020-12-08T19:50:37Z

Running tests to see if CI has different results from my local run.

adrian-prantl · 2020-12-08T20:40:48Z

Great summary!

With the fallback to SwiftASTContext (3331e29), the following GetNumChildren validation assertions are logged during a full test run:

1. Possible Bugs in SwiftASTContext

It seems that for these types, TypeRefs provide the correct value, and SwiftASTContext is not. All I've done is look at the type definitions in the source and seen that they do have two (2) children, which matches the TypeRef value.
TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 2
r = 1
failing type was $s1a18SwiftStructCMemberVD
--
TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 2
r = 1
failing type was $s11SomeLibrary15ContainsTwoIntsVD
--
TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 2
r = 1
failing type was $s2AA7MyPointVD

That looks great. In previous cases, I've amended the Equivalent function to accept lhs >= rhs (more=better). We could do the same thing here. Does it make sense why SwiftASTContext cannot see the extra children?

2. Function Types "false positive"

These cases are function types. TypeRefs say there are children, while SwiftASTContext says there are zero children. The latter seems right to me, so I'll add a case to handle function types.
TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 2
r = 0
failing type was $sSSSiSbIggyl_D
--
TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 2
r = 0
failing type was $sSiycD

what are the "children" of a function type? I'm curious what path we go down that reports children and if that is indicative of a bug. If that code looks reasonable, then special-casing function types sounds right.

3. Protocol Type Bugs

I don't have any insight to these. Both values seem weird to me. For example sypD is Any, and my intuition is that statically it should have neither 1 child nor 4, unless there's some runtime dynamic type determination happening, even though the logging reports the static type.
TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 1
r = 4
failing type was $sypD
--
TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 2
r = 5
failing type was $s1a10PointUtils_pD

I agree, these are both suspicious. We could ignore them in the comparison and file a bug to investigate later.

4. TBD

TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 0
r = 1
failing type was $s10Foundation3URLVD

This one is scary, because it is a Foundation value type! Is that just an unimplemented case, or do we have a deeper problem?

--
TypeSystemSwiftTypeRef diverges from SwiftASTContext:
l = 0
r = 1
failing type was $s4main1CCD

kastiglione · 2020-12-08T21:52:04Z

This one is scary, because it is a Foundation value type! Is that just an unimplemented case, or do we have a deeper problem?

Since there was only the one failure, I was optimistic it wouldn't be too much of any issue. The problem was specific to target variable (for a global). In that case, SwiftLanguageRuntime::Get(exe_scope->CalculateProcess()) was returning null. I suspect that might be a bug, which I can look at later, in the mean time I moved the fallback so that it handles this case too (
9e41188).

kastiglione · 2020-12-08T21:52:29Z

@swift-ci test

adrian-prantl · 2020-12-08T22:46:48Z

This one is scary, because it is a Foundation value type! Is that just an unimplemented case, or do we have a deeper problem?

Since there was only the one failure, I was optimistic it wouldn't be too much of any issue. The problem was specific to target variable (for a global). In that case, SwiftLanguageRuntime::Get(exe_scope->CalculateProcess()) was returning null. I suspect that might be a bug, which I can look at later, in the mean time I moved the fallback so that it handles this case too (
9e41188).

That makes sense. Without a process, we don't have a language runtime. In GetChildCompilerTypeAtIndex() I partially worked around this by still resolving Clang types statically, but in this particular case that wouldn't work. I think we have to drop support for target variable-ing Foundation value types without a process. I.e., change the test to run to a breakpoint first.

kastiglione · 2020-12-08T23:40:54Z

That looks great. In previous cases, I've amended the Equivalent function to accept lhs >= rhs (more=better). We could do the same thing here. Does it make sense why SwiftASTContext cannot see the extra children?

Ah it turns out one of the cases is quite relevant. One case of these assertions is coming from the parseable_interfaces tests. SwiftASTContext determines only one child, while TypeRef exposes two children. The type does has two children, but one is private. That would explain why SwiftASTContext thinks there's only one child. So this assertion indicates an improvement.

adrian-prantl · 2020-12-09T00:42:33Z

Awesome!

kastiglione · 2020-12-09T01:00:36Z

For protocol types, the difference in child count is 3 in each case (5 vs 2, 4 vs 1). Looking at one test that asserts, the raw output for the protocol includes payload, metadata, and witness table.

Example:

self.expect("frame variable --raw-output --show-types loc2d",
            substrs=['PointUtils) loc2d =',
                     '(Builtin.RawPointer) payload_data_0 = 0x',
                     '(Builtin.RawPointer) payload_data_1 = 0x',
                     '(Builtin.RawPointer) payload_data_2 = 0x',
                     '(Any.Type) metadata = 0x',
                     '(Builtin.RawPointer) wtable = 0x'])

For TypeRefs, the fields don't include the 3 payload data fields, only metadata and wtable.

Seems then the way to proceed to is to stick with current behavior? Have GetNumChildren add 3 to the count for protocols?

@adrian-prantl I can check, but do you know off the top of your head how GetChildCompilerTypeAtIndex() handles the payload_data fields of protocols?

adrian-prantl · 2020-12-09T01:07:04Z

If you look at the code, these (payload_data,...) are actually hardcoded in LLDB and their usefulness is questionable if you look at the dataformatters.

kastiglione · 2020-12-09T01:16:24Z

If you look at the code

🤦

Yeah, they're not that used. I'll keep compatibility, we can visit their existence later.

kastiglione · 2020-12-09T02:37:45Z

@swift-ci test

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp

kastiglione · 2020-12-09T06:38:30Z

@swift-ci test

kastiglione · 2020-12-09T07:20:45Z

lldb/test/API/lang/swift/implementation_only_imports/library_resilient/TestLibraryResilient.py

@@ -82,7 +82,7 @@ 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 = (other = 10)",
+            "(SomeLibrary.ContainsTwoInts) container = {", "other = 10",


This is perhaps indicative of issues that need to be solved in this PR, or in follow ups.

ContainsTwoInts is defined as:

public struct ContainsTwoInts { internal var wrapped: TwoInts public var other: Int }

This was previously formatted as container = (other = 10) but with these changes it's formatted as:

container = { other = 10 }

The same data is printed, but the difference is (presumably) that ShouldPrintAsOneLiner is no longer true. Note that despite there being two children in the type, lldb prints only one, the public property other.

Based on my debugging of other discrepancies, my guess is that GetNumChildren now returns 2 instead of 1, which then causes ShouldPrintAsOneLiner to return false.

What I think should be possible now, is for lldb to print both properties, wrapped and other. I'll take a look tomorrow to see if the TypeRef implementation of GetChildCompilerTypeAtIndex is excluding wrapped to mirror the behavior of SwiftASTContext. If that is the case, it seems like something we should fix, showing private properties too would be an improvement.

Sounds great! Thanks for the thoughtful analysis.

kastiglione · 2020-12-09T15:58:23Z

All tests pass, ready for review.

adrian-prantl · 2020-12-09T16:47:07Z

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp

 uint32_t
 TypeSystemSwiftTypeRef::GetNumChildren(opaque_compiler_type_t type,
                                       bool omit_empty_base_classes,
                                       const ExecutionContext *exe_ctx) {
-  return m_swift_ast_context->GetNumChildren(ReconstructType(type),
-                                             omit_empty_base_classes, exe_ctx);
+  if (IsFunctionType(type, nullptr))


Just for my curiosity: What are the children of a function type that the runtime reports?

I meant to find that out too, I'll reply with the answer.

My guess is implicit self, and/or a capture context.

The fields are function, whose mangled symbol is a ThinFunctionType, and context whose mangled symbol is Builtin.NativeObject. The type info is a RecordKind::ThickFunction.

Since RecordKind::OpaqueExistential is handled in SwiftLanguageRuntimeImpl, I'm thinking it's reasonable and maybe good to handle RecordKind::ThickFunction there too.

Moved function handling: a5d4945

kastiglione · 2020-12-09T17:28:11Z

@swift-ci test

[lldb] Wire-up TypeSystemSwiftTypeRef::GetNumChildren

b82ad6d

kastiglione marked this pull request as draft December 8, 2020 01:38

kastiglione commented Dec 8, 2020

View reviewed changes

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp Outdated Show resolved Hide resolved

add EnumTypeInfo handling in GetNumChildren

79f7869

kastiglione requested a review from adrian-prantl December 8, 2020 03:36

adrian-prantl reviewed Dec 8, 2020

View reviewed changes

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp Outdated Show resolved Hide resolved

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp Outdated Show resolved Hide resolved

fallback to SwiftASTContext for unsupported cases

3331e29

use fallback for any failed data dependency

9e41188

kastiglione added 5 commits December 8, 2020 18:28

handle OpaqueExistential's three payload fields

e2381ff

tweak comment

58a76c3

call VALIDATE_AND_RETURN only when not using SwiftASTContext

6449eb1

hardcode function types to zero children

92d02fe

change Equivalent<uint32_t> to assert >=

3b030b8

adrian-prantl reviewed Dec 9, 2020

View reviewed changes

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp Outdated Show resolved Hide resolved

kastiglione added 3 commits December 8, 2020 22:32

log GetNumChildren fallback

9af8222

use Equivalent(Optional<T>, Optional<T>)

1cb9b37

tweak substrs in TestLibraryResilient.py

dc3fcf9

kastiglione marked this pull request as ready for review December 9, 2020 06:40

kastiglione commented Dec 9, 2020

View reviewed changes

adrian-prantl reviewed Dec 9, 2020

View reviewed changes

adrian-prantl approved these changes Dec 9, 2020

View reviewed changes

move function type handling into SwiftLanguageRuntimeImpl

a5d4945

kastiglione mentioned this pull request Dec 9, 2020

[lldb] Wire-up TypeSystemSwiftTypeRef::GetNumChildren #2215

Merged

kastiglione merged commit 5db02e5 into swift/main Dec 9, 2020

kastiglione deleted the lldb-Wire-up-TypeSystemSwiftTypeRef-GetNumChildren branch December 9, 2020 21:50

kastiglione mentioned this pull request Dec 10, 2020

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

Merged

[lldb] Wire-up TypeSystemSwiftTypeRef::GetNumChildren #2200

[lldb] Wire-up TypeSystemSwiftTypeRef::GetNumChildren #2200

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

kastiglione commented Dec 8, 2020

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

1. Possible Bugs in SwiftASTContext

2. Function Types "false positive"

3. Protocol Type implementation differences

4. TBD

Uh oh!

kastiglione commented Dec 8, 2020

Uh oh!

kastiglione commented Dec 8, 2020

Uh oh!

adrian-prantl commented Dec 8, 2020

1. Possible Bugs in SwiftASTContext

2. Function Types "false positive"

3. Protocol Type Bugs

4. TBD

Uh oh!

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

Uh oh!

Uh oh!

kastiglione commented Dec 8, 2020

Uh oh!

adrian-prantl commented Dec 8, 2020

Uh oh!

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

Uh oh!

Uh oh!

adrian-prantl commented Dec 9, 2020

Uh oh!

kastiglione commented Dec 9, 2020

Uh oh!

adrian-prantl commented Dec 9, 2020

Uh oh!

kastiglione commented Dec 9, 2020

Uh oh!

kastiglione commented Dec 9, 2020

Uh oh!

Uh oh!

kastiglione commented Dec 9, 2020

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrian-prantl Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

kastiglione commented Dec 9, 2020

Uh oh!

adrian-prantl Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

kastiglione Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

kastiglione Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

kastiglione Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

kastiglione Dec 9, 2020

kastiglione commented Dec 8, 2020 •

edited

Loading

kastiglione commented Dec 8, 2020 •

edited

Loading

kastiglione commented Dec 8, 2020 •

edited

Loading

kastiglione commented Dec 8, 2020 •

edited

Loading

kastiglione commented Dec 8, 2020 •

edited

Loading

kastiglione commented Dec 8, 2020 •

edited

Loading

kastiglione Dec 9, 2020 •

edited

Loading