This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb8567559cf38: [lldb] Make the ClassTemplateDecl merging
logic in TypeSystemClang respect… (authored by teemperor).
Herald added a subscriber: lldb-commits.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100662/new/
https://reviews.llvm.org/D100662
Files:
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
lldb/test/API/lang/cpp/forward-declared-template-specialization/Makefile
lldb/test/API/lang/cpp/forward-declared-template-specialization/TestCppForwardDeclaredTemplateSpecialization.py
lldb/test/API/lang/cpp/forward-declared-template-specialization/main.cpp
lldb/test/API/lang/cpp/incompatible-class-templates/Makefile
lldb/test/API/lang/cpp/incompatible-class-templates/TestCppIncompatibleClassTemplates.py
lldb/test/API/lang/cpp/incompatible-class-templates/main.cpp
lldb/test/API/lang/cpp/incompatible-class-templates/other.cpp
lldb/unittests/Symbol/TestTypeSystemClang.cpp
Index: lldb/unittests/Symbol/TestTypeSystemClang.cpp
===================================================================
--- lldb/unittests/Symbol/TestTypeSystemClang.cpp
+++ lldb/unittests/Symbol/TestTypeSystemClang.cpp
@@ -515,6 +515,187 @@
}
}
+class TestCreateClassTemplateDecl : public TestTypeSystemClang {
+protected:
+ /// The class templates created so far by the Expect* functions below.
+ llvm::DenseSet<ClassTemplateDecl *> m_created_templates;
+
+ /// Utility function for creating a class template.
+ ClassTemplateDecl *
+ CreateClassTemplate(const TypeSystemClang::TemplateParameterInfos &infos) {
+ ClassTemplateDecl *decl = m_ast->CreateClassTemplateDecl(
+ m_ast->GetTranslationUnitDecl(), OptionalClangModuleID(), eAccessPublic,
+ "foo", TTK_Struct, infos);
+ return decl;
+ }
+
+ /// Creates a new class template with the given template parameters.
+ /// Asserts that a new ClassTemplateDecl is created.
+ /// \param description The gtest scope string that should describe the input.
+ /// \param infos The template parameters that the class template should have.
+ /// \returns The created ClassTemplateDecl.
+ ClassTemplateDecl *
+ ExpectNewTemplate(std::string description,
+ const TypeSystemClang::TemplateParameterInfos &infos) {
+ SCOPED_TRACE(description);
+ ClassTemplateDecl *first_template = CreateClassTemplate(infos);
+ // A new template should have been created.
+ EXPECT_FALSE(m_created_templates.contains(first_template))
+ << "Didn't create new class template but reused this existing decl:\n"
+ << ClangUtil::DumpDecl(first_template);
+ m_created_templates.insert(first_template);
+
+ // Creating a new template with the same arguments should always return
+ // the template created above.
+ ClassTemplateDecl *second_template = CreateClassTemplate(infos);
+ EXPECT_EQ(first_template, second_template)
+ << "Second attempt to create class template didn't reuse first decl:\n"
+ << ClangUtil::DumpDecl(first_template) << "\nInstead created/reused:\n"
+ << ClangUtil::DumpDecl(second_template);
+ return first_template;
+ }
+
+ /// Tries to create a new class template but asserts that an existing class
+ /// template in the current AST is reused (in contract so a new class
+ /// template being created).
+ /// \param description The gtest scope string that should describe the input.
+ /// \param infos The template parameters that the class template should have.
+ void
+ ExpectReusedTemplate(std::string description,
+ const TypeSystemClang::TemplateParameterInfos &infos,
+ ClassTemplateDecl *expected) {
+ SCOPED_TRACE(description);
+ ClassTemplateDecl *td = CreateClassTemplate(infos);
+ EXPECT_EQ(td, expected)
+ << "Created/reused class template is:\n"
+ << ClangUtil::DumpDecl(td) << "\nExpected to reuse:\n"
+ << ClangUtil::DumpDecl(expected);
+ }
+};
+
+TEST_F(TestCreateClassTemplateDecl, FindExistingTemplates) {
+ // This tests the logic in TypeSystemClang::CreateClassTemplateDecl that
+ // decides whether an existing ClassTemplateDecl in the AST can be reused.
+ // The behaviour should follow the C++ rules for redeclaring templates
+ // (e.g., parameter names can be changed/omitted.)
+
+ // This describes a class template *instantiation* from which we will infer
+ // the structure of the class template.
+ TypeSystemClang::TemplateParameterInfos infos;
+
+ // Test an empty template parameter list: <>
+ ExpectNewTemplate("<>", infos);
+
+ // Test that <typename T> with T = int creates a new template.
+ infos.names = {"T"};
+ infos.args = {TemplateArgument(m_ast->getASTContext().IntTy)};
+ ClassTemplateDecl *single_type_arg = ExpectNewTemplate("<typename T>", infos);
+
+ // Test that changing the parameter name doesn't create a new class template.
+ infos.names = {"A"};
+ ExpectReusedTemplate("<typename A> (A = int)", infos, single_type_arg);
+
+ // Test that changing the used type doesn't create a new class template.
+ infos.args = {TemplateArgument(m_ast->getASTContext().FloatTy)};
+ ExpectReusedTemplate("<typename A> (A = float)", infos, single_type_arg);
+
+ // Test that <typename A, signed char I> creates a new template with A = int
+ // and I = 47;
+ infos.names.push_back("I");
+ infos.args.push_back(TemplateArgument(m_ast->getASTContext(),
+ llvm::APSInt(llvm::APInt(8, 47)),
+ m_ast->getASTContext().SignedCharTy));
+ ClassTemplateDecl *type_and_char_value =
+ ExpectNewTemplate("<typename A, signed char I> (I = 47)", infos);
+
+ // Change the value of the I parameter to 123. The previously created
+ // class template should still be reused.
+ infos.args.pop_back();
+ infos.args.push_back(TemplateArgument(m_ast->getASTContext(),
+ llvm::APSInt(llvm::APInt(8, 123)),
+ m_ast->getASTContext().SignedCharTy));
+ ExpectReusedTemplate("<typename A, signed char I> (I = 123)", infos,
+ type_and_char_value);
+
+ // Change the type of the I parameter to int so we have <typename A, int I>.
+ // The class template from above can't be reused.
+ infos.args.pop_back();
+ infos.args.push_back(TemplateArgument(m_ast->getASTContext(),
+ llvm::APSInt(llvm::APInt(32, 47)),
+ m_ast->getASTContext().IntTy));
+ ExpectNewTemplate("<typename A, int I> (I = 123)", infos);
+
+ // Test a second type parameter will also cause a new template to be created.
+ // We now have <typename A, int I, typename B>.
+ infos.names.push_back("B");
+ infos.args.push_back(TemplateArgument(m_ast->getASTContext().IntTy));
+ ClassTemplateDecl *type_and_char_value_and_type =
+ ExpectNewTemplate("<typename A, int I, typename B>", infos);
+
+ // Remove all the names from the parameters which shouldn't influence the
+ // way the templates get merged.
+ infos.names = {"", "", ""};
+ ExpectReusedTemplate("<typename, int, typename>", infos,
+ type_and_char_value_and_type);
+}
+
+TEST_F(TestCreateClassTemplateDecl, FindExistingTemplatesWithParameterPack) {
+ // The same as FindExistingTemplates but for templates with parameter packs.
+
+ TypeSystemClang::TemplateParameterInfos infos;
+ infos.packed_args =
+ std::make_unique<TypeSystemClang::TemplateParameterInfos>();
+ infos.packed_args->names = {"", ""};
+ infos.packed_args->args = {TemplateArgument(m_ast->getASTContext().IntTy),
+ TemplateArgument(m_ast->getASTContext().IntTy)};
+ ClassTemplateDecl *type_pack =
+ ExpectNewTemplate("<typename ...> (int, int)", infos);
+
+ // Special case: An instantiation for a parameter pack with no values fits
+ // to whatever class template we find. There isn't enough information to
+ // do an actual comparison here.
+ infos.packed_args =
+ std::make_unique<TypeSystemClang::TemplateParameterInfos>();
+ ExpectReusedTemplate("<...> (no values in pack)", infos, type_pack);
+
+ // Change the type content of pack type values.
+ infos.packed_args->names = {"", ""};
+ infos.packed_args->args = {TemplateArgument(m_ast->getASTContext().IntTy),
+ TemplateArgument(m_ast->getASTContext().LongTy)};
+ ExpectReusedTemplate("<typename ...> (int, long)", infos, type_pack);
+
+ // Change the number of pack values.
+ infos.packed_args->args = {TemplateArgument(m_ast->getASTContext().IntTy)};
+ ExpectReusedTemplate("<typename ...> (int)", infos, type_pack);
+
+ // The names of the pack values shouldn't matter.
+ infos.packed_args->names = {"A", "B"};
+ ExpectReusedTemplate("<typename ...> (int)", infos, type_pack);
+
+ // Changing the kind of template argument will create a new template.
+ infos.packed_args->args = {TemplateArgument(m_ast->getASTContext(),
+ llvm::APSInt(llvm::APInt(32, 1)),
+ m_ast->getASTContext().IntTy)};
+ ClassTemplateDecl *int_pack = ExpectNewTemplate("<int ...> (int = 1)", infos);
+
+ // Changing the value of integral parameters will not create a new template.
+ infos.packed_args->args = {TemplateArgument(
+ m_ast->getASTContext(), llvm::APSInt(llvm::APInt(32, 123)),
+ m_ast->getASTContext().IntTy)};
+ ExpectReusedTemplate("<int ...> (int = 123)", infos, int_pack);
+
+ // Changing the integral type will create a new template.
+ infos.packed_args->args = {TemplateArgument(m_ast->getASTContext(),
+ llvm::APSInt(llvm::APInt(64, 1)),
+ m_ast->getASTContext().LongTy)};
+ ExpectNewTemplate("<long ...> (long = 1)", infos);
+
+ // Prependinding a non-pack parameter will create a new template.
+ infos.names = {"T"};
+ infos.args = {TemplateArgument(m_ast->getASTContext().IntTy)};
+ ExpectNewTemplate("<typename T, long...> (T = int, long = 1)", infos);
+}
+
TEST_F(TestTypeSystemClang, OnlyPackName) {
TypeSystemClang::TemplateParameterInfos infos;
infos.pack_name = "A";
Index: lldb/test/API/lang/cpp/incompatible-class-templates/other.cpp
===================================================================
--- /dev/null
+++ lldb/test/API/lang/cpp/incompatible-class-templates/other.cpp
@@ -0,0 +1,7 @@
+namespace {
+template <typename T1, typename T2> struct Temp { int x; };
+// This emits the 'Temp' template from this TU.
+Temp<int, float> Template2;
+} // namespace
+
+int other() { return Template2.x; }
Index: lldb/test/API/lang/cpp/incompatible-class-templates/main.cpp
===================================================================
--- /dev/null
+++ lldb/test/API/lang/cpp/incompatible-class-templates/main.cpp
@@ -0,0 +1,11 @@
+int other();
+
+namespace {
+template <typename T1> struct Temp { int x; };
+// This emits the 'Temp' template in this TU.
+Temp<float> Template1;
+} // namespace
+
+int main() {
+ return Template1.x + other(); // break here
+}
Index: lldb/test/API/lang/cpp/incompatible-class-templates/TestCppIncompatibleClassTemplates.py
===================================================================
--- /dev/null
+++ lldb/test/API/lang/cpp/incompatible-class-templates/TestCppIncompatibleClassTemplates.py
@@ -0,0 +1,19 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+ mydir = TestBase.compute_mydir(__file__)
+
+ def test(self):
+ """
+ Test debugging a binary that has two templates with the same name
+ but different template parameters.
+ """
+ self.build()
+ lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.cpp"))
+
+ # Try using both templates in the same expression. This shouldn't crash.
+ self.expect_expr("Template1.x + Template2.x", result_type="int")
Index: lldb/test/API/lang/cpp/incompatible-class-templates/Makefile
===================================================================
--- /dev/null
+++ lldb/test/API/lang/cpp/incompatible-class-templates/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp other.cpp
+
+include Makefile.rules
Index: lldb/test/API/lang/cpp/forward-declared-template-specialization/main.cpp
===================================================================
--- /dev/null
+++ lldb/test/API/lang/cpp/forward-declared-template-specialization/main.cpp
@@ -0,0 +1,16 @@
+// Forward declare a template and a specialization;
+template <typename T> class Temp;
+template <> class Temp<int>;
+
+// Force that debug informatin for the specialization is emitted.
+// Clang and GCC will create debug information that lacks any description
+// of the template argument 'int'.
+Temp<int> *a;
+
+// Define the template and create an implicit instantiation.
+template <typename T> class Temp { int f; };
+Temp<float> b;
+
+int main() {
+ return 0; // break here
+}
Index: lldb/test/API/lang/cpp/forward-declared-template-specialization/TestCppForwardDeclaredTemplateSpecialization.py
===================================================================
--- /dev/null
+++ lldb/test/API/lang/cpp/forward-declared-template-specialization/TestCppForwardDeclaredTemplateSpecialization.py
@@ -0,0 +1,19 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+ mydir = TestBase.compute_mydir(__file__)
+
+ def test(self):
+ """
+ Tests a forward declared template and a normal template in the same
+ executable. GCC/Clang emit very limited debug information for forward
+ declared templates that might trip up LLDB.
+ """
+ self.build()
+ lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.cpp"))
+
+ self.expect_expr("a; b", result_type="Temp<float>")
Index: lldb/test/API/lang/cpp/forward-declared-template-specialization/Makefile
===================================================================
--- /dev/null
+++ lldb/test/API/lang/cpp/forward-declared-template-specialization/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===================================================================
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -328,6 +328,8 @@
(!packed_args || !packed_args->packed_args);
}
+ bool hasParameterPack() const { return static_cast<bool>(packed_args); }
+
llvm::SmallVector<const char *, 2> names;
llvm::SmallVector<clang::TemplateArgument, 2> args;
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===================================================================
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -1357,9 +1357,11 @@
}
namespace {
- bool IsValueParam(const clang::TemplateArgument &argument) {
- return argument.getKind() == TemplateArgument::Integral;
- }
+/// Returns true iff the given TemplateArgument should be represented as an
+/// NonTypeTemplateParmDecl in the AST.
+bool IsValueParam(const clang::TemplateArgument &argument) {
+ return argument.getKind() == TemplateArgument::Integral;
+}
}
static TemplateParameterList *CreateTemplateParameterList(
@@ -1463,6 +1465,99 @@
template_args_ptr, nullptr);
}
+/// Returns true if the given template parameter can represent the given value.
+/// For example, `typename T` can represent `int` but not integral values such
+/// as `int I = 3`.
+static bool TemplateParameterAllowsValue(NamedDecl *param,
+ const TemplateArgument &value) {
+ if (auto *type_param = llvm::dyn_cast<TemplateTypeParmDecl>(param)) {
+ // Compare the argument kind, i.e. ensure that <typename> != <int>.
+ if (value.getKind() != TemplateArgument::Type)
+ return false;
+ } else if (auto *type_param =
+ llvm::dyn_cast<NonTypeTemplateParmDecl>(param)) {
+ // Compare the argument kind, i.e. ensure that <typename> != <int>.
+ if (!IsValueParam(value))
+ return false;
+ // Compare the integral type, i.e. ensure that <int> != <char>.
+ if (type_param->getType() != value.getIntegralType())
+ return false;
+ } else {
+ // There is no way to create other parameter decls at the moment, so we
+ // can't reach this case during normal LLDB usage. Log that this happened
+ // and assert.
+ Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+ LLDB_LOG(log,
+ "Don't know how to compare template parameter to passed"
+ " value. Decl kind of parameter is: {0}",
+ param->getDeclKindName());
+ lldbassert(false && "Can't compare this TemplateParmDecl subclass");
+ // In release builds just fall back to marking the parameter as not
+ // accepting the value so that we don't try to fit an instantiation to a
+ // template that doesn't fit. E.g., avoid that `S<1>` is being connected to
+ // `template<typename T> struct S;`.
+ return false;
+ }
+ return true;
+}
+
+/// Returns true if the given class template declaration could produce an
+/// instantiation with the specified values.
+/// For example, `<typename T>` allows the arguments `float`, but not for
+/// example `bool, float` or `3` (as an integer parameter value).
+static bool ClassTemplateAllowsToInstantiationArgs(
+ ClassTemplateDecl *class_template_decl,
+ const TypeSystemClang::TemplateParameterInfos &instantiation_values) {
+
+ TemplateParameterList ¶ms = *class_template_decl->getTemplateParameters();
+
+ // Save some work by iterating only once over the found parameters and
+ // calculate the information related to parameter packs.
+
+ // Contains the first pack parameter (or non if there are none).
+ llvm::Optional<NamedDecl *> pack_parameter;
+ // Contains the number of non-pack parameters.
+ size_t non_pack_params = params.size();
+ for (size_t i = 0; i < params.size(); ++i) {
+ NamedDecl *param = params.getParam(i);
+ if (param->isParameterPack()) {
+ pack_parameter = param;
+ non_pack_params = i;
+ break;
+ }
+ }
+
+ // The found template needs to have compatible non-pack template arguments.
+ // E.g., ensure that <typename, typename> != <typename>.
+ // The pack parameters are compared later.
+ if (non_pack_params != instantiation_values.args.size())
+ return false;
+
+ // Ensure that <typename...> != <typename>.
+ if (pack_parameter.hasValue() != instantiation_values.hasParameterPack())
+ return false;
+
+ // Compare the first pack parameter that was found with the first pack
+ // parameter value. The special case of having an empty parameter pack value
+ // always fits to a pack parameter.
+ // E.g., ensure that <int...> != <typename...>.
+ if (pack_parameter && !instantiation_values.packed_args->args.empty() &&
+ !TemplateParameterAllowsValue(
+ *pack_parameter, instantiation_values.packed_args->args.front()))
+ return false;
+
+ // Compare all the non-pack parameters now.
+ // E.g., ensure that <int> != <long>.
+ for (const auto pair : llvm::zip_first(instantiation_values.args, params)) {
+ const TemplateArgument &passed_arg = std::get<0>(pair);
+ NamedDecl *found_param = std::get<1>(pair);
+ if (!TemplateParameterAllowsValue(found_param, passed_arg))
+ return false;
+ }
+
+ return class_template_decl;
+}
+
ClassTemplateDecl *TypeSystemClang::CreateClassTemplateDecl(
DeclContext *decl_ctx, OptionalClangModuleID owning_module,
lldb::AccessType access_type, const char *class_name, int kind,
@@ -1476,12 +1571,22 @@
IdentifierInfo &identifier_info = ast.Idents.get(class_name);
DeclarationName decl_name(&identifier_info);
+ // Search the AST for an existing ClassTemplateDecl that could be reused.
clang::DeclContext::lookup_result result = decl_ctx->lookup(decl_name);
-
for (NamedDecl *decl : result) {
class_template_decl = dyn_cast<clang::ClassTemplateDecl>(decl);
- if (class_template_decl)
- return class_template_decl;
+ if (!class_template_decl)
+ continue;
+ // The class template has to be able to represents the instantiation
+ // values we received. Without this we might end up putting an instantiation
+ // with arguments such as <int, int> to a template such as:
+ // template<typename T> struct S;
+ // Connecting the instantiation to an incompatible template could cause
+ // problems later on.
+ if (!ClassTemplateAllowsToInstantiationArgs(class_template_decl,
+ template_param_infos))
+ continue;
+ return class_template_decl;
}
llvm::SmallVector<NamedDecl *, 8> template_param_decls;
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
