llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.27k stars 12.1k forks source link

Support DW_TAG_template_alias #54624

Closed pogo59 closed 7 months ago

pogo59 commented 2 years ago

While experimenting with -gsimple-template-names our debugger folks discovered the following interesting case.

template<typename T, typename U>
struct X {
  T m1;
  U m2;
};
template<typename V>
using Y = X<V, int>;
Y<int> y = {1, 2};

produces (edited for size):

0x0000001e:   DW_TAG_variable
                DW_AT_name  ("y")
                DW_AT_type  (0x00000029 "Y<int>")

0x00000029:   DW_TAG_typedef
                DW_AT_type  (0x00000031 "X<int, int>")
                DW_AT_name  ("Y<int>")

0x00000031:   DW_TAG_structure_type
                DW_AT_name  ("X") // without -gsimple-template-names, this is "X<int,int>"

0x00000037:     DW_TAG_template_type_parameter
                  DW_AT_type    (0x00000056 "int")
                  DW_AT_name    ("T")

0x0000003d:     DW_TAG_template_type_parameter
                  DW_AT_type    (0x00000056 "int")
                  DW_AT_name    ("U")

0x00000043:     DW_TAG_member
                  DW_AT_name    ("m1")
                  DW_AT_type    (0x00000056 "int")

0x0000004c:     DW_TAG_member
                  DW_AT_name    ("m2")
                  DW_AT_type    (0x00000056 "int")

The only difference with/without -gsimple-template-names is as noted. I'm a little surprised to see the typedef's DW_AT_type have a name with template parameters in it rather than just "X" but I guess llvm-dwarfdump does the reconstruction?

More to the point, in both cases we see DW_TAG_typedef with name Y<int> when it really seems like it ought to be DW_TAG_template_alias with name Y. The alias could then have template parameters, and the name simplification would work like expected.

I don't see any support in clang/llvm at all for DW_TAG_template_alias, which is mildly surprising as it goes back to DWARF v4. I guess we'd still have to emit DW_TAG_typedef for v2/v3.

llvmbot commented 2 years ago

@llvm/issue-subscribers-debuginfo

dwblaikie commented 2 years ago

I'm a little surprised to see the typedef's DW_AT_type have a name with template parameters in it rather than just "X" but I guess llvm-dwarfdump does the reconstruction?

Yep - it's an easy way to test the name reconstruction. This piggy backs on the fact that we were already doing some work to rebuild names of types in llvm-dwarfdump to make the dumps easier to read (eg: printing int* for a DW_AT_type that refers to a DW_TAG_pointer_type that refers to a DW_TAG_basic_type for int). The work to rebuild types for simplified template names improved this general name rebuilding - gave better/more complete function type pretty printing and various other similar things.

in both cases we see DW_TAG_typedef with name Y when it really seems like it ought to be DW_TAG_template_alias with name Y. The alias could then have template parameters, and the name simplification would work like expected.

Sure, sounds good to me. But yeah, as you say, currently the representation never has template parameters, so it's a bit of work to change the tag type, include parameters, and simplify the name. I wouldn't object to it, but it's not on my immediate list (it's not shown up as high impact for string size so far in the situations I'm trying to address, so far as I've seen).

OCHyams commented 8 months ago

Hi @dwblaikie & @pogo59 . I'm looking at implementing this - do either of you have an opinion on whether this should be default behaviour or tied to a debugger tuning (e.g., -gsce)?

dwblaikie commented 8 months ago

