Add a callback to swift::reflection::MemoryReader that allows LLDB to… #33417

adrian-prantl · 2020-08-12T00:45:39Z

Add a callback to swift::reflection::MemoryReader that allows LLDB to provide

swift::reflection::TypeInfo for (Clang-)imported non-Objective-C types. This is
needed to reflect on the size mixed Swift / Clang types.

adrian-prantl · 2020-08-12T00:48:06Z

@augusto2112 For some reason github did not let me request a review from you.

adrian-prantl · 2020-08-12T00:53:53Z

Some more context: I'm working on a TypeRef-based Swift context for LLDB. Because we do not have reflection metadata for pure C types in Swift, reflection cannot compute TypeInfo for NominalTypeRefs for those types. By providing this callback, LLDB can supply this information for DWARF, and reflection can compute TypeInfos for mixed Swift/C types.

slavapestov · 2020-08-12T16:11:51Z

For pure C types that are referenced from code, we emit BuiltinTypeDescriptors in IRGen. I'm guessing here you need the callback because expression evaluation can pull in arbitrary C types that are not referenced from code? If so this approach sounds great.

I have reservations about adding this to MemoryReader since it's not really about reading memory. Maybe we need a second callback interface, or would that be too awkward to plumb through?

slavapestov · 2020-08-12T16:12:31Z

include/swift/Reflection/ReflectionContext.h

@@ -623,7 +623,7 @@ class ReflectionContext

        // Perform layout
        if (start)
-          TI = TC.getClassInstanceTypeInfo(TR, *start);
+          TI = TC.getClassInstanceTypeInfo(TR, *start, getReader());


This is why I think a different callback would be nice. In theory it should be possible to look up TypeInfos without a remote process attached; and you could still have a callback to get C type info.

slavapestov · 2020-08-12T16:13:18Z

stdlib/public/Reflection/TypeLowering.cpp

  if (ThickFunctionTI != nullptr)
    return ThickFunctionTI;

  RecordTypeInfoBuilder builder(*this, RecordKind::ThickFunction);
-  builder.addField("function", getThinFunctionTypeRef());
-  builder.addField("context", getNativeObjectTypeRef());
+  builder.addField("function", getThinFunctionTypeRef(), reader);


This shouldn't need a reader. A thick function is just a pair of pointers as far as RemoteMirrors are concerned.

slavapestov · 2020-08-12T16:13:49Z

stdlib/public/Reflection/TypeLowering.cpp

@@ -1176,12 +1180,12 @@ class ExistentialTypeInfoBuilder {
      // Class existentials consist of a single retainable pointer
      // followed by witness tables.
      if (Refcounting == ReferenceCounting::Unknown)
-        builder.addField("object", TC.getUnknownObjectTypeRef());
+        builder.addField("object", TC.getUnknownObjectTypeRef(), reader);


None of these should need to read imported TypeInfo either

slavapestov · 2020-08-12T16:14:13Z

stdlib/public/Reflection/TypeLowering.cpp

+            // to instantiate a dummy reader here. This helps preserving the
+            // layering.
+            remote::MemoryReader dummyReader;
+            auto *SuperclassTI = TC.getTypeInfo(Superclass, dummyReader);


Could the reader parameter to getTypeInfo() be a pointer instead, with handing of the null case?

Maybe llvm:Optional to make it clear the reader is... well... optional?

I believe llvm::Optional also has the benefit of being able to keep queryDataLayout, getSymbolAddress and readString deleted in the base class.

augusto2112 · 2020-08-12T16:19:31Z

@augusto2112 For some reason github did not let me request a review from you.

Probably because I'm not a project member (that's why I have to @ people when I want a review).

adrian-prantl · 2020-08-12T17:27:52Z

For pure C types that are referenced from code, we emit BuiltinTypeDescriptors in IRGen. I'm guessing here you need the callback because expression evaluation can pull in arbitrary C types that are not referenced from code? If so this approach sounds great.

Interesting! The case I primarily debugged this with was using a type referenced in the code, but perhaps the example program was too simple to trigger emission of the metadata. I'll look into that — but it also seems like this could be useful even if that were a bug because of the pulling-extra-types-in scenario you mentioned.

