-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Fix scope of typedefs present inside a template class #146729
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
Conversation
|
LGTM |
|
Fixes #91451 |
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-debuginfo Author: ykhatav (ykhatav) ChangesWhen a typedef is declared within a templated class, clang incorrectly assigns the typedef to the compilation unit (CU) scope rather than the intended scope of the templated class. This issue arises because, during the creation of the typedef, the context lookup in the RegionMap fails to locate the templated class, despite its prior creation. Full diff: https://github.com/llvm/llvm-project/pull/146729.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index ee5e3d68a5ffa..8be9a296a9e4b 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4170,8 +4170,14 @@ llvm::DICompositeType *CGDebugInfo::CreateLimitedType(const RecordType *Ty) {
llvm::MDNode::replaceWithDistinct(llvm::TempDICompositeType(RealDecl));
break;
}
-
- RegionMap[Ty->getDecl()].reset(RealDecl);
+ auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(Ty->getDecl());
+ if (CTSD) {
+ CXXRecordDecl *TemplateDecl =
+ CTSD->getSpecializedTemplate()->getTemplatedDecl();
+ RegionMap[TemplateDecl].reset(RealDecl);
+ } else {
+ RegionMap[Ty->getDecl()].reset(RealDecl);
+ }
TypeCache[QualType(Ty, 0).getAsOpaquePtr()].reset(RealDecl);
if (const auto *TSpecial = dyn_cast<ClassTemplateSpecializationDecl>(RD))
diff --git a/clang/test/CodeGenCXX/dependent-template-type-scope.cpp b/clang/test/CodeGenCXX/dependent-template-type-scope.cpp
new file mode 100644
index 0000000000000..25a4d8741b01e
--- /dev/null
+++ b/clang/test/CodeGenCXX/dependent-template-type-scope.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -debug-info-kind=standalone -o - %s | FileCheck %s
+
+template <typename T = int>
+struct Y {
+ typedef int outside;
+ outside o;
+};
+
+Y<> y;
+
+// CHECK: ![[Y:.*]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Y<int>", {{.*}}identifier: "_ZTS1YIiE")
+// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "outside", scope: ![[Y]],
|
|
@llvm/pr-subscribers-clang Author: ykhatav (ykhatav) ChangesWhen a typedef is declared within a templated class, clang incorrectly assigns the typedef to the compilation unit (CU) scope rather than the intended scope of the templated class. This issue arises because, during the creation of the typedef, the context lookup in the RegionMap fails to locate the templated class, despite its prior creation. Full diff: https://github.com/llvm/llvm-project/pull/146729.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index ee5e3d68a5ffa..8be9a296a9e4b 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4170,8 +4170,14 @@ llvm::DICompositeType *CGDebugInfo::CreateLimitedType(const RecordType *Ty) {
llvm::MDNode::replaceWithDistinct(llvm::TempDICompositeType(RealDecl));
break;
}
-
- RegionMap[Ty->getDecl()].reset(RealDecl);
+ auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(Ty->getDecl());
+ if (CTSD) {
+ CXXRecordDecl *TemplateDecl =
+ CTSD->getSpecializedTemplate()->getTemplatedDecl();
+ RegionMap[TemplateDecl].reset(RealDecl);
+ } else {
+ RegionMap[Ty->getDecl()].reset(RealDecl);
+ }
TypeCache[QualType(Ty, 0).getAsOpaquePtr()].reset(RealDecl);
if (const auto *TSpecial = dyn_cast<ClassTemplateSpecializationDecl>(RD))
diff --git a/clang/test/CodeGenCXX/dependent-template-type-scope.cpp b/clang/test/CodeGenCXX/dependent-template-type-scope.cpp
new file mode 100644
index 0000000000000..25a4d8741b01e
--- /dev/null
+++ b/clang/test/CodeGenCXX/dependent-template-type-scope.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -debug-info-kind=standalone -o - %s | FileCheck %s
+
+template <typename T = int>
+struct Y {
+ typedef int outside;
+ outside o;
+};
+
+Y<> y;
+
+// CHECK: ![[Y:.*]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Y<int>", {{.*}}identifier: "_ZTS1YIiE")
+// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "outside", scope: ![[Y]],
|
|
Ping! |
|
I don't really feel comfortable reviewing Clang patches, living mostly on the LLVM side of things. @dwblaikie do you know who could take a look? Edit: Just noticed that @stevemerr you gave an LGTM but didn't approve it, was that intentional? |
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.
Seems reasonable, since this is what we already do for most typedefs. Looks like this is only an issue if the typedef is not dependent on the template parameter. E.g., LLDB relies on the typedefs to be scoped inside the structure for some of the STL data-formatters, but all of those are scoped correctly because they are template-parameter dependent.
What I'm slightly confused about is why it works when the typedef is template-dependent. It looks like the DeclContext for the typedef in this case is the ClassTemplateSpecializationDecl, which we put in the RegionMap. But if the typedef is not template-dependent, then the context is the CXXRecordDecl. That's probably just how the Clang AST works (seems like a fairly fundamental property), but would be nice to confirm that this is not an AST issue.
Side-note, does DW_TAG_template_alias suffer from the same issue?
|
Yea this even works when the typedef is not template-dependent but used outside of the class only: Again, in this case the |
|
(yeah, it'd be nice if there's a way for this to fall out more naturally, so that dependent typedefs and nondependent ones follow the same codepath) |
I left comments for the initial draft. I don't have approval authority. |
I think the main difference stems from when Clang processes the typedef type. In the earlier test case, while processing the members of a struct, we encounter the typedef during the creation of the type for the member variable "outside." During this typedef creation, the scope is determined based on the typedef's context, which is a CXXRecordDecl (while in the RegionMap, we store ClassTemplateSpecializationDecl). In the example above, however, the typedef is created while emitting the global variable "g," and the context for the typedef is a ClassTemplateSpecializationDecl, which can be found in the RegionMap, thereby setting the correct scope for the typedef. |
This should also be included in the summary of the PR. |
shafik
left a comment
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.
This should have a release note.
It does not look like template_alias has the same issue, although I am not very familiar with template_alias types. I tried a small example where the scope is set correctly: |
|
@Michael137 - did I answer your question? |
|
gentle ping! |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
When a typedef is declared within a templated class, clang incorrectly assigns the typedef to the compilation unit (CU) scope rather than the intended scope of the templated class. This issue arises because, during the creation of the typedef, the context lookup in the RegionMap fails to locate the templated class, despite its prior creation.
The problem stems from the way the context is stored in the RegionMap. When handling templated types, the current implementation stores the class specialization rather than the templated declaration itself. This leads to a mismatch when attempting to retrieve the context for the typedef.
To address this issue, the solution involves modifying the CreatedLimitedType() function. Specifically, when a struct or class is a templated type, we should store the actual templated declaration in the RegionMap instead of the class specialization. This ensures that subsequent lookups for context, such as those needed for typedef declarations, correctly identify the templated class scope.
Fixes #91451