-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Fix type lookup bug where wrong decl context was being used for a DIE. #94846
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
CXX_SOURCES := main.cpp | ||
include Makefile.rules |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,66 @@ | ||||||||||
""" | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's also a unit-test for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can do that. I have a DWARF generator that I wrote in python. |
||||||||||
Test the SBModule and SBTarget type lookup APIs to find multiple types. | ||||||||||
""" | ||||||||||
|
||||||||||
import lldb | ||||||||||
from lldbsuite.test.lldbtest import * | ||||||||||
from lldbsuite.test import lldbutil | ||||||||||
|
||||||||||
|
||||||||||
class TypeFindFirstTestCase(TestBase): | ||||||||||
def test_find_first_type(self): | ||||||||||
""" | ||||||||||
Test SBTarget::FindTypes() and SBModule::FindTypes() APIs. | ||||||||||
We had issues where our declaration context when finding types was | ||||||||||
incorrectly calculated where a type in a namepace, and a type in a | ||||||||||
function that was also in the same namespace would match a lookup. For | ||||||||||
example: | ||||||||||
namespace a { | ||||||||||
struct Foo { | ||||||||||
int foo; | ||||||||||
}; | ||||||||||
unsigned foo() { | ||||||||||
typedef unsigned Foo; | ||||||||||
Foo foo = 12; | ||||||||||
return foo; | ||||||||||
} | ||||||||||
} // namespace a | ||||||||||
Previously LLDB would calculate the declaration context of "a::Foo" | ||||||||||
correctly, but incorrectly calculate the declaration context of "Foo" | ||||||||||
from within the foo() function as "a::Foo". Adding tests to ensure this | ||||||||||
works correctly. | ||||||||||
""" | ||||||||||
self.build() | ||||||||||
target = self.createTestTarget() | ||||||||||
exe_module = target.GetModuleAtIndex(0) | ||||||||||
self.assertTrue(exe_module.IsValid()) | ||||||||||
# Test the SBTarget and SBModule APIs for FindFirstType | ||||||||||
for api in [target, exe_module]: | ||||||||||
# We should find the "a::Foo" but not the "Foo" type in the function | ||||||||||
types = api.FindTypes("a::Foo") | ||||||||||
self.assertEqual(types.GetSize(), 1) | ||||||||||
type_str0 = str(types.GetTypeAtIndex(0)) | ||||||||||
self.assertIn('struct Foo {', type_str0) | ||||||||||
|
||||||||||
# When we search by type basename, we should find any type whose | ||||||||||
# basename matches "Foo", so "a::Foo" and the "Foo" type in the | ||||||||||
# function. | ||||||||||
types = api.FindTypes("Foo") | ||||||||||
self.assertEqual(types.GetSize(), 2) | ||||||||||
type_str0 = str(types.GetTypeAtIndex(0)) | ||||||||||
type_str1 = str(types.GetTypeAtIndex(1)) | ||||||||||
# We don't know which order the types will come back as, so | ||||||||||
self.assertEqual(set([str(t).split('\n')[0] for t in types]), set(["typedef Foo", "struct Foo {"])) | ||||||||||
|
||||||||||
# When we search by type basename with "::" prepended, we should | ||||||||||
# only types in the root namespace which means only "Foo" type in | ||||||||||
Comment on lines
+60
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just asking why the typedef will be at the root level but not the struct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say this is actually a bug, and it's one of the reasons why I said the two functions should be merged (as Michael alluded to in the other comment). I think that a better full name for this type would be something like
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I don't know of anyone that would use the function name when asking for a type, even if that type was defined in a function. To make the expression parser work, we should be able to find all of the types using a context like just "Foo", where we would find both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's certainly not valid C++, if that's what you mean (c++ just doesn't give you the option of referring to a local type from outside of the function), but I don't think this kind of qualification is a revolutionary concept. For example if the aforementioned local struct had a constructor, it's mangled name would be
This approach has the problem that a type (or really, anything) defined with the same name at a class level would take precedence over a local declaration. For example with code like:
if we were stopped in |
||||||||||
# the function. | ||||||||||
types = api.FindTypes("::Foo") | ||||||||||
self.assertEqual(types.GetSize(), 1) | ||||||||||
type_str0 = str(types.GetTypeAtIndex(0)) | ||||||||||
self.assertIn('typedef Foo', type_str0) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
namespace a { | ||
struct Foo {}; | ||
|
||
unsigned foo() { | ||
typedef unsigned Foo; | ||
Foo foo = 12; | ||
return foo; | ||
} | ||
} // namespace a | ||
|
||
int main() { | ||
a::Foo f = {}; | ||
a::foo(); | ||
return 0; | ||
} |
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 looks like the early-return was removed in the refactor in #93291?
This LGTM but @labath had plans to align
GetTypeLookupContext
andGetDeclContext
, so wanted him to have a look tooThere 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 fine. The removal of the early return was unintentional, although (see other comment) I don't think it's totally the right thing to do either.