[lldb] Implement TypeSystemSwiftTypeRef::GetIndexOfChildMemberWithName #2260
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -256,6 +256,10 @@ class SwiftLanguageRuntime : public LanguageRuntime { | |
| llvm::Optional<unsigned> GetNumChildren(CompilerType type, | ||
| ValueObject *valobj); | ||
|
|
||
| llvm::Optional<size_t> GetIndexOfChildMemberWithName( | ||
| CompilerType type, llvm::StringRef name, ExecutionContext *exe_ctx, | ||
| bool omit_empty_base_classes, std::vector<uint32_t> &child_indexes); | ||
|
|
||
|
There was a problem hiding this comment. This looks good — but it would be better to use an There was a problem hiding this comment. 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. There was a problem hiding this comment. thanks will do! TIL about There was a problem hiding this comment. Seems There was a problem hiding this comment. Whoops, you're right! We could go to a Side note: Having now read the documentation for There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. |
||
| /// Ask Remote Mirrors about a child of a composite type. | ||
| CompilerType GetChildCompilerTypeAtIndex( | ||
| CompilerType type, size_t idx, bool transparent_pointers, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,9 @@ | |
| #include "clang/APINotes/APINotesManager.h" | ||
| #include "clang/APINotes/APINotesReader.h" | ||
|
|
||
| #include <algorithm> | ||
| #include <sstream> | ||
|
|
||
|
|
||
| using namespace lldb; | ||
| using namespace lldb_private; | ||
|
|
||
|
|
@@ -2324,7 +2327,7 @@ CompilerType TypeSystemSwiftTypeRef::GetChildCompilerTypeAtIndex( | |
| if (m_swift_ast_context->GetNumChildren(ReconstructType(type), | ||
| omit_empty_base_classes, exe_ctx) < | ||
| runtime->GetNumChildren({this, type}, valobj).getValueOr(0)) | ||
| return impl(); | ||
|
|
||
| #ifndef NDEBUG | ||
| std::string ast_child_name; | ||
|
|
@@ -2365,11 +2368,70 @@ CompilerType TypeSystemSwiftTypeRef::GetChildCompilerTypeAtIndex( | |
| } | ||
|
|
||
| size_t TypeSystemSwiftTypeRef::GetIndexOfChildMemberWithName( | ||
| opaque_compiler_type_t type, const char *name, ExecutionContext *exe_ctx, | ||
| bool omit_empty_base_classes, std::vector<uint32_t> &child_indexes) { | ||
| if (auto *exe_scope = exe_ctx->GetBestExecutionContextScope()) | ||
| if (auto *runtime = | ||
| SwiftLanguageRuntime::Get(exe_scope->CalculateProcess())) | ||
| if (auto index_size = runtime->GetIndexOfChildMemberWithName( | ||
| GetCanonicalType(type), name, exe_ctx, omit_empty_base_classes, | ||
| child_indexes)) { | ||
| #ifndef NDEBUG | ||
| // This block is a custom VALIDATE_AND_RETURN implementation to support | ||
| // checking the return value, plus the by-ref `child_indexes`. | ||
| if (!m_swift_ast_context) | ||
| return *index_size; | ||
| auto ast_type = ReconstructType(type); | ||
| if (!ast_type) | ||
| return *index_size; | ||
| std::vector<uint32_t> ast_child_indexes; | ||
| auto ast_index_size = m_swift_ast_context->GetIndexOfChildMemberWithName( | ||
| ast_type, name, exe_ctx, omit_empty_base_classes, | ||
| ast_child_indexes); | ||
| // The runtime has more info than the AST. No useful validation can be | ||
| // done. | ||
| if (*index_size > ast_index_size) | ||
| return *index_size; | ||
|
|
||
| auto fail = [&]() { | ||
| auto join = [](const auto &v) { | ||
| std::ostringstream buf; | ||
|
There was a problem hiding this comment. ... by calling llvm::join from StringExtras.h http://llvm.org/doxygen/namespacellvm.html#aa1fd1719afe5b07ebe55f1d105c98916 There was a problem hiding this comment. I went to use |
||
| buf << "{"; | ||
| for (const auto &item : v) | ||
| buf << item << ","; | ||
| buf.seekp(-1, std::ios_base::end); | ||
| buf << "}"; | ||
| return buf.str(); | ||
| }; | ||
| llvm::dbgs() << join(child_indexes) | ||
| << " != " << join(ast_child_indexes) << "\n"; | ||
| llvm::dbgs() << "failing type was " << (const char *)type | ||
| << ", member was " << name << "\n"; | ||
| assert(false && | ||
| "TypeSystemSwiftTypeRef diverges from SwiftASTContext"); | ||
| }; | ||
| if (*index_size != ast_index_size) | ||
| fail(); | ||
| for (unsigned i = 0; i < *index_size; ++i) | ||
| if (child_indexes[i] < ast_child_indexes[i]) | ||
| // When the runtime may know know about more children. When this | ||
| // happens, indexes will be larger. But if an index is smaller, that | ||
| // means the runtime has dropped info somehow. | ||
| fail(); | ||
| #endif | ||
| return *index_size; | ||
| } | ||
|
|
||
| LLDB_LOGF(GetLogIfAllCategoriesSet(LIBLLDB_LOG_TYPES), | ||
| "Using SwiftASTContext::GetIndexOfChildMemberWithName fallback for " | ||
| "type %s", | ||
| AsMangledName(type)); | ||
|
|
||
| return m_swift_ast_context->GetIndexOfChildMemberWithName( | ||
| ReconstructType(type), name, exe_ctx, omit_empty_base_classes, | ||
| child_indexes); | ||
| } | ||
|
|
||
| size_t | ||
| TypeSystemSwiftTypeRef::GetNumTemplateArguments(opaque_compiler_type_t type) { | ||
| return m_swift_ast_context->GetNumTemplateArguments(ReconstructType(type)); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for what it's worth, the functions I've seen need the process, so passing a process instead of
ExecutionContextmakes 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use cases and distinction between
ExecutionContextandExecutionContextScopeis not clear to me at all. I read their docs and looked at their APIs. The primary reason I went withExecutionContexthere is because fromValueObjectit's more direct/convenient.Within
ValueObject, both of these patterns exist:ExecutionContext exe_ctx(GetExecutionContextRef());What's unclear to me, is why bring
ExecutionContextScopeinto the equation? From its documentation,ExecutionContextsays:Which does describe the needs of
SwiftLanguageRuntime.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I guess an answer — @jimingham ^ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...