Page MenuHomePhabricator

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

Authored by probinson on Nov 4 2015, 4:26 PM.

Diff Detail

Event Timeline

probinson updated this revision to Diff 39286.Nov 4 2015, 4:26 PM
probinson retitled this revision from to DWARF's forward decl of a template should have template parameters..
probinson updated this object.
probinson added reviewers: echristo, dblaikie, aprantl.
probinson added a subscriber: cfe-commits.

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

aprantl edited edge metadata.Nov 4 2015, 5:12 PM

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?

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.

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:dblaikie@gmail.com]
Sent: Wednesday, November 04, 2015 8:30 PM
To: reviews+D14358+public+d3104135076f0a10@reviews.llvm.org; Robinson, Paul
Subject: Re: [PATCH] D14358: DWARF's forward decl of a template should have template parameters.

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

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

probinson updated this revision to Diff 116711.Sep 26 2017, 2:21 PM

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. :-)

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?

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 edited edge metadata.Sep 27 2017, 11:21 AM

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
7–18

Probably simpler:

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

?

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.

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

dblaikie accepted this revision.Sep 27 2017, 1:41 PM

Looks OK to me - couple of minor questions.

include/clang/Frontend/CodeGenOptions.def
222

Maybe 'Decl' rather than 'Fwd'.

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

Any particular reason for const int, rather than int?

This revision is now accepted and ready to land.Sep 27 2017, 1:41 PM

+rnk for the CodeView question.

include/clang/Frontend/CodeGenOptions.def
222

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

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

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

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.

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.

This revision was automatically updated to reflect the committed changes.