See how it works with gdb/lldb to decide whether it should be on-by-default for them? If I had to guess, I'd guess lldb doesn't cope with it (though might be amenable to patches, or maybe even some of the APple folks (@aprantl @JDevlieghere @Michael137 ) might be interested in fixing it - but I don't know that the representation change brings with it any great benefits to debugger users, especially for Apple, so might not be much motivation to change things)

What're the debugger features you're expecting to build with this, out of curiosity? Since there's line you might be debugging that would bring you in the scope of the using declaration, the user can't really name the template parameters of the alias template to print them out (unlike in a function or class template, you might be in scope and able to print out the value or type of a template parameter in an expression there)... - I guess with some kind of API access you could access the template parameters of an alias template, but not sure that'd be hugely motivational.

The ability to simplify the name is nice, I suppose - not sure it'll make a huge difference to total size, but it'd be nice to actually get to the point where simplified template names meant /all/ names. (though there are other problems to solve to get there - operator overloads and lambdas come to mind (lambdas don't have a nice generic naming at the moment (see my thread on dwarf-discuss, I think... ) - which is a problem for cross-compilet type matching, and for simplified template names))

OCHyams commented 8 months ago

See how it works with gdb/lldb to decide whether it should be on-by-default for them?

Good point. Both LLDB (trunk wip 19.0, af7c3ca538efc401d78c868c639851639b193392) and GDB (12.1) don't seem to support DW_TAG_template_alias, and GCC (11.4.0) emits typedefs for this case. So leaving it behind -gsce tuning makes sense.

What're the debugger features you're expecting to build with this, out of curiosity?

We have a private dwarf extension to hint to dwarf consumers that a namespace should be hidden, and we noticed the namespace gets printed in template alias names when a type from the namespace is used as a template argument to an alias instantiation. e.g.

namespace hide_me { using INT = int; }

template<typename T, typename U>
struct X {
  T m1;
  U m2;
};
template<typename V>
using Y = X<V, int>;
Y<hide_me::INT> y = {1, 2};

The typedef emitted for y's type has DW_AT_name = Y<hide_me::INT>.

dwblaikie commented 8 months ago

See how it works with gdb/lldb to decide whether it should be on-by-default for them?

Good point. Both LLDB (trunk wip 19.0, af7c3ca) and GDB (12.1) don't seem to support DW_TAG_template_alias, and GCC (11.4.0) emits typedefs for this case. So leaving it behind -gsce tuning makes sense.

What're the debugger features you're expecting to build with this, out of curiosity?

We have a private dwarf extension to hint to dwarf consumers that a namespace should be hidden, and we noticed the namespace gets printed in template alias names when a type from the namespace is used as a template argument to an alias instantiation. e.g.

namespace hide_me { using INT = int; }

template<typename T, typename U>
struct X {
  T m1;
  U m2;
};
template<typename V>
using Y = X<V, int>;
Y<hide_me::INT> y = {1, 2};

The typedef emitted for y's type has DW_AT_name = Y<hide_me::INT>.

So do you use simple template names, or strip them out after the fact/on input and rely on the template parameter DIEs to rebuild them (the same as one would with simplified template names)?

(do you have any interest in knowing all the places where simplified template names doesn't trigger because we don't have canonical names (eg: the way we name our lambdas)? I'd love to get those fixed and if it happened to be also relevant to you, that might be a nice shared motivation)

In any case, that seems fair - and doing it under SCE-only for now (I think our rules are that nothing sohuld be only accessible via tuning, so there should be a separate flag for it too, I think? If I'm remembering the rules correctly - I think @pogo59 had firm feelings about this so he might remember better than me)

OCHyams commented 8 months ago

So do you use simple template names, or strip them out after the fact/on input and rely on the template parameter DIEs to rebuild them (the same as one would with simplified template names)?

Yep the latter (I think using -gsimple-template-names is being looked into, or was looked into and discarded, I'm doing some investigation there to understand the current status).

Do you have any interest in knowing all the places where simplified template names doesn't trigger because we don't have canonical names (eg: the way we name our lambdas)? I'd love to get those fixed and if it happened to be also relevant to you, that might be a nice shared motivation)

If it's not difficult to dig up I think that would be useful, yes please. I don't know if/when I'll be able to look into it but our debugger team have expressed an interest in getting these cases fixed.

dwblaikie commented 8 months ago

So do you use simple template names, or strip them out after the fact/on input and rely on the template parameter DIEs to rebuild them (the same as one would with simplified template names)?

Yep the latter (I think using -gsimple-template-names is being looked into, or was looked into and discarded, I'm doing some investigation there to understand the current status).

Good to know. (might actually be interesting to know how you do that, since some of the opt-outs of Simplified Template Names were done because it seemed hard to reliably strip the template parameters - eg: from operator overloads. I didn't want the stripping to be too complicated, and having to do <> matching on something like operator<< <int> to know which <> to strip and which to keep seemed too complex)

Do you have any interest in knowing all the places where simplified template names doesn't trigger because we don't have canonical names (eg: the way we name our lambdas)? I'd love to get those fixed and if it happened to be also relevant to you, that might be a nice shared motivation)

If it's not difficult to dig up I think that would be useful, yes please. I don't know if/when I'll be able to look into it but our debugger team have expressed an interest in getting these cases fixed.

Best thing to do would be to check the examples in the test cases (some are positive tests, some negative, but if you ran the test and skimmed the names - looking at any unsimplified) and the implementation

Ones that I remember particularly well included lambdas (there's some negative consequences to our current lambda naming (it's not canonical - you can construct code where two distinct types end up with the same name, and two linkage-identical types have distinct names - so the debugger would have a hard time realizing they're the same type (or not)) even without simplified template names)) and operator overloads (hard to distinguish the template parameter list from the operator symbol - at least I didn't give it too much thought).

OCHyams commented 8 months ago

might actually be interesting to know how you do that

I've asked the team, I'll get back to you on that.

Thanks for the examples, I'll dig into them soon!


Regarding this fix for this ticket, I'm running into an issue trying to get the DW_TAG_template_alias I'm generating to respect -gsimple-template-names (specifically =mangled; it's easy enough to omit/add the template arguments by just (not/) using printTemplateArgumentList). I'm not very familiar with the front end, so wondered if any of this makes sense to you and if you had any tips?

Using this example:

template<int Z>
struct X { int m1 = Z; };

template<int Value>
using A = X<Value>;

X<5> x;
A<4> a;

Using CGDebugInfo::GetName, the specialised struct looks like this:

ClassTemplateSpecializationDecl 0x55586fc94838 <test3.cpp:1:1, line:2:24> col:8 struct X definition implicit_instantiation
|-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable literal has_constexpr_non_copy_move_ctor can_const_default_init
| |-DefaultConstructor...
| |-CopyConstructor...
| |-MoveConstructor...
| |-CopyAssignment...
| |-MoveAssignment...
| `-Destructor...
|-TemplateArgument integral 4
|-CXXRecordDecl 0x55586fc94b60 <col:1, col:8> col:8 implicit struct X
|-FieldDecl 0x55586fc94bf0 <col:12, col:21> col:16 m1 'int'
| `-SubstNonTypeTemplateParmExpr 0x55586fc9dc88 <col:21> 'int'
|   |-NonTypeTemplateParmDecl 0x55586fc74180 <line:1:10, col:14> col:14 referenced 'int' depth 0 index 0 Z
|   `-IntegerLiteral 0x55586fc9dc68 <line:2:21> 'int' 4
|-CXXConstructorDecl...
|-CXXConstructorDecl...
`-CXXConstructorDecl...

But the template alias instantiation looks like this:

TypeAliasTemplateDecl 0x55586fc74850 <test3.cpp:4:1, line:5:18> col:1 A
|-NonTypeTemplateParmDecl 0x55586fc74698 <line:4:10, col:14> col:14 referenced 'int' depth 0 index 0 Value
`-TypeAliasDecl 0x55586fc747f0 <line:5:1, col:18> col:7 A 'X<Value>'
  `-ElaboratedType 0x55586fc74780 'X<Value>' sugar dependent
    `-TemplateSpecializationType 0x55586fc74730 'X<Value>' dependent X
      `-TemplateArgument expr
        `-DeclRefExpr 0x55586fc74708 <col:13> 'int' NonTypeTemplateParm 0x55586fc74698 'Value' 'int'

Notice the TemplateArgument is integral 4 for the struct, but it's expr for the template alias. This causes issues in GetName, which has an assert that fires when the template argument is type Expression.

Why is it an Expression for the template alias specialisation, but an Integer for the struct (and for function) specialisations?

I'm getting the TypeAliasTemplateDecl from CreateType, which is where I'm creating the DW_TAG_template_alias metadata.

dwblaikie commented 8 months ago

Huh, I was going to guess it'd be the other way around (expr in a real template, integer in an alias template).

But I guess the reason is that the alias template doesn't have to be cannonicalized/doesn't have to really care what the expression is - just passing it along to the usage?

Though perhaps for AST reasons, maybe we do have to canonicalize some of it so we don't repeatedly do whatever the template work is... let's see.

Yeah, the alias template isn't instantiated, it's just a pattern - I'm not sure where the "A<4>" or "A<5>" comes from - maybe we reference the alias template pattern and keep a list of arguments, but don't have a full instantiation:

https://godbolt.org/z/hGsT9GE4q

template<int Value>
using A = int;

A<4> a;
A<4> b;
A<5> c;
|-TypeAliasTemplateDecl 0xcfaf4d8 <<source>:1:1, line:2:11> col:1 A
| |-NonTypeTemplateParmDecl 0xcfaf3f0 <line:1:10, col:14> col:14 'int' depth 0 index 0 Value
| `-TypeAliasDecl 0xcfaf478 <line:2:1, col:11> col:7 A 'int'
|   `-BuiltinType 0xcf652f0 'int'
|-VarDecl 0xcfaf630 <line:4:1, col:6> col:6 a 'A<4>':'int'
|-VarDecl 0xcfaf7e0 <line:5:1, col:6> col:6 b 'A<4>':'int'
`-VarDecl 0xcfaf950 <line:6:1, col:6> col:6 c 'A<5>':'int'

I guess adding support to evaluate the expression to simplified template names is probably a thing we could do.

OCHyams commented 7 months ago

Thanks for the pointers. I've put up a pull request for this - #87623

Yeah, the alias template isn't instantiated, it's just a pattern - I'm not sure where the "A<4>" or "A<5>" comes from - maybe we reference the alias template pattern and keep a list of arguments, but don't have a full instantiation:

Oh I see (I think -- front end is all new to me). I tried shuffling everything through GetName but couldn't get it working because of this (again, I think). AFAIU TypeAliasTemplateDecl doesn't have instantiation info / template args. so TypeAliasTemplateDecl::getNameForDiagnostic (getNameForDiagnostic is called form GetName) wouldn't have access to the template args to construct the name. In order to avoid fiddling with the AST (because I do not know what I'm doing there) I've avoided going through GetName. I suppose we should move discussion on this approach to the pull request though.

OCHyams commented 7 months ago

Fixed in 8d6a9c05f6d676c93c84ebf06cf6263657e74c00 and e772a268ef75332b72dd9b9ca0341a6af8b0db72

Copying my comment from the pull request which outlines a limitation with the implementation:

There's a discussion starting https://github.com/llvm/llvm-project/pull/87623#discussion_r1567968127 that discusses whether or not we should include defaulted arguments in the list. The very short summary is that I tried this by scraping the defaults from the template parameter list, but it turns out this is difficult in the face of parameter defaults that are dependent types and values. So the current implementation ignores defaulted arguments, i.e., doesn't include them in the argument list (and as a consequence also omits empty parameter packs that come after defaulted arguments).

This is not ideal, but not a regression from the DW_TAG_typedef names for template aliases which also do not include defaulted arg values.

OCHyams commented 7 months ago

See also #89774