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.
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/DWARF] Fix type definition search with simple template names #95905
[lldb/DWARF] Fix type definition search with simple template names #95905
Changes from 1 commit
9429eef
e5b7c3e
25c2776
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.
At some point we should probably extract this into a standalone helper. We're accumulating a lot of these checks
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.
Yeah.. Doesn't exactly help here, but I've been wondering about this in the context of the CompilerContext class. If we don't care about the class vs. struct distinction when resolving types, maybe we could remove the distinction at least from there?
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.
Mind pointing me to the place you're referring to?
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 have both CompilerContextKind::Class and CompilerContextKind::Struct. Since these classes are (mainly?) used for looking things up in the debug info (where we treat the two as (mostly) the same), I'm thinking whether we could merge these two into one.
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.
Ah I see, yea those two seem to always be used in the same way. Merging them would make sense. Of course that still leaves us with the
DW_TAG_structure_type || DW_TAG_class_type
checks, but that can always be cleaned up later.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.
Yeah, I'm not quite sure how to handle those. We can't just completely erase the difference there because they matter for creation of clang asts. One idea (that just occurred to me) would be to have a sort of a getCanonicalTag() which maps structure_type to class_type (or the other way around), and then do these comparisons on the "canonical" tags.
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.
FWIW, we do disable the main distinction (access control) between the two in the AST. But fair point, there might be other places that care, not sure off the top of my head.
Good idea!
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.
FWIW, I'm pretty sure that the only difference between class and struct is the access control.
Though C++ technically rejects things like forward declaring a thing as a struct, then actually defining it as a class - not sure if that'd ever come up for lldb (for instance when interacting with clang header modules? If the DWARF contained a declaration of a thing and always created it as a struct, but then tried to load a module with that name as a class definition) I guess it'd also maybe break technically valid user code in expression evaluation - if the name was used for a varibale and a type, the user should be able to disambiguate in favor of the type by saying, eg: "sizeof(class MyType)" but if lldb made everything a struct ... hmm, nope, that'd be fine:
I guess printing types in lldb would be technically wrong, since it'd print as "struct MyType {" instead of "class MyType {"?
(but it might mean not having to have the access modifier hack? If everything was made as a struct, and none of the members were given other access modifiers - everything would be accessible by default anyway)
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.
Yes, that's why I said "it matters for ast construction". I think it would nice to still print the type in the original way, which mostly comes up when someone (technically incorrectly) forward-declares something as
struct Foo
, but defines it as a class.If we wanted to, we could make all members public regardless of the tag type. I don't know why we don't do that. Is it just because it's nice to see the original access specifiers in the type readouts ? (a rhetorical question) I know that the access specifier can in theory affect the type layout, but I don't think that's true for any real world ABI (and dwarf already encodes the layout anyway).. Some users may declare types inside expressions and their members would be private without the hack, but maybe they should just get what they asked for...
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 lost a sentence here.
It would be nice to print the types in the original way. We need the canonicalization to handle the cases where the forward declarations and definitions don't have matching tags.
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.
yay