[lldb] Implement TypeSystemSwiftTypeRef::GetIndexOfChildMemberWithName #2260

kastiglione · 2020-12-17T03:27:18Z

This ports the implementation of GetIndexOfChildMemberWithName, from SwiftASTContext to TypeSystemSwiftTypeRef.

With this change, when compiling with swiftinterfaces, private properties in Swift types are now visible to lldb through the TypeRef metadata contained in the binary. Tests that expected private properties to not be accessible have been updated.

rdar://68171371

kastiglione · 2020-12-17T03:33:39Z

@adrian-prantl this doesn't need a review yet, but can you let me know what you think about the plumbing changes added in
e5300ff and e80330f?

e5300ff: Adds an ExecutionContext * parameter to all forms of GetIndexOfChildMemberWithName
e80330f: Adds to SwiftLanguageRuntime a stub implementation of GetIndexOfChildMemberWithName, and calls that from TypeSystemSwiftTypeRef (including some manually handled validation)

In summary, all this is setup to implement and use SwiftLanguageRuntimeImpl::GetIndexOfChildMemberWithName.

If this is all fine, a follow up commit will add commits to implement the behavior.

adrian-prantl · 2020-12-17T17:03:48Z

lldb/include/lldb/Symbol/CompilerType.h

@@ -324,7 +324,7 @@ class CompilerType {

  /// Lookup a child given a name. This function will match base class names and
  /// member member names in "clang_type" only, not descendants.
-  uint32_t GetIndexOfChildWithName(const char *name,
+  uint32_t GetIndexOfChildWithName(const char *name, ExecutionContext *exe_ctx,


We should eventually upstream this change, but I don't see an issue with doing so. @jimingham previously pointed out that requiring a ValueObject in CompilerType is problematic, but there is prior art for requiring an optional ExecutionContext in calls like GetBitSize(). @jimingham do you agree with that characterization?

for what it's worth, the functions I've seen need the process, so passing a process instead of ExecutionContext makes the dependency clearer, at the cost of being less flexible if in the future something else from the context is needed instead of or in addition to the process.

Since this is an API for all TypeSystems, maybe keeping it more general is actually desirable? It also looks like most CompilerType methods (https://lldb.llvm.org/cpp_reference/classlldb__private_1_1CompilerType.html) take an ExecutionContextScope. Maybe we should also do that here too, for symmetry?

The use cases and distinction between ExecutionContext and ExecutionContextScope is not clear to me at all. I read their docs and looked at their APIs. The primary reason I went with ExecutionContext here is because from ValueObject it's more direct/convenient.

Within ValueObject, both of these patterns exist:

ExecutionContext exe_ctx(GetExecutionContextRef());

ExecutionContext exe_ctx(GetExecutionContextRef()); someFunction(exe_ctx.GetBestExecutionContextScope());

What's unclear to me, is why bring ExecutionContextScope into the equation? From its documentation, ExecutionContext says:

These objects are designed to be used for short term execution context object storage while a function might be trying to evaluate something that requires a thread or frame.

Which does describe the needs of SwiftLanguageRuntime.

We should eventually upstream this change, but I don't see an issue with doing so. @jimingham previously pointed out that requiring a ValueObject in CompilerType is problematic, but there is prior art for requiring an optional ExecutionContext in calls like GetBitSize(). @jimingham do you agree with that characterization?

That's right. Since you really only want the process, or maybe a frame if you are trying to bind some generic, a ValueObject is way more than you need. But that you might need a process to determine some aspects of the type system, it makes sense to have API's into the type system that provide an ExecutionContext in some form.

The use cases and distinction between ExecutionContext and ExecutionContextScope is not clear to me at all. I read their docs and looked at their APIs.

Before I guess an answer — @jimingham ^ ?

As I understand it, the ExecutionContextScope is there because it is really handy to be able to use one of the objects in the Execution Context hierarchy to stand in for a scope specifier regardless of where you are in the hierarchy. I'm not sure for local passing of contexts there's a hard and fast rule on which to use, so far as I can tell it's more a matter of convenience. If you know you will mostly always have a process/thread/frame object on hand when calling the API, then ExecutionContextScope is way more convenient as you can bypass the conversion to ExecutionContext.

But you might also want to check with Greg in case I'm misremembering something. These were his designs originally.

Let's make the minimal API change to just add the context pointer here and then separately refurbish the API on llvm.org where we get rid of the char *, etc...

adrian-prantl · 2020-12-17T17:05:08Z

lldb/include/lldb/Target/SwiftLanguageRuntime.h

+  llvm::Optional<size_t> GetIndexOfChildMemberWithName(
+      CompilerType type, const char *name, ExecutionContext *exe_ctx,
+      bool omit_empty_base_classes, std::vector<uint32_t> &child_indexes);
+


This looks good — but it would be better to use an llvm::StringRef instead of a const char* for name.

Similarly, a MutableArrayRef http://llvm.org/doxygen/classllvm_1_1MutableArrayRef.html is preferred over a std::vector&. Then call sites can also use SmallVectors if they wish to do so.

thanks will do! TIL about MutableArrayRef

Seems MutableArrayRef won't work here, I don't see any way to modify an underlying vector, in particular to append to it.

Whoops, you're right! We could go to a SmallVectorImpl<uint32_t> but the returns are marginal.

Side note: Having now read the documentation for child_indexes (https://lldb.llvm.org/cpp_reference/classlldb__private_1_1CompilerType.html#abbeabd29258512dc7869c3f01afff93d) this method's entire API is weird.

haha very weird

For posteriority: The point of the child_indexes array is to support returning more than one match. I have no idea in which language this would even be possible. To top that off, the return value is that of child_indexes[0] or INT_MAX, and the comment has a TODO mark in it. We might want to check if this is even wired up in any language backend and otherwise potentially even remove it.

sounds good to me.

In the case of multiple children with the same name, a class can have a property of the same name as a property in its base class if the base class property is private.

In my quick check of c++, it allows duplicate members names regardless of access control.

It really depends on what we decide to mean by "ChildMember". If it means any member of the current object, then in C++ you can have two with the same name since they can come from different base classes. If it only means "any member contributed by the most specific class of the object, and you have to get to the others by recursing through the base class by hand", then I can't think of a way you could have two members with the same name.

I think the first meaning is more useful, but I'm not 100% set on that.

adrian-prantl · 2020-12-17T17:08:50Z

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

@@ -832,7 +832,8 @@ class TypeSystemClang : public TypeSystem {
  // so we catch all names that match a given child name, not just the first.
  size_t
  GetIndexOfChildMemberWithName(lldb::opaque_compiler_type_t type,
-                                const char *name, bool omit_empty_base_classes,


When we upstream the change to add the exe_ctx, we can also modernize the signature to use StringRef and ArrayRef. And perhaps even return an Optional<>!

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

kastiglione · 2020-12-18T00:37:06Z

@adrian-prantl d0a8d14 is the meat of this PR. Still to do:

Tune validation to handle cases where the runtime contains more info than the AST (0d5d9e2 and 93a4ce8)
Fix tests that assume private members are not valid (93a4ce8)
Debug failing TestSwiftAtObjCIvars.py test
~~Implement DumpTypeValue~~
Dereference weak types
Iterate over superclasses
Update the swiftinterface tests to verify the contents of the newly visible members
Debug infinite(?) recursion in TestSwiftProtocolTypes.py

Note: there's now a lot of common structure (logic and flow control) in a few of these functions, which I am planning to refactor as a follow up.

Now that `TypeSystemSwiftTypeRef::GetNumChildren` is implemented, if the runtime knows of more children than the AST contains, then the runtime should be used (`impl`), not the AST (`fallback`).

kastiglione · 2020-12-18T19:42:43Z

TestSwiftAtObjCIvars.py is failing because it needs DumpTypeValue to be implemented using the runtime.

kastiglione · 2020-12-18T21:26:08Z

Follow up: TestSwiftAtObjCIvars.py failed because I had a buggy first version of b85f9cd.

kastiglione · 2020-12-18T21:26:31Z

@swift-ci test

kastiglione · 2020-12-18T22:03:24Z

@adrian-prantl I think this is ready to review, the core changes are here 95dd368...ff5203d.

adrian-prantl

Great! Some minor suggestions inside.

adrian-prantl · 2020-12-18T23:53:15Z

lldb/include/lldb/Symbol/CompilerType.h

@@ -324,7 +324,7 @@ class CompilerType {

  /// Lookup a child given a name. This function will match base class names and
  /// member member names in "clang_type" only, not descendants.
-  uint32_t GetIndexOfChildWithName(const char *name,
+  uint32_t GetIndexOfChildWithName(const char *name, ExecutionContext *exe_ctx,


Let's make the minimal API change to just add the context pointer here and then separately refurbish the API on llvm.org where we get rid of the char *, etc...

adrian-prantl · 2020-12-19T00:01:59Z

lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

+  using namespace swift::reflection;
+  // Try the static type metadata.
+  const TypeRef *tr = nullptr;
+  auto *ti = GetTypeInfo(type, exe_ctx->GetFramePtr(), &tr);


this is outside of this patch, but maybe we should return a std::pair or a struct here.

I was thinking that too. Initially, only one caller used the TypeRef, it was less disruptive to add as an optional out-param. But now more callers are using it, so I will make that follow up change.

adrian-prantl · 2020-12-19T00:05:54Z

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

@@ -35,6 +35,9 @@
 #include "clang/APINotes/APINotesManager.h"
 #include "clang/APINotes/APINotesReader.h"

+#include <algorithm>
+#include <sstream>


We can avoid this...

adrian-prantl · 2020-12-19T00:06:19Z

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

+
+        auto fail = [&]() {
+          auto join = [](const auto &v) {
+            std::ostringstream buf;


... by calling llvm::join from StringExtras.h http://llvm.org/doxygen/namespacellvm.html#aa1fd1719afe5b07ebe55f1d105c98916

I went to use llvm::join, but it's for containers of strings, and this case is a vector<int> basically. Since I'd need to convert to strings, it was shorter to do this, and since it will hopefully soon be deleted (that's the plan for the validation code right?), I didn't force the issue of using llvm::join.

lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

lldb/test/API/lang/swift/parseable_interfaces/shared/TestSwiftInterfaceNoDebugInfo.py

kastiglione · 2020-12-19T00:19:56Z

@swift-ci test

kastiglione · 2020-12-19T03:37:41Z

@swift-ci test

kastiglione · 2020-12-19T06:59:32Z

@swift-ci test

kastiglione requested a review from adrian-prantl December 17, 2020 03:27

adrian-prantl reviewed Dec 17, 2020

View reviewed changes

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwift.cpp Show resolved Hide resolved

kastiglione added 3 commits December 17, 2020 10:52

thread exe_ctx through GetIndexOfChild(Member)?WithName

95dd368

call newly stubbed SwiftLanguageRuntime::GetIndexOfChildMemberWithName

0a86741

use StringRef and MutableArrayRef in new API

4bd5d15

kastiglione force-pushed the lldb-Implement-TypeSystemSwiftTypeRef-GetIndexOfChildMemberWithName branch from e80330f to 4bd5d15 Compare December 17, 2020 19:32

kastiglione added 2 commits December 17, 2020 12:46

revert MutableArrayRef back to vector

7b8951c

implement most of GetIndexOfChildMemberWithName

d0a8d14

kastiglione added 3 commits December 18, 2020 09:43

in GetChildCompilerTypeAtIndex, switch fallback() to impl()

0d5d9e2

Now that `TypeSystemSwiftTypeRef::GetNumChildren` is implemented, if the runtime knows of more children than the AST contains, then the runtime should be used (`impl`), not the AST (`fallback`).

rework GetIndexOfChildMemberWithName validation

ba443ff

first changes to tests now that these private members are visible

93a4ce8

kastiglione added 2 commits December 18, 2020 13:24

take base class into account when calculating member index

b85f9cd

use <3 instead of <=2 - for consistency

fc3c654

kastiglione added 2 commits December 18, 2020 13:31

dereference weak types

1d92d40

traverse class hierarchy

d89aacb

kastiglione marked this pull request as ready for review December 18, 2020 21:56

kastiglione added 4 commits December 18, 2020 15:28

fix superclass iteration

74d407a

return 0 not None for ThickFunction

ff5203d

handle ExistentialMetatype in SwiftLanguageRuntimeImpl

49a045c

update TestLibraryIndirect.py

094fdb3

adrian-prantl approved these changes Dec 19, 2020

View reviewed changes

kastiglione added 2 commits December 18, 2020 16:16

comment: LLDB can find it through type metadata

922d587

use T v(...) style initialization

5d0f70d

update No.swiftmodule.swift

466cba7

kastiglione added 2 commits December 18, 2020 22:44

reenable TestSwiftFoundationTypeNotification.py

dd9ee01

add superclass to child_indexes

ecc71dd

kastiglione mentioned this pull request Dec 19, 2020

[lldb] Implement TypeSystemSwiftTypeRef::GetIndexOfChildMemberWithName #2271

Merged

kastiglione merged commit 4ccae3c into swift/release/5.4 Dec 19, 2020

kastiglione deleted the lldb-Implement-TypeSystemSwiftTypeRef-GetIndexOfChildMemberWithName branch December 19, 2020 20:02

[lldb] Implement TypeSystemSwiftTypeRef::GetIndexOfChildMemberWithName #2260

[lldb] Implement TypeSystemSwiftTypeRef::GetIndexOfChildMemberWithName #2260

Uh oh!

Conversation

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

Uh oh!

Uh oh!

kastiglione commented Dec 17, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimingham Dec 18, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

kastiglione commented Dec 18, 2020

Uh oh!

kastiglione commented Dec 18, 2020

Uh oh!

kastiglione commented Dec 18, 2020

Uh oh!

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

Uh oh!

Uh oh!

adrian-prantl left a comment

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 17, 2020 •

edited

Loading

kastiglione commented Dec 17, 2020 •

edited

Loading

jimingham Dec 18, 2020 •

edited

Loading

kastiglione commented Dec 18, 2020 •

edited

Loading

kastiglione commented Dec 18, 2020 •

edited

Loading