I have reservations about adding this to MemoryReader since it's not really about reading memory. Maybe we need a second callback interface, or would that be too awkward to plumb through?

That sounds totally reasonable, I'll take a look at how well that fit.

adrian-prantl · 2020-08-12T18:56:04Z

I inspected the LLVM IR generated for my testcase and there is no mention of the imported C type outside of debug info. When adding a print() of a variable of that type to the program, this changes. From that I'm concluding that the compiler will only emit the type metadata when there is an actual dependency on it. So it looks like I'm going to need this mechanism regardless.

slavapestov · 2020-08-12T19:35:56Z

I see, you're right. The compiler only emits the builtin descriptor when the type appears as the field type of another type, because the original use case for RemoteMirrors was to enable object graph debugging. It's reasonable to pull in additional type info for the types of local variables and such using the callback.

adrian-prantl · 2020-08-12T23:00:23Z

I have now created a new swift::remote::TypeInfoProvider interface that is to be passed in on a per-function basis rather than owned by the TypeConverter. This actually helps removing some ugly statefulness in the LLDB patch using this.

… provide swift::reflection::TypeInfo for (Clang-)imported non-Objective-C types. This is needed to reflect on the size mixed Swift / Clang types, when no type metadata is available for the C types. This is a necessary ingredient for the TypeRef-based Swift context in LLDB. Because we do not have reflection metadata for pure C types in Swift, reflection cannot compute TypeInfo for NominalTypeRefs for those types. By providing this callback, LLDB can supply this information for DWARF, and reflection can compute TypeInfos for mixed Swift/C types.

adrian-prantl · 2020-08-28T16:42:22Z

@slavapestov The LLDB side of this is ready now. Can you check if I addressed all your concerns?

slavapestov

I only have one last suggestion. I think we should stick the TypeInfoProvider inside the TypeConverter, so that it doesn't have to be passed around. What do you think?

adrian-prantl · 2020-09-02T17:51:53Z

I think we should stick the TypeInfoProvider inside the TypeConverter, so that it doesn't have to be passed around.

My understanding is that swift::reflection::ReflectionContext owns exactly one Builder, which owns exactly one TypeConverter. LLDB, however, has one TypeInfoProvider per lldb::Module (≈ dylib) to localize type lookup, and one ReflectionContext per process. That's why it's passed in explicitly.

slavapestov · 2020-09-03T01:55:24Z

I didn't realize we had one TypeInfoProvider per Module. In that case, I think it's fine to do what you're doing now (but we could also investigate having one TypeConverter per module or something?).

shahmishal · 2020-09-03T01:59:45Z

@adrian-prantl This PR need to be updated to target master branch form master-rebranch.

adrian-prantl · 2020-09-03T02:40:21Z

@swift-ci test

adrian-prantl · 2020-09-03T02:41:07Z

@swift-ci test

swift-ci · 2020-09-03T03:44:28Z

Build failed
Swift Test Linux Platform
Git Sha - 56b2e90

swift-ci · 2020-09-03T04:22:06Z

Build failed
Swift Test OS X Platform
Git Sha - 56b2e90

adrian-prantl · 2020-09-03T15:43:55Z

@swift-ci smoke test

adrian-prantl · 2020-09-03T21:59:57Z

@swift-ci smoke test

adrian-prantl · 2020-09-03T22:29:29Z

@swift-ci smoke test

adrian-prantl · 2020-09-04T19:18:46Z

test with swiftlang/llvm-project#1636
@swift-ci test

swift-ci · 2020-09-04T20:20:05Z

Build failed
Swift Test Linux Platform
Git Sha - 56b2e90

swift-ci · 2020-09-04T21:36:37Z

Build failed
Swift Test OS X Platform
Git Sha - 56b2e90

adrian-prantl · 2020-09-04T23:01:45Z

test with swiftlang/llvm-project#1636
@swift-ci smoke test

adrian-prantl · 2020-09-04T23:02:31Z

test with swiftlang/llvm-project#1636
@swift-ci test

swift-ci · 2020-09-04T23:04:51Z

Build failed
Swift Test OS X Platform
Git Sha - 56b2e90

swift-ci · 2020-09-04T23:57:32Z

Build failed
Swift Test Linux Platform
Git Sha - 56b2e90

