DWARF's forward decl of a template should have template parameters.

[probinson]

Summary

We were forgetting the template parameters on the forward declaration of a template class.

[probinson]

In debug-info-template-member.cpp, some things came out in a different order; that's the only change there.

[aprantl]

Unless this is some kind of optimization that we specifically added to minimize debug info size (I have never looked at our template support in detail), this looks totally reasonable. Do other compilers do the same here?

[probinson]

GCC 4.8.4 on Linux doesn't produce these, but DWARF 4 section 5.5.8 says a class template instantiation is just like the equivalent non-template class entry, with the exception of the template parameter entries. I read that as meaning an incomplete description (i.e. with DW_AT_declaration) lets you omit all the other children, but not the template parameters.

I don't think omitting the template DIEs was an intentional optimization, in the sense of being a decision separate from deciding to emit the incomplete/forward declaration in the first place. They were just omitted because we were omitting everything, but everything turns out to be non-compliant.

[cfe-commits]

Would citing PR20455 help? It wasn't actually my primary motivation but it's not too far off. Having the template parameters there lets you know what's going on in the DWARF, without having to fetch and parse the name string of every struct you come across. Actually I'm not sure parsing the name string is unambiguous either; each parameter is either a typename, or an expression, but without the parameter DIEs you don't know which, a-priori. (What does <foo> mean? Depends on whether you think it should be a type name or a value; you can't tell, syntactically, you have to do some lookups. Ah, but if you had the parameter DIEs, you would Just Know.)

Choosing to emit a forward/incomplete declaration in the first place fails source fidelity, but it is a practical engineering tradeoff of compile/link performance against utility; and, after all, the source *could* have been written that way, with no semantic difference. But, if we're going to emit a white-lie incomplete declaration, we should do so correctly.
--paulr

P.S. We should talk about this forward-declaration tactic wrt LTO sometime. I have a case where a nested class got forward-declared; it's entirely conceivable that the outer class with the inner forward-declared class would end up being picked by LTO, leaving the user with no debug info for the inner class contents.

From: David Blaikie [mailto:[e-mail]]
Sent: Wednesday, November 04, 2015 8:30 PM
To: [e-mail]; Robinson, Paul
Subject: Re: [PATCH] D14358: DWARF's forward decl of a template should have template parameters.

• msg-18258-201.txt159 BDownload

[probinson]

Building a debug clang before and after this change and comparing the sum of .debug_* sizes, shows just a shade under 1% increase.

[probinson]

Refresh to current TOT, and ping. Funny what you can find in a year-old to-do list....

[probinson]

Add a command-line flag to control emitting the template parameter children. Default to on for SCE debugger tuning.
I am not super excited about my choice of option name or the help text; alternate suggestions welcome.

I would prefer to eliminate the <params> from the instance name as well, because our debugger reconstructs a name more to its liking from the parameter children. However, IIUC the name with params is used for deduplication in LTO, so that is probably not such a good idea. :-)

[aprantl]

Does this have to be exposed through the driver or could this be a cc1 option only?

include/clang/Basic/LangOptions.def

144 ↗

