[lldb] Make CompilerDecl::GetName (always) return template args #116068
Conversation
I ran into this while look at a different bug (patch coming soon). This function has only two callers. The first is SBTypeStaticField::GetName (which doesn't care about templates), and the other is CompilerDecl::GetCompilerContext (in the TypeQuery constructor), which does want template arguments. This function was (normally) returning the name without template args. Since this code is only used when looking up a type in another shared library, the odds of running into this bug are relatively low, but I add a test to demonstrate the scenario and the fix for it nonetheless. Amazingly (and scarily), this test actually passes without this change in the default configuration -- and only fails with -gsimple-template-names. The reason for that is that in the non-simplified case we create a regular CXXRecordDecl whose name is "bar<int>" (instead of a template record "foo" with an argument of "int"). When evaluating the expression, we are somehow able to replace this with a proper template specialization decl.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesI ran into this while look at a different bug (patch coming soon). This function has only two callers. The first is SBTypeStaticField::GetName (which doesn't care about templates), and the other is CompilerDecl::GetCompilerContext (in the TypeQuery constructor), which does want template arguments. This function was (normally) returning the name without template args. Since this code is only used when looking up a type in another shared library, the odds of running into this bug are relatively low, but I add a test to demonstrate the scenario and the fix for it nonetheless. Amazingly (and scarily), this test actually passes without this change in the default configuration -- and only fails with -gsimple-template-names. The reason for that is that in the non-simplified case we create a regular CXXRecordDecl whose name is "bar<int>" (instead of a template record "foo" with an argument of "int"). When evaluating the expression, we are somehow able to replace this with a proper template specialization decl. Full diff: https://github.com/llvm/llvm-project/pull/116068.diff 6 Files Affected:
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index f5063175d6e070..c5249088a291b8 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -9116,7 +9116,7 @@ ConstString TypeSystemClang::DeclGetName(void *opaque_decl) {
clang::NamedDecl *nd =
llvm::dyn_cast<NamedDecl>((clang::Decl *)opaque_decl);
if (nd != nullptr)
- return ConstString(nd->getDeclName().getAsString());
+ return ConstString(GetTypeNameForDecl(nd, /*qualified=*/false));
}
return ConstString();
}
diff --git a/lldb/test/API/lang/cpp/forward/Makefile b/lldb/test/API/lang/cpp/forward/Makefile
new file mode 100644
index 00000000000000..0b806b314397c5
--- /dev/null
+++ b/lldb/test/API/lang/cpp/forward/Makefile
@@ -0,0 +1,5 @@
+CXX_SOURCES := main.cpp
+DYLIB_CXX_SOURCES := foo.cpp
+DYLIB_NAME := foo
+
+include Makefile.rules
diff --git a/lldb/test/API/lang/cpp/forward/TestCPPForwardDeclaration.py b/lldb/test/API/lang/cpp/forward/TestCPPForwardDeclaration.py
new file mode 100644
index 00000000000000..5e9dd9c2227dd5
--- /dev/null
+++ b/lldb/test/API/lang/cpp/forward/TestCPPForwardDeclaration.py
@@ -0,0 +1,61 @@
+"""Test that forward declaration of a c++ template gets resolved correctly."""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+import lldbsuite.test.lldbutil as lldbutil
+
+
+class ForwardDeclarationTestCase(TestBase):
+ def do_test(self, dictionary=None):
+ """Display *bar_ptr when stopped on a function with forward declaration of struct bar."""
+ self.build(dictionary=dictionary)
+ exe = self.getBuildArtifact("a.out")
+ target = self.dbg.CreateTarget(exe)
+ self.assertTrue(target, VALID_TARGET)
+
+ environment = self.registerSharedLibrariesWithTarget(target, ["foo"])
+
+ # Break inside the foo function which takes a bar_ptr argument.
+ lldbutil.run_break_set_by_symbol(self, "foo", num_expected_locations=1)
+
+ process = target.LaunchSimple(
+ None, environment, self.get_process_working_directory()
+ )
+ self.assertTrue(process, PROCESS_IS_VALID)
+
+ # The stop reason of the thread should be breakpoint.
+ self.expect(
+ "thread list",
+ STOPPED_DUE_TO_BREAKPOINT,
+ substrs=["stopped", "stop reason = breakpoint"],
+ )
+
+ # The breakpoint should have a hit count of 1.
+ lldbutil.check_breakpoint(self, bpno=1, expected_hit_count=1)
+
+ self.expect_expr(
+ "*bar_ptr",
+ result_type="bar<int>",
+ result_children=[ValueCheck(value="47", name="a", type="int")],
+ )
+
+ def test(self):
+ self.do_test()
+
+ @no_debug_info_test
+ @skipIfDarwin
+ @skipIf(compiler=no_match("clang"))
+ @skipIf(compiler_version=["<", "8.0"])
+ @expectedFailureAll(oslist=["windows"])
+ def test_debug_names(self):
+ """Test that we are able to find complete types when using DWARF v5
+ accelerator tables"""
+ self.do_test(dict(CFLAGS_EXTRAS="-gdwarf-5 -gpubnames"))
+
+ @no_debug_info_test
+ @skipIf(compiler=no_match("clang"))
+ def test_simple_template_names(self):
+ """Test that we are able to find complete types when using DWARF v5
+ accelerator tables"""
+ self.do_test(dict(CFLAGS_EXTRAS="-gsimple-template-names"))
diff --git a/lldb/test/API/lang/cpp/forward/foo.cpp b/lldb/test/API/lang/cpp/forward/foo.cpp
new file mode 100644
index 00000000000000..6a2f0a49c4b62e
--- /dev/null
+++ b/lldb/test/API/lang/cpp/forward/foo.cpp
@@ -0,0 +1,3 @@
+#include "foo.h"
+
+int foo(bar<int> *bar_ptr) { return 1; }
diff --git a/lldb/test/API/lang/cpp/forward/foo.h b/lldb/test/API/lang/cpp/forward/foo.h
new file mode 100644
index 00000000000000..a80b39f03febcc
--- /dev/null
+++ b/lldb/test/API/lang/cpp/forward/foo.h
@@ -0,0 +1,3 @@
+template <typename T> struct bar;
+
+LLDB_TEST_API int foo(bar<int> *bar_ptr);
diff --git a/lldb/test/API/lang/cpp/forward/main.cpp b/lldb/test/API/lang/cpp/forward/main.cpp
new file mode 100644
index 00000000000000..c47036b9aa0a7c
--- /dev/null
+++ b/lldb/test/API/lang/cpp/forward/main.cpp
@@ -0,0 +1,13 @@
+#include "foo.h"
+
+template <typename T> struct bar {
+ T a;
+};
+
+int main() {
+ bar<int> b{47};
+
+ foo(&b);
+
+ return 0;
+}
|
There was a problem hiding this comment.
Very interesting find, I think this makes sense since the FindTypes query expects to find template parameters if we're looking up templates. And given this API is used specifically to perform these kinds of queries, this LGTM. But please confirm if my understanding is correct:
In the non-simple names case, when we stop at the breakpoint and start formatting the function arguments, LLDB creates a CXXRecordDecl with the name bar<int> in the shared library TypeSystem because it only sees a DW_TAG_structure_type "bar<int>" forward declaration without any template parameter children DIEs. Then when we call FindCompleteType, the Decls DeclarationName would've been set to bar<int>, so the TypeQuery will be able to match the bar<int> definition using the index. When FindTypes resolves the definition DIE we get from the index, it triggers the creation of a ClassTemplateSpecializationDecl in the main module's TypeSystem and everything works out.
But in the simple names case we create a ClassTemplateSpecializationDecl in the shared library module (as opposed to a plain CXXRecordDecl), because the forward declaration with -gsimple-template-names has a template parameter DIE. But that declaration name is bar, because that's what DWARF told us the DeclarationName of this ClassTemplateSpecializationDecl was. But then FindTypes doesn't know what to do. It finds the bar definition DIE from the index, but we actually end up not doing a simple-template-names lookup because UpdateCompilerContextForSimpleTemplateNames is only true if the query contained template parameters.
So I guess my question is, why is a TypeQuery by basename without template parameters not supported if we compiled with -gsimple-template-names? Despite us being able to find the definition DIE in the index? I guess the idea of UpdateCompilerContextForSimpleTemplateNames was to avoid doing a double-lookup if the first FindTypes failed for a template basename? (I vaguely remember this coming up on the original review)
| @skipIf(compiler=no_match("clang")) | ||
| def test_simple_template_names(self): | ||
| """Test that we are able to find complete types when using DWARF v5 | ||
| accelerator tables""" |
That is all correct.
I think the problem here is of a more fundamental nature. As far as i know, this is all WAI. Existing FindType callers expect to get a specific type as a result of their query -- not a collection of instantiations of that template. (That's definitely true in this case, where returning the collection of instantiations would just push the burden of filtering them onto the caller.) That's not to say this kind of a template search is not useful. Having something like that would go a long way towards making expressions like |
Makes sense, thanks for elaborating LGTM The |
I ran into this while look at a different bug (patch coming soon). This function has only two callers. The first is SBTypeStaticField::GetName (which doesn't care about templates), and the other is CompilerDecl::GetCompilerContext (in the TypeQuery constructor), which does want template arguments.
This function was (normally) returning the name without template args. Since this code is only used when looking up a type in another shared library, the odds of running into this bug are relatively low, but I add a test to demonstrate the scenario and the fix for it nonetheless.
Amazingly (and scarily), this test actually passes without this change in the default configuration -- and only fails with -gsimple-template-names. The reason for that is that in the non-simplified case we create a regular CXXRecordDecl whose name is "bar" (instead of a template record "foo" with an argument of "int"). When evaluating the expression, we are somehow able to replace this with a proper template specialization decl.