[clang] CGDebugInfo asserts `!DT.isNull()` when compiling with debug symbols

[kamleshbhalui]

Summary

Fixes this assert failure.
Which occured due to deduction of auto type of templated variables (required by debuginfo) resulted into null.
https://bugs.llvm.org/show_bug.cgi?id=42710

[kamleshbhalui]

formatted the diff.

[vsk]

Could you add a regression test?

[kamleshbhalui]

Added a test.

[vsk]

clang/lib/CodeGen/CGDebugInfo.cpp

1438

I see that we don't attempt to handle VarTemplateSpecializationDecl when emitting CodeView. But it looks like we generally do handle these when emitting DWARF. I think the problem you're trying to address has more to do with clang not being able to describe globals with undeduced types in debug info.

E.g. if I replace auto with int in your test case, I get (https://godbolt.org/z/gRpRws):

!17 = distinct !DIGlobalVariable(name: "value", linkageName: "_ZN6TypeId5valueIJiEEE", scope: !2, file: !8, line: 7, type: !14, isLocal: false, isDefinition: true, declaration: !13, templateParams: !18)
!18 = !{!19}
!19 = !DITemplateValueParameter(tag: DW_TAG_GNU_template_parameter_pack, name: "Args", value: !20)

Could you limit the scope of this patch to just ignore VarTemplateSpecializationDecl's with undeduced types? I think there's an example of how to do this in CGDebugInfo::EmitUsingDecl, you could factor that out and reuse it here.

Please also add a test case for the s/auto/int/ example, to make sure that doesn't regress.

[kamleshbhalui]

Thanks, @vsk for reviewing.
I have incorporated your suggestions.

[vsk]

clang/test/CodeGenCXX/pr42710.cpp

4 ↗

(On Diff #230747)

Could you validate the debug info for the struct members in the int case? (It may be simpler to structure the test using separate namespaces for the deduced/undeduced VarTemplateSpecializationDecl cases)

[probinson]

I guess first I'm confused about why the type would be undeduced in the first place, given that it is actually instantiated.
And if undeduced is correct, wouldn't we rather emit these with DW_TAG_unspecified_type?

clang/test/CodeGenCXX/pr42710.cpp

2 ↗

(On Diff #230747)

I believe you would use expected-no-diagnostics only if you run clang with -verify. So, please remove those two directives.

[dblaikie]

I feel like there's something missing about this bug/issue - is there a good explanation/understanding for why does the bug only occur with the two levels of static member inline variable templates & not one? Perhaps there's some existing code that works for simple cases but is insufficiently general and should be modified to be so, rather than adding a new case?

[kamleshbhalui]

In D70537#1765607, @probinson wrote:

I guess first I'm confused about why the type would be undeduced in the first place, given that it is actually instantiated.
And if undeduced is correct, wouldn't we rather emit these with DW_TAG_unspecified_type?

While emitting the debug info this is the AST (for given testcase), in which static variable value is not yet deduced.
and when it tries to unwrap the type it fails.

|-CXXRecordDecl 0x30dc88 <col:1, col:8> col:8 implicit struct TypeId
|-VarDecl 0x30dd30 <line:3:5, col:31> col:23 used counter 'int' static inline listinit
| `-InitListExpr 0x30ddd8 <col:30, col:31> 'int'
|-VarTemplateDecl 0x30df68 <line:5:5, line:6:50> col:30 identifier
| |-TemplateTypeParmDecl 0x30de18 <line:5:14> col:25 typename depth 0 index 0 ...
| |-VarDecl 0x30df00 <line:6:5, col:50> col:30 identifier 'const int':'const int' static inline cinit
| | `-UnaryOperator 0x30dfe0 <col:43, col:50> 'int' postfix '++'
| |   `-DeclRefExpr 0x30dfc0 <col:43> 'int' lvalue Var 0x30dd30 'counter' 'int'
| `-VarTemplateSpecializationDecl 0x341f30 <col:5, col:50> col:30 referenced identifier 'const int':'const int' static inline cinit
|   |-TemplateArgument pack
|   | `-TemplateArgument type 'int'
|   `-UnaryOperator 0x30dfe0 <col:43, col:50> 'int' postfix '++'
|     `-DeclRefExpr 0x30dfc0 <col:43> 'int' lvalue Var 0x30dd30 'counter' 'int'
|-VarTemplateDecl 0x30e228 <line:8:5, line:9:56> col:30 value
| |-TemplateTypeParmDecl 0x30e128 <line:8:14, col:26> col:26 referenced typename depth 0 index 0 ... Args
| |-VarDecl 0x30e1c0 <line:9:5, col:56> col:30 value 'const auto' static inline cinit
| | `-UnresolvedLookupExpr 0x30e2f8 <col:38, col:56> '<dependent type>' lvalue (no ADL) = 'identifier' 0x30df68
| `-VarTemplateSpecializationDecl 0x30e698 <col:5, col:30> col:30 value 'const auto' static inline
|   `-TemplateArgument pack
|     `-TemplateArgument type 'int'
|-VarTemplateSpecializationDecl 0x30e698 <col:5, col:30> col:30 value 'const auto' static inline
| `-TemplateArgument pack
|   `-TemplateArgument type 'int'
`-VarTemplateSpecializationDecl 0x341f30 <line:6:5, col:50> col:30 referenced identifier 'const int':'const int' static inline cinit
  |-TemplateArgument pack
  | `-TemplateArgument type 'int'
  `-UnaryOperator 0x30dfe0 <col:43, col:50> 'int' postfix '++'
    `-DeclRefExpr 0x30dfc0 <col:43> 'int' lvalue Var 0x30dd30 'counter' 'int'

[kamleshbhalui]

In D70537#1769458, @dblaikie wrote:

I feel like there's something missing about this bug/issue - is there a good explanation/understanding for why does the bug only occur with the two levels of static member inline variable templates & not one? Perhaps there's some existing code that works for simple cases but is insufficiently general and should be modified to be so, rather than adding a new case?

Here is the situation which is occuring in the particuler case(referring added testcase pr42710.cpp),

while instantiating the template variable value it also needs the type to declare it
(see at https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaTemplateInstantiateDecl.cpp#L4590)
and it sees that it is undeduced type , so it starts instantiating the
initialization expression and in order to doing so it comes to at this point
https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/CGDebugInfo.cpp#L1424
which iterates over all the VarDecl in record and try to generate debug info.
again here template variable value comes , which was undeduced and fails to unwrap the type and finally assert fails.

[kamleshbhalui]

added validation check in testcase

[probinson]

I have to wonder if we're not being too eager to produce the debug info. It seems that the undeduced type problem arises because we're trying to produce debug info before we've really finished instantiating value<int> here. But how Clang works pretty much always confuses me.

[dblaikie]

Yeah, this is working around a more fundamental representational issue.

Like member function templates, member variable templates shouldn't be pre-emptively built or ever appear in the "members" list of the type (because that list isn't closed - we don't know what instantiations would be needed when the type is built so member template instantiations never appear in the member list, but instead the type appears as the scope of the member template instantiation and are added as member DIEs during DWARF emission lazily (eg: if a member function template instantiation gets optimized away, nothing of it remains and no member function template declaration of that instantiation will appear in as a child of the type DIE))

For instance, it seems creating two instantiations of a member variable template seems to get messed up by having a member list with the same instantiation twice:

!9 = !DIDerivedType(tag: DW_TAG_member, name: "variable", scope: !10, file: !3, line: 3, baseType: !12, flags: DIFlagStaticMember, extraData: i32 0)
!10 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "type", file: !3, line: 1, size: 8, flags: DIFlagTypePassByValue, elements: !11, identifier: "_ZTS4type")
!11 = !{!9, !9}

Even though !7 is another member variable template instantiation.

(here's the source:

struct type {
  template <typename T>
  inline static auto variable = T();
};
int main() {
  type t;
  int i = type::variable<int>;
  float f = type::variable<float>;
}

So clearly something is modifying the "members" list (since it'd be created empty after the "type t;" line, then a member variable template for the variable<int> would be added to the member list, then when it goes to add "variable<float>" it gets messed up and adds another copy of !9)

Oh... both variable<int> and variable<float> definitions share a single declaration DIDerivedType... that's no good.

I'm also generally wondering if variable template instantiations should have the template parameters in their name, same as function templates - currently they don't in LLVM's output at least. Building GCC 9.2 to compare there.

[dblaikie]

Yeah, GCC's debug info for this is... not good. I don't think it's anything we'd need to be inspired by. Here's the tags and names:

DW_TAG_compile_unit
  DW_AT_name   ("templ2.cpp")
  DW_TAG_structure_type
    DW_AT_name ("type")
  DW_TAG_variable
    DW_AT_name ("variable")
  DW_TAG_base_type
    DW_AT_name ("int")
  DW_TAG_variable
    DW_AT_name ("variable")
  DW_TAG_base_type
    DW_AT_name ("float")
  DW_TAG_subprogram
    DW_AT_name ("main")
    DW_TAG_variable
      DW_AT_name       ("t")
    DW_TAG_variable
      DW_AT_name       ("i")
    DW_TAG_variable
      DW_AT_name       ("f")

The structure_type has no child DIEs and no mention of the variable template instantiations - the global variables are both called "global" but with different types. No mention of the scope they were actually declared in. I guess maybe at least this "works" for free in some ways, but probably makes it impossible to refer to one or the other variable or with the right name.

@aprantl @probinson @echristo @JDevlieghere - So my proposal is that the 'right' DWARF for this is:

DW_TAG_structure_type
  DW_AT_name ("type")
  DW_TAG_member
    DW_AT_name ("variable<int>")
    DW_AT_declaration (true)
  DW_TAG_member
    DW_AT_name ("variable<float>")
    DW_AT_declaration (true)
DW_TAG_variable
  DW_AT_specification (0x.... "variable<int>")
  DW_AT_location
  DW_AT_linkage_name ("_ZN4type1iE")
DW_TAG_base_type
  DW_AT_name ("int")
DW_TAG_variable
  DW_AT_specification (0x.... "variable<float>")
  DW_AT_location
  DW_AT_linkage_name ("_ZN4type1fE")
DW_TAG_base_type
  DW_AT_name ("float")

And the LLVM IR should be done the same way as member function templates - never appear in the member list, but appear otherwise the same - a DIGlobalVariable named "variable<int>" for the definition with a 'declaration:' attribute that refers to a DIDerivedType DW_TAG_member for the declaration of "variable<int>" where the 'scope:' attribute of the DW_TAG_member refers to the DW_TAG_structure_type, but that structure_type's member list does not contain this member. (same as functions - DISubprogram definition -declaration-> DISubprogram declaration -scope-> DW_TAG_structure_type but doesn't appear in the member list of the structure_type)

[probinson]

@dblaikie let me reflect this back to make sure I get it:
Template members (methods or variables) would never appear in the *metadata* description of the struct; but metadata descriptions of the instances would refer back to that struct (as the scope for the instance). Then DwarfDebug would paste them all together when emitting the DWARF description(s). The in-struct child DIE could then have the deduced type because by the time the definition metadata is produced, we actually know what that type is. This is okay because template data members are necessarily static; they don't affect size or layout of the struct in any way.

So, CGDebugInfo would skip basically any templated member when constructing the struct, but when the template is (finally, fully) instantiated, its definition could know everything it needs to, and point back to the struct.

Sounds reasonable.

[dblaikie]

In D70537#1788252, @probinson wrote:

@dblaikie let me reflect this back to make sure I get it:
Template members (methods or variables) would never appear in the *metadata* description of the struct; but metadata descriptions of the instances would refer back to that struct (as the scope for the instance). Then DwarfDebug would paste them all together when emitting the DWARF description(s). The in-struct child DIE could then have the deduced type because by the time the definition metadata is produced, we actually know what that type is. This is okay because template data members are necessarily static; they don't affect size or layout of the struct in any way.

So, CGDebugInfo would skip basically any templated member when constructing the struct, but when the template is (finally, fully) instantiated, its definition could know everything it needs to, and point back to the struct.

Sounds reasonable.

Right - and that's what's already done for member function templates & what should be (but isn't currently) done for member variable templates & I think that would fix this bug.

clang/lib/CodeGen/CGDebugInfo.cpp

for (const auto *I : record->decls())
         // doesn't emit them.
         if (CGM.getCodeGenOpts().EmitCodeView &&
             isa<VarTemplateSpecializationDecl>(V))
           continue;
 
         if (isa<VarTemplatePartialSpecializationDecl>(V))
           continue;
 
+        if (isa<VarTemplateSpecializationDecl>(V))
+          continue;
         // Reuse the existing static member declaration if one exists
         auto MI = StaticDataMemberCache.find(V->getCanonicalDecl());
         if (MI != StaticDataMemberCache.end()) {
           assert(MI->second &&
                  "Static data member declaration should still exist");
           elements.push_back(MI->second);
         } else {
           auto Field = CreateRecordStaticField(V, RecordTy, record);