(On Diff #116711)

Why is this a LangOpt instead of a CodeGenOpt?
Should it reference debug somewhere in the name?

[probinson]

In D14358#881666, @aprantl wrote:

Does this have to be exposed through the driver or could this be a cc1 option only?

My thinking was to make it easier for LLDB to play with it and decide whether DWARF conformance on this point is a good thing for them also. But I admit it's not something an end user would really care about or want to change. I can make it a cc1 option.

include/clang/Basic/LangOptions.def

144 ↗

(On Diff #116711)

Because I thought EmitAllDecls was for debug info, and this felt related. But I see EmitAllDecls is not used in CGDebugInfo and generally we do put debug-related options in CodeGenOpt so I will move that. (Someday we should collect all that stuff into a DebugOpt class.)
And I will rename it to DebugFwdTemplateChildren.

[dblaikie]

I would prefer to eliminate the <params> from the instance name as well, because our debugger reconstructs a name more to its liking from the parameter children. However, IIUC the name with params is used for deduplication in LTO, so that is probably not such a good idea. :-)

Though you have this out of tree? How do you cope with LTO there?

I've not fully refreshed myself on the previous conversations - but it looks like my thought was that this state proposed here is weird/inconsistent: The parameters are already in the name, so adding them in the DIEs seems redundant. If the parameters weren't in the name then this change might make more sense.

test/CodeGenCXX/debug-info-fwd-template-param.cpp

6–17 ↗

(On Diff #116711)

Probably simpler:

template<typename T> class A;
A<int> *p;

?

[probinson]

In D14358#882445, @dblaikie wrote:

I would prefer to eliminate the <params> from the instance name as well, because our debugger reconstructs a name more to its liking from the parameter children. However, IIUC the name with params is used for deduplication in LTO, so that is probably not such a good idea. :-)

Though you have this out of tree? How do you cope with LTO there?

We discovered that we had to keep them in the name for LTO.

I've not fully refreshed myself on the previous conversations - but it looks like my thought was that this state proposed here is weird/inconsistent: The parameters are already in the name, so adding them in the DIEs seems redundant. If the parameters weren't in the name then this change might make more sense.

Our debugger throws away the params in the name, and relies on the children. The names as rendered in DWARF by Clang are not textually consistent with names as rendered by the demangler. Our debugger uses the children to construct names that are consistent with how the demangler works. Then it can match up type names returned by the demangler to type names it has constructed. Assuming I am not misunderstanding our debugger guys again, but that's my recollection.

I believe we have talked previously about using a different scheme for deduplication that doesn't depend (or not so much) on the names. If we had that, we could eliminate params from the name, and save probably a noticeable chunk of space in the string section. I haven't tried to measure that, though. But we have to have the children in place before we can experiment with other deduplication schemes.

There is also the pedantic point that DWARF doesn't say these child entries are optional, or omitted for forward declarations. I know gcc doesn't (or didn't, anyway; what I have locally is gcc 5.4) but gcc is not the arbiter of what constitutes conforming DWARF.

[probinson]

command-line option is cc1 not driver
internal flag moved from LangOpts to CodeGenOpts and renamed
simplified test

[dblaikie]

Looks OK to me - couple of minor questions.

include/clang/Frontend/CodeGenOptions.def

222 ↗

(On Diff #116862)

Maybe 'Decl' rather than 'Fwd'.

test/CodeGenCXX/debug-info-fwd-template-param.cpp

7 ↗

(On Diff #116862)

Any particular reason for const int, rather than int?

[probinson]

+rnk for the CodeView question.

include/clang/Frontend/CodeGenOptions.def

222 ↗

(On Diff #116862)

Well, in a sense they are all declarations, and 'Fwd' is a clearer statement of the distinction this flag is trying to make. Unless you feel strongly I'd prefer to leave it as is.

lib/CodeGen/CGDebugInfo.cpp

705

It just occurred to me... should CodeView care about this?

test/CodeGenCXX/debug-info-fwd-template-param.cpp

7 ↗

(On Diff #116862)

It was the illustrative example of the difference between the demangler ("int const") and clang ("const int") that the debugger guys tripped over, and so was in the source I started with when creating this test. I think you are correct, it is not important to have it.

[probinson]

I think I will go ahead and commit this; it doesn't change the status quo for CodeView and if something is warranted we can do that in a follow-up.

lib/CodeGen/CGDebugInfo.cpp

if (D && D->isCompleteDefinition()) {
     Align = CGM.getContext().getTypeAlign(Ty);
   }
 
   // Create the type.
   SmallString<256> FullName = getUniqueTagTypeName(Ty, CGM, TheCU);
   llvm::DICompositeType *RetTy = DBuilder.createReplaceableCompositeType(
       getTagForRecord(RD), RDName, Ctx, DefUnit, Line, 0, Size, Align,
       llvm::DINode::FlagFwdDecl, FullName);
+  if (const ClassTemplateSpecializationDecl *TSpecial =
+          dyn_cast<ClassTemplateSpecializationDecl>(RD))
+    DBuilder.replaceArrays(RetTy, llvm::DINodeArray(),
+                           CollectCXXTemplateParams(TSpecial, DefUnit));
   ReplaceMap.emplace_back(
       std::piecewise_construct, std::make_tuple(Ty),
       std::make_tuple(static_cast<llvm::Metadata *>(RetTy)));
   return RetTy;
 }
 
 llvm::DIType *CGDebugInfo::CreatePointerLikeType(llvm::dwarf::Tag Tag,
                                                  const Type *Ty,

test/CodeGenCXX/debug-info-template-fwd-param.cpp

+// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin -debug-info-kind=limited -emit-llvm -o - | FileCheck %s
+// A forward declaration of a template should still have template parameters.
+
+template<typename T> class A {
+public:
+  A(T val);
+private:
+  T x;
+};
+
+struct B {
+  A<const int> *p;
+};
+
+B b;
+
+// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A<const int>"
+// CHECK-SAME: flags: DIFlagFwdDecl
+// CHECK-SAME: templateParams: [[PARAM_LIST:![0-9]*]]
+// CHECK: [[PARAM_LIST]] = !{[[PARAM:![0-9]*]]}
+// CHECK: [[PARAM]] = !DITemplateTypeParameter(name: "T",
+// CHECK-SAME: type: [[CTYPE:![0-9]*]]
+// CHECK: [[CTYPE]] = !DIDerivedType(tag: DW_TAG_const_type
+// CHECK-SAME: baseType: [[BTYPE:![0-9]*]]
+// CHECK: [[BTYPE]] = !DIBasicType(name: "int"

test/CodeGenCXX/debug-info-template-member.cpp

 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "virt<elem>"
 // CHECK-SAME:             elements: [[VIRT_MEM:![0-9]*]]
 // CHECK-SAME:             vtableHolder: !"_ZTS4virtI4elemE"
 // CHECK-SAME:             templateParams: [[VIRT_TEMP_PARAM:![0-9]*]]
 // CHECK-SAME:             identifier: "_ZTS4virtI4elemE"
 // CHECK: [[VIRT_TEMP_PARAM]] = !{[[VIRT_T:![0-9]*]]}
 // CHECK: [[VIRT_T]] = !DITemplateTypeParameter(name: "T", type: !"_ZTS4elem")
 
+// CHECK: [[ELEM:![0-9]*]] = !DICompositeType(tag: DW_TAG_structure_type, name: "elem"
+// CHECK-SAME:                                elements: [[ELEM_MEM:![0-9]*]]
+// CHECK-SAME:                                identifier: "_ZTS4elem"
+// CHECK: [[ELEM_MEM]] = !{[[ELEM_X:![0-9]*]]}
+// CHECK: [[ELEM_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: !"_ZTS4elem"
+// CHECK-SAME:                        baseType: !"_ZTS4virtI4elemE"
+
 // CHECK: [[C:![0-9]*]] = !DICompositeType(tag: DW_TAG_structure_type, name: "MyClass"
 // CHECK-SAME:                             elements: [[C_MEM:![0-9]*]]
 // CHECK-SAME:                             vtableHolder: !"_ZTS7MyClass"
 // CHECK-SAME:                             identifier: "_ZTS7MyClass")
 // CHECK: [[C_MEM]] = !{[[C_VPTR:![0-9]*]], [[C_FUNC:![0-9]*]]}
 // CHECK: [[C_VPTR]] = !DIDerivedType(tag: DW_TAG_member, name: "_vptr$MyClass"
 
 // CHECK: [[C_FUNC]] = !DISubprogram(name: "func",{{.*}} line: 7,
 
-// CHECK: [[ELEM:![0-9]*]] = !DICompositeType(tag: DW_TAG_structure_type, name: "elem"
-// CHECK-SAME:                                elements: [[ELEM_MEM:![0-9]*]]
-// CHECK-SAME:                                identifier: "_ZTS4elem"
-// CHECK: [[ELEM_MEM]] = !{[[ELEM_X:![0-9]*]]}
-// CHECK: [[ELEM_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: !"_ZTS4elem"
-// CHECK-SAME:                        baseType: !"_ZTS4virtI4elemE"
-
 // Check that the member function template specialization and implicit special
 // members (the default ctor) refer to their class by scope, even though they
 // didn't appear in the class's member list (C_MEM). This prevents the functions
 // from being added to type units, while still appearing in the type
 // declaration/reference in the compile unit.
 // CHECK: !DISubprogram(name: "MyClass"
 // CHECK-SAME:          scope: !"_ZTS7MyClass"
 // CHECK: !DISubprogram(name: "add<2>"