Fix type lookup bug where wrong decl context was being used for a DIE. #94846
Conversation
The function that calculated the declaration context for a DIE was incorrectly transparently traversing acrosss DW_TAG_subprogram dies when climbing the parent DIE chain. This meant that types defined in functions would appear to have the declaration context of anything above the function. I fixed the GetTypeLookupContextImpl(...) function in DWARFDIE.cpp to not transparently skip over functions, lexical blocks and inlined functions and compile and type units. Added a test to verify things are working.
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-lldb Author: Greg Clayton (clayborg) ChangesThe function that calculated the declaration context for a DIE was incorrectly transparently traversing acrosss DW_TAG_subprogram dies when climbing the parent DIE chain. This meant that types defined in functions would appear to have the declaration context of anything above the function. I fixed the GetTypeLookupContextImpl(...) function in DWARFDIE.cpp to not transparently skip over functions, lexical blocks and inlined functions and compile and type units. Added a test to verify things are working. Full diff: https://github.com/llvm/llvm-project/pull/94846.diff 4 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
index 7cf92adc6ef57..c10174e8848ee 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
@@ -491,6 +491,18 @@ static void GetTypeLookupContextImpl(DWARFDIE die,
case DW_TAG_base_type:
push_ctx(CompilerContextKind::Builtin, name);
break;
+ // If any of the tags below appear in the parent chain, stop the decl
+ // context and return. Prior to these being in here, if a type existed in a
+ // namespace "a" like "a::my_struct", but we also have a function in that
+ // same namespace "a" which contained a type named "my_struct", both would
+ // return "a::my_struct" as the declaration context since the
+ // DW_TAG_subprogram would be skipped and its parent would be found.
+ case DW_TAG_compile_unit:
+ case DW_TAG_type_unit:
+ case DW_TAG_subprogram:
+ case DW_TAG_lexical_block:
+ case DW_TAG_inlined_subroutine:
+ return;
default:
break;
}
diff --git a/lldb/test/API/functionalities/type_types/Makefile b/lldb/test/API/functionalities/type_types/Makefile
new file mode 100644
index 0000000000000..3d0b98f13f3d7
--- /dev/null
+++ b/lldb/test/API/functionalities/type_types/Makefile
@@ -0,0 +1,2 @@
+CXX_SOURCES := main.cpp
+include Makefile.rules
diff --git a/lldb/test/API/functionalities/type_types/TestFindTypes.py b/lldb/test/API/functionalities/type_types/TestFindTypes.py
new file mode 100644
index 0000000000000..adbaaba51d080
--- /dev/null
+++ b/lldb/test/API/functionalities/type_types/TestFindTypes.py
@@ -0,0 +1,69 @@
+"""
+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
+ if 'struct Foo' in type_str0:
+ self.assertIn('typedef Foo', type_str1)
+ else:
+ self.assertIn('struct Foo', type_str1)
+
+ # When we search by type basename with "::" prepended, we should
+ # only types in the root namespace which means only "Foo" type in
+ # the function.
+ types = api.FindTypes("::Foo")
+ self.assertEqual(types.GetSize(), 1)
+ type_str0 = str(types.GetTypeAtIndex(0))
+ self.assertIn('typedef Foo', type_str0)
diff --git a/lldb/test/API/functionalities/type_types/main.cpp b/lldb/test/API/functionalities/type_types/main.cpp
new file mode 100644
index 0000000000000..57eecd81f0ce7
--- /dev/null
+++ b/lldb/test/API/functionalities/type_types/main.cpp
@@ -0,0 +1,16 @@
+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;
+}
|
You can test this locally with the following command:darker --check --diff -r 5aabbf0602c48b67bb89fd37f95bf97c95ded488...3d7b371cde6860a9bfa66900249696cc84b4444d lldb/test/API/functionalities/type_types/TestFindTypes.pyView the diff from darker here.--- TestFindTypes.py 2024-06-11 00:12:32.000000 +0000
+++ TestFindTypes.py 2024-06-11 00:37:04.178767 +0000
@@ -43,24 +43,27 @@
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)
+ 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 {"]))
+ 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
# the function.
types = api.FindTypes("::Foo")
self.assertEqual(types.GetSize(), 1)
type_str0 = str(types.GetTypeAtIndex(0))
- self.assertIn('typedef Foo', type_str0)
+ self.assertIn("typedef Foo", type_str0)
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| # When we search by type basename with "::" prepended, we should | ||
| # only types in the root namespace which means only "Foo" type in |
There was a problem hiding this comment.
| # When we search by type basename with "::" prepended, we should | |
| # only types in the root namespace which means only "Foo" type in | |
| # When we search by type basename with "::" prepended, we should | |
| # return only types in the root namespace which means only "Foo" type in |
There was a problem hiding this comment.
Just asking why the typedef will be at the root level but not the struct?
There was a problem hiding this comment.
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 ::a::foo()::Foo
I did look into this briefly (before being preempted). Basically, I had this function follow DW_AT_specification links just like it was done for GetDeclContext, but I ran into two problems:
- this just gives you a name like
::a::foo::Foowhich, while probably better than::Foois still not fully correct as the typeFooinfoo(void)can be completely different from typeFooinfoo(something_else). - it prevented us from accessing the type
Fooin the expression evaluator, which currently "works" (scare quotes, because it only works if there are no conflicts) because we pretend the type to be global. To fix this we'd need to implement a dedicated method for injecting local types into the expression, just like we do with local variables. I don't think that's particularly hard, but it definitely takes more time that I had at the moment.
There was a problem hiding this comment.
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 ::Foo and the Foo from a::foo(), but sort the results based on the CompilerDeclContext of each type that was returned. If we are evaluating an expression in a::foo(), the Foo type from that function could tell us the CompilerDeclContext it was defined in and we would find the closest match to where the expression is being run.
There was a problem hiding this comment.
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.
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 _ZZN1a3fooEvEN3FooC1Ev , that is a::foo()::Foo::Foo(). Gdb (AFAICT) does not support these kinds of lookups, but it also does not return the type for query like ::Foo. (instead gdb supports searching for a type in a specific block/function, which is kind of similar to this)
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
::Fooand theFoofroma::foo(), but sort the results based on the CompilerDeclContext of each type that was returned. If we are evaluating an expression ina::foo(), theFootype from that function could tell us the CompilerDeclContext it was defined in and we would find the closest match to where the expression is being run.
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:
struct Foo { int one; }
class Class {
struct Foo { int two; }
void Method() {
struct Foo { int three; }
}
};
if we were stopped in Class::Method, then the name Foo should refer to the third definition with that name, but I believe this will end up using the second one because clang will see the class-level declaration first and will not bother asking for other possible definitions. I think we used to use the same approach for local variables, but then we changed the implementation because of the same shadowing problem.
| # When we search by type basename with "::" prepended, we should | ||
| # only types in the root namespace which means only "Foo" type in |
There was a problem hiding this comment.
Just asking why the typedef will be at the root level but not the struct?
| // same namespace "a" which contained a type named "my_struct", both would | ||
| // return "a::my_struct" as the declaration context since the | ||
| // DW_TAG_subprogram would be skipped and its parent would be found. | ||
| case DW_TAG_compile_unit: |
There was a problem hiding this comment.
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.
| @@ -0,0 +1,69 @@ | |||
| """ | |||
There was a problem hiding this comment.
There's also a unit-test for GetTypeLookupContext in lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp. Would be nice to add a test-case there too (if the yaml isn't too much of a hassle)
There was a problem hiding this comment.
I can do that. I have a DWARF generator that I wrote in python.
| 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 | ||
| if 'struct Foo' in type_str0: | ||
| self.assertIn('typedef Foo', type_str1) | ||
| else: | ||
| self.assertIn('struct Foo', type_str1) |
There was a problem hiding this comment.
Would something like this work:
assertEqual(set([t.GetName() for t in types]), set(["typedef Foo", "struct Foo"]))
There was a problem hiding this comment.
I believe it might, I will try it
| # When we search by type basename with "::" prepended, we should | ||
| # only types in the root namespace which means only "Foo" type in |
There was a problem hiding this comment.
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 ::a::foo()::Foo
I did look into this briefly (before being preempted). Basically, I had this function follow DW_AT_specification links just like it was done for GetDeclContext, but I ran into two problems:
- this just gives you a name like
::a::foo::Foowhich, while probably better than::Foois still not fully correct as the typeFooinfoo(void)can be completely different from typeFooinfoo(something_else). - it prevented us from accessing the type
Fooin the expression evaluator, which currently "works" (scare quotes, because it only works if there are no conflicts) because we pretend the type to be global. To fix this we'd need to implement a dedicated method for injecting local types into the expression, just like we do with local variables. I don't think that's particularly hard, but it definitely takes more time that I had at the moment.
| // same namespace "a" which contained a type named "my_struct", both would | ||
| // return "a::my_struct" as the declaration context since the | ||
| // DW_TAG_subprogram would be skipped and its parent would be found. | ||
| case DW_TAG_compile_unit: |
There was a problem hiding this comment.
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.
| @@ -0,0 +1,69 @@ | |||
| """ | |||
- Modified the python test to use self.assertEqual() - Added a unittest
The function that calculated the declaration context for a DIE was incorrectly transparently traversing acrosss DW_TAG_subprogram dies when climbing the parent DIE chain. This meant that types defined in functions would appear to have the declaration context of anything above the function. I fixed the GetTypeLookupContextImpl(...) function in DWARFDIE.cpp to not transparently skip over functions, lexical blocks and inlined functions and compile and type units. Added a test to verify things are working.