Do not split variables definitions into 2 DIEs.

[friss]

Summary

This is currently done for variables that aren't a file or function scope, but
the reason is unclear and might be historical. Static class member where
handled specifically and always generated a DW_TAG_member declarartion and
a DW_TAG_variable instance. This patch removes all these and only ever emits
one definition DIE, be it a DW_TAG_variable or a DW_TAG_member.

It would be good to get a gdb testsuite run with the patch applied before it
goes in to prevent any fallback (and I guess this kind of patch has a huge
fallback potential). I'll try to run lldb tests on my side.

[dblaikie]

test/DebugInfo/X86/gnu-public-names.ll

56

This actually seems like it /might/ be a reasonable place for a separate declaration and definition, since the user wrote an out of line definition.

What location do we attribute the definition to? It might be worth checking the decl_line attribute in this test. Did we previously attribute the declaration and definition to their respective lines, or just duplicate one/the other of them?

How does this compare to other nested definitions (I suppose for functions in namespaces we don't emit both distinguishing the declaration and definition in cases like: "namespace x { void func(); } void x::func() {}" so there's arguably no reason to similarly for variables)

[friss]

test/DebugInfo/X86/gnu-public-names.ll

56

I would agree totally if we somehow emitted information corresponding to the definition location in the debug info. But the only line information that we provide is the one of the declaration in all cases (the attribute is called decl_line, so maybe that's really how it should be). If the definition DIE has no overlapping attributes with the declaration, I don't see a reason to keep both.
There is one thing that is special to class members though. If we only emit a definition (what the patch actually does), it will be inside the type. And this is be an issue if we want to emit type units, isn't it? Maybe this whole split-if-not-file/function-scope heuristic is to only ever emit declarations into classes.
For functions in namespaces, we only emit one definition DIE, but for class methods we split declaration and definition. So maybe we should really keep the class type exempt of definitions.

[friss]

I finally managed to get some test results for this patch. Indeed putting the static member variable definition broke some lldb tests, thus I left the behavior unchanged for static member variables. This is still a nice cleanup of the global variable DIE creation logic.

David, if you still like the direction I'd appreciate if you could give it a test run on the GDB testsuite to see if it doesn't break anything there. Note that I haven't included an additional test for type units as I removed the behavior change that might have caused issues with that usecase.

[friss]

ping

[dblaikie]

I've checked this out on the GDB test suite & it seems fine.

Looking at the code itself, I think it could be made a bit more
obvious/uniform to have VariableDIE always refer to the definition and have
just a single conditional to decide whether to attach the
DW_AT_specification or not.

Actually this works pretty well & even adds another source fidelity
benefit, allowing the definition, while out-of-line of the class, to be
attributed to the scope in which the definition occurs (eg: namespace x {
struct foo { static int i; }; int foo::i; } - putting the definition in
namespace 'x' instead of just forcing it out to the global namespace) and
exposes a bug in Clang where the definition is scoped to the decl context
instead of the lexical context. (patch attached to show a slightly
short-sighted fix there)

If we did a few more smart things in the frontend, we could get better
fidelity for declarations/definitions of namespace/global variables -
removing the weird special case introduced with my patch. Probably the
right thing here is to optimize in the frontend, if the definition appears
in the decl context (eg: if the global variable's decl context is the same
as the lexical context) then just produce the definition. If the decl and
lexical context differ, produce both a declaration (in the decl context)
and a definition (in the lexical context) - this would be the best source
fidelity, but probably isn't too worthwhile.

Oh, and if we wanted to be really good here, we could make sure we emit any
attributes onto the definition that differ from the declaration - note GCC
does the right thing here, putting DW_AT_decl_line/file on the definitions
if the line/file happens to be different from the declaration's line/file.
We don't do that - we just put nothing on the definition (& I don't think
we do this for function declarations/definitions either (again, here, GCC
gets this right)... could, but probably not important)

• static_var_def.diff1 KBDownload
• global_var.diff5 KBDownload

lib/CodeGen/AsmPrinter/DwarfUnit.cpp

if (!VariableDIE) {
     // Add scoping info.
     if (!GV.isLocalToUnit())
       addFlag(*VariableDIE, dwarf::DW_AT_external);
 
     // Add line number info.
     addSourceLine(*VariableDIE, GV);
   }
 
+  if (!GV.isDefinition())
+    addFlag(*VariableDIE, dwarf::DW_AT_declaration);
+
   // Add location.
   bool addToAccelTable = false;