adrian-prantl · 2020-09-05T00:01:34Z

test with swiftlang/llvm-project#1636
@swift-ci smoke test

adrian-prantl · 2020-09-05T01:31:00Z

test with swiftlang/llvm-project#1636
@swift-ci test

swift-ci · 2020-09-05T03:23:54Z

Build failed
Swift Test Linux Platform
Git Sha - 56b2e90

adrian-prantl · 2020-09-06T16:37:54Z

test with swiftlang/llvm-project#1636
@swift-ci test

adrian-prantl · 2020-09-06T16:41:08Z

test with swiftlang/llvm-project#1636
@swift-ci smoke test

compnerd · 2020-09-09T00:54:13Z

@adrian-prantl this seems to have caused a regression on the Windows builds :-(

adrian-prantl mentioned this pull request Aug 12, 2020

Implemement TypeSystemSwiftTypeRef::GetBitSize() (NFC) swiftlang/llvm-project#1636

Merged

adrian-prantl requested review from jckarter and slavapestov August 12, 2020 00:46

slavapestov reviewed Aug 12, 2020

View reviewed changes

adrian-prantl force-pushed the 55412920 branch from 2bbc71e to 54c8104 Compare August 12, 2020 22:58

adrian-prantl force-pushed the 55412920 branch 3 times, most recently from 757c094 to 203923e Compare August 20, 2020 22:05

adrian-prantl added 5 commits August 25, 2020 15:19

Thread Labels through TupleTypeRef.

961dc59

Support generic parent contexts in DemanglingForTypeRef.

9287707

Take TypeInfoProvider into account when caching TypeInfos.

9b5b265

Support MetatypeTypeRefs in DemanglingForTypeRef.

56b2e90

adrian-prantl force-pushed the 55412920 branch from b535050 to 56b2e90 Compare August 25, 2020 22:45

slavapestov reviewed Sep 1, 2020

View reviewed changes

shahmishal closed this Sep 3, 2020

shahmishal reopened this Sep 3, 2020

adrian-prantl changed the base branch from master-rebranch to master September 3, 2020 02:40

adrian-prantl merged commit 4247b60 into swiftlang:master Sep 8, 2020

Add a callback to swift::reflection::MemoryReader that allows LLDB to… #33417

Add a callback to swift::reflection::MemoryReader that allows LLDB to… #33417

Uh oh!

Conversation

adrian-prantl commented Aug 12, 2020

Uh oh!

adrian-prantl commented Aug 12, 2020

Uh oh!

adrian-prantl commented Aug 12, 2020

Uh oh!

slavapestov commented Aug 12, 2020

Uh oh!

slavapestov Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

slavapestov Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

slavapestov Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

slavapestov Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

augusto2112 Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

augusto2112 Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

augusto2112 commented Aug 12, 2020

Uh oh!

adrian-prantl commented Aug 12, 2020

Uh oh!

adrian-prantl commented Aug 12, 2020

Uh oh!

slavapestov commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrian-prantl commented Aug 12, 2020

Uh oh!

adrian-prantl commented Aug 28, 2020

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

adrian-prantl commented Sep 2, 2020

Uh oh!

slavapestov commented Sep 3, 2020

Uh oh!

shahmishal commented Sep 3, 2020

Uh oh!

adrian-prantl commented Sep 3, 2020

Uh oh!

adrian-prantl commented Sep 3, 2020

Uh oh!

swift-ci commented Sep 3, 2020

Uh oh!

swift-ci commented Sep 3, 2020

Uh oh!

adrian-prantl commented Sep 3, 2020

Uh oh!

adrian-prantl commented Sep 3, 2020

Uh oh!

adrian-prantl commented Sep 3, 2020

Uh oh!

adrian-prantl commented Sep 4, 2020

Uh oh!

swift-ci commented Sep 4, 2020

Uh oh!

swift-ci commented Sep 4, 2020

Uh oh!

adrian-prantl commented Sep 4, 2020

Uh oh!

adrian-prantl commented Sep 4, 2020

Uh oh!

swift-ci commented Sep 4, 2020

Uh oh!

swift-ci commented Sep 4, 2020

Uh oh!

slavapestov commented Aug 12, 2020 •

edited

Loading