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

836 ↗

(On Diff #116862)

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.

cfe/trunk/include/clang/Driver/CC1Options.td

def split_dwarf : Flag<["-"], "split-dwarf">,
   HelpText<"Split out the dwarf .dwo sections">;
 def gnu_pubnames : Flag<["-"], "gnu-pubnames">,
   HelpText<"Emit newer GNU style pubnames">;
 def arange_sections : Flag<["-"], "arange_sections">,
   HelpText<"Emit DWARF .debug_arange sections">;
 def dwarf_ext_refs : Flag<["-"], "dwarf-ext-refs">,
   HelpText<"Generate debug info with external references to clang modules"
            " or precompiled headers">;
+def debug_forward_template_params : Flag<["-"], "debug-forward-template-params">,
+  HelpText<"Emit complete descriptions of template parameters in forward"
+           " declarations">;
 def fforbid_guard_variables : Flag<["-"], "fforbid-guard-variables">,
   HelpText<"Emit an error if a C++ static local initializer would need a guard variable">;
 def no_implicit_float : Flag<["-"], "no-implicit-float">,
   HelpText<"Don't generate implicit floating point instructions">;
 def fdump_vtable_layouts : Flag<["-"], "fdump-vtable-layouts">,
   HelpText<"Dump the layouts of all vtables that will be emitted in a translation unit">;
 def fmerge_functions : Flag<["-"], "fmerge-functions">,
   HelpText<"Permit merging of identical functions when optimizing.">;

cfe/trunk/include/clang/Frontend/CodeGenOptions.def

 
 CODEGENOPT(DebugExplicitImport, 1, 0)  ///< Whether or not debug info should
                                        ///< contain explicit imports for
                                        ///< anonymous namespaces
 CODEGENOPT(EnableSplitDwarf, 1, 0) ///< Whether to enable split DWARF
 CODEGENOPT(SplitDwarfInlining, 1, 1) ///< Whether to include inlining info in the
                                      ///< skeleton CU to allow for symbolication
              ///< of inline stack frames without .dwo files.
+CODEGENOPT(DebugFwdTemplateParams, 1, 0) ///< Whether to emit complete
+                                         ///< template parameter descriptions in
+                                         ///< forward declarations (versus just
+                                         ///< including them in the name).
 
 CODEGENOPT(EmitLLVMUseLists, 1, 0) ///< Control whether to serialize use-lists.
 
 CODEGENOPT(WholeProgramVTables, 1, 0) ///< Whether to apply whole-program
                                       ///  vtable optimization.
 
 /// Whether to use public LTO visibility for entities in std and stdext
 /// namespaces. This is enabled by clang-cl's /MT and /MTd flags.

cfe/trunk/lib/CodeGen/CGDebugInfo.cpp

CGDebugInfo::getOrCreateRecordFwdDecl(const RecordType *Ty,
   uint64_t Size = 0;
   uint32_t Align = 0;
 
   // 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 (CGM.getCodeGenOpts().DebugFwdTemplateParams)
+    if (auto *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,

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

static void RenderDebugOptions(const ToolChain &TC, const Driver &D,
   }
 
   if (Args.hasFlag(options::OPT_fdebug_types_section,
                    options::OPT_fno_debug_types_section, false)) {
     CmdArgs.push_back("-backend-option");
     CmdArgs.push_back("-generate-type-units");
   }
 
+  // Decide how to render forward declarations of template instantiations.
+  // SCE wants full descriptions, others just get them in the name.
+  if (DebuggerTuning == llvm::DebuggerKind::SCE)
+    CmdArgs.push_back("-debug-forward-template-params");
+
   RenderDebugInfoCompressionArgs(Args, CmdArgs, D);
 }
 
 void Clang::ConstructJob(Compilation &C, const JobAction &JA,
                          const InputInfo &Output, const InputInfoList &Inputs,
                          const ArgList &Args, const char *LinkingOutput) const {
   const llvm::Triple &RawTriple = getToolChain().getTriple();
   const llvm::Triple &Triple = getToolChain().getEffectiveTriple();

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.MacroDebugInfo = Args.hasArg(OPT_debug_info_macro);
   Opts.WholeProgramVTables = Args.hasArg(OPT_fwhole_program_vtables);
   Opts.LTOVisibilityPublicStd = Args.hasArg(OPT_flto_visibility_public_std);
   Opts.EnableSplitDwarf = Args.hasArg(OPT_enable_split_dwarf);
   Opts.SplitDwarfFile = Args.getLastArgValue(OPT_split_dwarf_file);
   Opts.SplitDwarfInlining = !Args.hasArg(OPT_fno_split_dwarf_inlining);
   Opts.DebugTypeExtRefs = Args.hasArg(OPT_dwarf_ext_refs);
   Opts.DebugExplicitImport = Triple.isPS4CPU();
+  Opts.DebugFwdTemplateParams = Args.hasArg(OPT_debug_forward_template_params);
 
   for (const auto &Arg : Args.getAllArgValues(OPT_fdebug_prefix_map_EQ))
     Opts.DebugPrefixMap.insert(StringRef(Arg).split('='));
 
   if (const Arg *A =
           Args.getLastArg(OPT_emit_llvm_uselists, OPT_no_emit_llvm_uselists))
     Opts.EmitLLVMUseLists = A->getOption().getID() == OPT_emit_llvm_uselists;
 

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

+// RUN: %clang_cc1 %s -triple=%itanium_abi_triple -debug-info-kind=limited -debug-forward-template-params -emit-llvm -o - | FileCheck --check-prefix=CHILD %s
+// RUN: %clang_cc1 %s -triple=%itanium_abi_triple -debug-info-kind=limited -emit-llvm -o - | FileCheck --check-prefix=NONE %s
+// A DWARF forward declaration of a template instantiation should have template
+// parameter children (if we ask for them).
+
+template<typename T> class A;
+A<int> *p;
+
+// CHILD:      !DICompositeType(tag: DW_TAG_class_type, name: "A<int>"
+// CHILD-SAME:     flags: DIFlagFwdDecl
+// CHILD-SAME:     templateParams: [[PARAM_LIST:![0-9]*]]
+// CHILD:      [[PARAM_LIST]] = !{[[PARAM:![0-9]*]]}
+// CHILD:      [[PARAM]] = !DITemplateTypeParameter(name: "T",
+// CHILD-SAME:     type: [[BTYPE:![0-9]*]]
+// CHILD:      [[BTYPE]] = !DIBasicType(name: "int"
+
+// NONE:       !DICompositeType(tag: DW_TAG_class_type, name: "A<int>"
+// NONE-SAME:      flags: DIFlagFwdDecl
+// NONE-NOT:       templateParams:
+// NONE-SAME:      )