Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[lldb] Add enum support to TypeSystemSwiftTypeRef::DumpTypeValue #2330
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.
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
[lldb] Add enum support to TypeSystemSwiftTypeRef::DumpTypeValue #2330
Changes from all commits
ccfcdc6
c4fbbcc
0d1bead
dae5d8a
48fd6e6
a976288
a9ded5d
df36fa0
0e28747
32e0a23
2ce9bbd
3e6a52c
83ef63f
511d858
3dad37a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This function strips the private discriminators (identifiers) from a demangled symbol. This function is used when comparing symbols in
Equivalent
, so that two symbols that differ only by the unique identifier inPrivateDeclName
, are considered equal.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.
Would this hide bugs where we accidentally mix up these two types named Foo?
a.swift
fileprivate struct Foo {}
b.swift
fileprivate struct Foo { let i = "i am different" }
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.
What I want to say is: we need to make sure to not forget to investigate why private discriminators reported by remote mirrors look different than the ones the Swift compiler emits in the debug info.
But that's not something blocking this patch. (But maybe there should be a bug FIXME in the code).
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.
good catch. I'll have to constrain this more.
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.
I ended up asking about this, and the explanation is:
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 the purpose of verification, it's okay to compare tham in a relaxed manner. But for correctness, this is a potential problem with our incremental adoption of TypeSystemSwiftTypeRef. Since we mix&match between type systems, this makes it impossible to convert between AST <-> TypeRef types with private discriminators unless we have a ValueObject with an AST type to bind the two together.
We do store the private discriminator of each source file in DWARF (as a DW_TAG_namespace). If there is a way to tie a typeref type to a DWARF compile unit, we could do the conversion from runtime -> ast discriminator. But I don't think this is going to be possible in all cases: For example when using remote mirrors to resolve the dynamic type of an existential, how would we know where that type was defined?
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.
Is my understanding here correct?
This is a problem for private types which lldb has the AST for. For the SDK, we'll never have private types, so the issue of AST <-> TypeRef conversion can't occur for SDK types.
For project code, there is code that's built using full swiftmodules, and code that's builds using library evolution and swiftinterfaces. For builds using full fidelity swiftmodules, can or should lldb use the AST only? If AST were strictly used when it's available, and TypeRefs used only when the AST is not available, then would we never need to worry about conversion? However, would we be able to recognize the difference between a full swiftmodule, and a swiftmodule generated from a swiftinterface?
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.
I'm not sure whether the TypeRef can be cross referenced to a compile unit. I'll have to look into that.
I wonder if we can cross reference through uniqueness? If, ignoring the private identifier, there's only a single type by that name (unlike say your
fileprivate struct Foo
example), then if we enumerated all types we would see that there's just the one. However that sounds heavy handed and poor for performance/maintenance.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.
do we need to do this, and if so why? To put it another way, if we get a private TypeRef, when might lldb try to use that type in an AST context, vs always using reflection for such types? In other words, once you go TypeRef, do you ever need to go back to AST?
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.
No Swift ASTs are shipped for the SDK overlays. Therefore, we may get private types from Remote Mirrors, but never in the ASTs. This same is not true for development builds where we build the older snapshot of the Swift overlays that is inside the apple/swift repository from source.
The situation for SDK types could arise in the testsuite but not in production.
Here is gets much more complicated. In the expression evaluator all type lookup requests from the Swift compiler get sent back to LLDB, and it's LLDB's job to resolve type names. LLDB might perform dynamic type resolution (using Remote Mirrors) here and then could try to reconstruct a type that does not exist in the AST world. We also can run into the exact same issue with Clang types. Dynamic type resolution can return a private implementation subclass (e.g., NSString has multiple internal implementations) that the AST cannot know about because its declaration is not part of the SDK at all.
In the case of private subclasses, we could try reconstructing more general types (superclasses) until we find one that can be reconstructed. But this does not work for all private types in general. LLDB would probably need to make up a declaration for these types and hand it to the AST.
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.
How was this resolved in this patch?
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.
It's not entirely resolved, more changes required, one at least is to support validation. But it's removal is required to support
Foundation.IndexPath.Storage
whose type would be ignored by this condition.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.
See here: #2330 (comment)
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.
Here's a thought. In this case, we could have two levels of "fallback" logic:
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.
I've had some second thoughts about this fallback logic. A possible deal breaker is that the AST could contain fewer children, and that would make the index invalid. In other words, a runtime index could return some other child in the AST. To proceed, we'd have to compare the AST child to see that it matches the runtime child, modulo the private identifier, but that style of comparison is in-exact which could cause rare bugs.
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.
Could we add a
if GetNumChildren(typeref) == GetNumChildren(ast)
condition to the fallback and error out otherwise?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.
I added the fallback with a child count check in 3dad37a.
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.
do we know what "some instances" are?
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.
There are many instances. I'm not currently sure which objc enums are mangled as structs, and which aren't, nor why. It's something I could look into.
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 save that investigation for another day.
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.
Swift reflection code expects this function to return true when a valid value is assigned to
outBuffer
.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.
This was previously unimplemented, and so would return false for
DLQ_GetLeastValidPointerValue
queries. This causedprojectEnumValue
in Swift reflection to fail.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.
See also rdar://70729149
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.
Some enum values are zero sized. Because of the asserts in this function,
PushLocalBuffer()
previously should be only called if the buffer was non-zero. This means you'd have to write code like:By using optional, the calling code can be made unconditional:
This works better with
make_scope_exit
too.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 work around is currently limited to
Foundation.IndexPath.Storage
, because that's the only case surfaced by the lldb test suite. I'd like this to be exhaustive, but I'll have to do some work to identify other any other multipayload enums in Foundation that we need to test and hardcode a special case for.