-  DIE *VariableSpecDIE = nullptr;
   bool isGlobalVariable = GV.getGlobal() != nullptr;
   if (isGlobalVariable) {
     addToAccelTable = true;
     DIELoc *Loc = new (DIEValueAllocator) DIELoc();
     const MCSymbol *Sym = Asm->getSymbol(GV.getGlobal());
     if (GV.getGlobal()->isThreadLocal()) {
       // FIXME: Make this work with -gsplit-dwarf.
       unsigned PointerSize = Asm->getDataLayout().getPointerSize();
if (GV.getGlobal()->isThreadLocal()) {
                 DD->getAddressPool().getIndex(Sym, /* TLS */ true));
       }
       // 3) followed by a custom OP to make the debugger do a TLS lookup.
       addUInt(*Loc, dwarf::DW_FORM_data1, dwarf::DW_OP_GNU_push_tls_address);
     } else {
       DD->addArangeLabel(SymbolCU(this, Sym));
       addOpAddress(*Loc, Sym);
     }
-    // A static member's declaration is already flagged as such.
-    if (!SDMDecl.Verify() && !GV.isDefinition())
-      addFlag(*VariableDIE, dwarf::DW_AT_declaration);
-    // Do not create specification DIE if context is either compile unit
-    // or a subprogram.
-    if (GVContext && GV.isDefinition() && !GVContext.isCompileUnit() &&
-        !GVContext.isFile() && !DD->isSubprogramContext(GVContext)) {
-      // Create specification DIE.
-      VariableSpecDIE = &createAndAddDIE(dwarf::DW_TAG_variable, UnitDie);
+
+    if (IsStaticMember) {
+      // Create specification DIE. The in-type declaration DIE is done.
+      DIE *VariableSpecDIE = &createAndAddDIE(dwarf::DW_TAG_variable, UnitDie);
       addDIEEntry(*VariableSpecDIE, dwarf::DW_AT_specification, *VariableDIE);
       addBlock(*VariableSpecDIE, dwarf::DW_AT_location, Loc);
+      VariableDIE = VariableSpecDIE;
       // A static member's declaration is already flagged as such.
-      if (!SDMDecl.Verify())
-        addFlag(*VariableDIE, dwarf::DW_AT_declaration);
     } else {
       addBlock(*VariableDIE, dwarf::DW_AT_location, Loc);
     }
+
     // Add the linkage name.
     StringRef LinkageName = GV.getLinkageName();
     if (!LinkageName.empty())
       // From DWARF4: DIEs to which DW_AT_linkage_name may apply include:
       // TAG_common_block, TAG_constant, TAG_entry_point, TAG_subprogram and
       // TAG_variable.
-      addString(IsStaticMember && VariableSpecDIE ? *VariableSpecDIE
-                                                  : *VariableDIE,
+      addString(*VariableDIE,
                 DD->getDwarfVersion() >= 4 ? dwarf::DW_AT_linkage_name
                                            : dwarf::DW_AT_MIPS_linkage_name,
                 GlobalValue::getRealLinkageName(LinkageName));
   } else if (const ConstantInt *CI =
                  dyn_cast_or_null<ConstantInt>(GV.getConstant())) {
     // AT_const_value was added when the static member was created. To avoid
     // emitting AT_const_value multiple times, we only add AT_const_value when
     // it is not a static member.
if (isGlobalVariable) {
     addUInt(*Loc, dwarf::DW_FORM_data1, dwarf::DW_OP_constu);
     SmallVector<Value *, 3> Idx(CE->op_begin() + 1, CE->op_end());
     addUInt(*Loc, dwarf::DW_FORM_udata,
             Asm->getDataLayout().getIndexedOffset(Ptr->getType(), Idx));
     addUInt(*Loc, dwarf::DW_FORM_data1, dwarf::DW_OP_plus);
     addBlock(*VariableDIE, dwarf::DW_AT_location, Loc);
   }
 
-  DIE *ResultDIE = VariableSpecDIE ? VariableSpecDIE : VariableDIE;
-
   if (addToAccelTable) {
-    DD->addAccelName(GV.getName(), *ResultDIE);
+    DD->addAccelName(GV.getName(), *VariableDIE);
 
     // If the linkage name is different than the name, go ahead and output
     // that as well into the name table.
     if (GV.getLinkageName() != "" && GV.getName() != GV.getLinkageName())
-      DD->addAccelName(GV.getLinkageName(), *ResultDIE);
+      DD->addAccelName(GV.getLinkageName(), *VariableDIE);
   }
 
-  addGlobalName(GV.getName(), *ResultDIE, GV.getContext());
-  return ResultDIE;
+  addGlobalName(GV.getName(), *VariableDIE, GV.getContext());
+  return VariableDIE;
 }
 
 /// constructSubrangeDIE - Construct subrange DIE from DISubrange.
 void DwarfUnit::constructSubrangeDIE(DIE &Buffer, DISubrange SR, DIE *IndexTy) {
   DIE &DW_Subrange = createAndAddDIE(dwarf::DW_TAG_subrange_type, Buffer);
   addDIEEntry(DW_Subrange, dwarf::DW_AT_type, *IndexTy);
 
   // The LowerBound value defines the lower bounds which is typically zero for

test/DebugInfo/X86/gnu-public-names.ll

 ; CHECK: Compile Unit:
 ; CHECK: DW_AT_GNU_pubnames [DW_FORM_flag_present]   (true)
 ; CHECK-NOT: DW_AT_GNU_pubtypes [
 
 ; CHECK: [[C:0x[0-9a-f]+]]: DW_TAG_structure_type
 ; CHECK-NEXT: DW_AT_name {{.*}} "C"
 
 ; CHECK: [[STATIC_MEM_DECL:0x[0-9a-f]+]]: DW_TAG_member
 ; CHECK-NEXT: DW_AT_name {{.*}} "static_member_variable"
 
 ; CHECK: [[MEM_FUNC_DECL:0x[0-9a-f]+]]: DW_TAG_subprogram
 ; CHECK-NEXT: DW_AT_MIPS_linkage_name
 ; CHECK-NEXT: DW_AT_name {{.*}} "member_function"
 
 ; CHECK: [[STATIC_MEM_FUNC_DECL:0x[0-9a-f]+]]: DW_TAG_subprogram
 ; CHECK-NEXT: DW_AT_MIPS_linkage_name
 ; CHECK-NEXT: DW_AT_name {{.*}} "static_member_function"
 
 ; CHECK: [[INT:0x[0-9a-f]+]]: DW_TAG_base_type
 ; CHECK-NEXT: DW_AT_name {{.*}} "int"
 
 ; CHECK: [[STATIC_MEM_VAR:0x[0-9a-f]+]]: DW_TAG_variable
 ; CHECK-NEXT: DW_AT_specification {{.*}} {[[STATIC_MEM_DECL]]}
 
 ; CHECK: [[GLOB_VAR:0x[0-9a-f]+]]: DW_TAG_variable
 ; CHECK-NEXT: DW_AT_name {{.*}} "global_variable"
 
 ; CHECK: [[NS:0x[0-9a-f]+]]: DW_TAG_namespace
 ; CHECK-NEXT: DW_AT_name {{.*}} "ns"
 
-; CHECK: [[GLOB_NS_VAR_DECL:0x[0-9a-f]+]]: DW_TAG_variable
+; CHECK: [[GLOB_NS_VAR:0x[0-9a-f]+]]: DW_TAG_variable
 ; CHECK-NEXT: DW_AT_name {{.*}} "global_namespace_variable"
 
-; CHECK: [[D_VAR_DECL:0x[0-9a-f]+]]: DW_TAG_variable
+; CHECK: [[D_VAR:0x[0-9a-f]+]]: DW_TAG_variable
 ; CHECK-NEXT: DW_AT_name {{.*}} "d"
 
 ; CHECK: [[D:0x[0-9a-f]+]]: DW_TAG_structure_type
 ; CHECK-NEXT: DW_AT_name {{.*}} "D"
 
 ; CHECK: [[GLOB_NS_FUNC:0x[0-9a-f]+]]: DW_TAG_subprogram
 ; CHECK-NOT: DW_TAG
 ; CHECK: DW_AT_MIPS_linkage_name
 ; CHECK-NOT: DW_TAG
 ; CHECK: DW_AT_name {{.*}} "global_namespace_function"
 
-; CHECK: [[GLOB_NS_VAR:0x[0-9a-f]+]]: DW_TAG_variable
-; CHECK-NEXT: DW_AT_specification {{.*}} {[[GLOB_NS_VAR_DECL]]}
-
-; CHECK: [[D_VAR:0x[0-9a-f]+]]: DW_TAG_variable
-; CHECK-NEXT: DW_AT_specification {{.*}} {[[D_VAR_DECL]]}
-
 ; CHECK: DW_TAG_subprogram
 ; CHECK-NOT: DW_TAG
 ; CHECK:   DW_AT_name {{.*}} "f3"
 ; CHECK-NOT: {{DW_TAG|NULL}}
 ; CHECK: [[F3_Z:.*]]:   DW_TAG_variable
 ; CHECK-NOT: DW_TAG
 ; CHECK:     DW_AT_name {{.*}} "z"
 ; CHECK-NOT: {{DW_TAG|NULL}}
 ; CHECK:     DW_AT_location
 ; CHECK-NOT: {{DW_TAG|NULL}}
 ; CHECK:   NULL
 ; CHECK-NOT: {{DW_TAG|NULL}}
 
 ; CHECK: [[OUTER:.*]]: DW_TAG_namespace
 ; CHECK-NOT: DW_TAG
 ; CHECK:   DW_AT_name {{.*}} "outer"
 ; CHECK-NOT: {{DW_TAG|NULL}}
 ; CHECK: [[OUTER_ANON:.*]]:  DW_TAG_namespace
 ; CHECK-NOT: {{DW_TAG|NULL}}
 ; CHECK-NOT:     DW_AT_name
-; CHECK: [[OUTER_ANON_C_DECL:.*]]:     DW_TAG_variable
+; CHECK: [[OUTER_ANON_C:.*]]:     DW_TAG_variable
 ; CHECK-NOT: DW_TAG
 ; CHECK:       DW_AT_name {{.*}} "c"
 ; CHECK-NOT: {{DW_TAG|NULL}}
 ; CHECK:     NULL
 ; CHECK-NOT: {{DW_TAG|NULL}}
 ; FIXME: We probably shouldn't bother describing the implicit
 ; import of the preceding anonymous namespace. This should be fixed
 ; in clang.
 ; CHECK:     DW_TAG_imported_module
 ; CHECK-NOT: {{DW_TAG|NULL}}
 ; CHECK:   NULL
 ; CHECK-NOT: {{DW_TAG|NULL}}
-; CHECK: [[OUTER_ANON_C:.*]]: DW_TAG_variable
-; CHECK-NOT: DW_TAG
-; CHECK-NEXT:   DW_AT_specification {{.*}} {[[OUTER_ANON_C_DECL]]}
 
 ; CHECK: [[ANON:.*]]: DW_TAG_namespace
 ; CHECK-NOT:   DW_AT_name
 ; CHECK: [[ANON_INNER:.*]]:  DW_TAG_namespace
 ; CHECK-NOT: DW_TAG
 ; CHECK:     DW_AT_name {{.*}} "inner"
 ; CHECK-NOT: {{DW_TAG|NULL}}
-; CHECK: [[ANON_INNER_B_DECL:.*]]:     DW_TAG_variable
+; CHECK: [[ANON_INNER_B:.*]]:     DW_TAG_variable
 ; CHECK-NOT: DW_TAG
 ; CHECK:       DW_AT_name {{.*}} "b"
 ; CHECK-NOT: {{DW_TAG|NULL}}
 ; CHECK:     NULL
 ; CHECK-NOT: {{DW_TAG|NULL}}
-; CHECK: [[ANON_I_DECL:.*]]:   DW_TAG_variable
+; CHECK: [[ANON_I:.*]]:   DW_TAG_variable
 ; CHECK-NOT: DW_TAG
 ; CHECK:     DW_AT_name {{.*}} "i"
 ; CHECK-NOT: {{DW_TAG|NULL}}
 ; CHECK:   NULL
 ; CHECK-NOT: {{DW_TAG|NULL}}
-; CHECK: [[ANON_INNER_B:.*]]: DW_TAG_variable
-; CHECK-NOT: DW_TAG
-; CHECK-NEXT:   DW_AT_specification {{.*}} {[[ANON_INNER_B_DECL]]}
-; CHECK: [[ANON_I:.*]]: DW_TAG_variable
-; CHECK-NOT: DW_TAG
-; CHECK-NEXT:   DW_AT_specification {{.*}} {[[ANON_I_DECL]]}
 
 ; CHECK: [[MEM_FUNC:0x[0-9a-f]+]]: DW_TAG_subprogram
 ; CHECK-NOT: DW_TAG
 ; CHECK: DW_AT_specification {{.*}} {[[MEM_FUNC_DECL]]}
 
 ; CHECK: [[STATIC_MEM_FUNC:0x[0-9a-f]+]]: DW_TAG_subprogram
 ; CHECK-NOT: DW_TAG
 ; CHECK: DW_AT_specification {{.*}} {[[STATIC_MEM_FUNC_DECL]]}