[PATCH] D108492: [DebugInfo] Place static variable DIEs under the correct parent

[Ellis Hoag via Phabricator via llvm-commits]

ellis marked 2 inline comments as done.
ellis added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:160
 
-  // Construct the context before querying for the existence of the DIE in
-  // case such construction creates the DIE.
-  auto *CB = GVContext ? dyn_cast<DICommonBlock>(GVContext) : nullptr;
-  DIE *ContextDIE = CB ? getOrCreateCommonBlock(CB, GlobalExprs)
-    : getOrCreateContextDIE(GVContext);
+  DIE *VariableDIE = DIE::get(DIEValueAllocator, GV->getTag());
+  if (GV->isLocalToUnit()) {
----------------
dblaikie wrote:
> I worry about creating DIEs without parents for extended periods of time within the DWARF emission - as they may confuse/break certain assumptions about parentage that are made when trying to determine when to use cross-unit references: https://github.com/llvm/llvm-project/blob/d8d84c9df82fc114f2b22a533a8183065ca1a2e0/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp#L380
> 
> Perhaps you could do some testing around whether this is an issue/comes up for this case? (and/or maybe add an expensive assertion that checks somewhere in that code that the DIE being queried for its unit is not in the UnfinishedStaticVariables list)
Yes, you are right. In fact, in this function we add attributes to this DIE which calls `addDIEEntry()` and triggers that assertion. I'll have to see if I can somehow defer the creation of all global variable DIEs until after the module has been processed like you mentioned earlier.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:161
+  DIE *VariableDIE = DIE::get(DIEValueAllocator, GV->getTag());
+  if (GV->isLocalToUnit()) {
+    // The scope of a static variable can be a subprogram, but DIEs of inlined
----------------
dblaikie wrote:
> This condition doesn't cover all/only the cases where a global variable is local to a function. It misses the case of local static variables in inline functions (these are not "local to unit", unlike a local static in a non-inline function definition) and it incorrectly classifies a file-scoped static variable as needing deferred initialization, when it doesn't.
I'm now checking the scope to see if it's a `DILocalScope`, which I assume means it is a static variable. Let me know if there is another case to consider.


================
Comment at: llvm/test/DebugInfo/PowerPC/strict-dwarf.ll:18-31
+; CHECK: DW_AT_name      ("f")
+; CHECK-NOT: DW_TAG_
+; CHECK: DW_AT_noreturn
 ; CHECK: DW_AT_name      ("var")
 ; CHECK-NOT: DW_TAG_
 ; CHECK: DW_AT_alignment
 ; CHECK: DW_AT_location  (DW_OP_addr 0x0)
----------------
dblaikie wrote:
> Could you explain more about what this test was testing for and what changed? Looks like the original code was checking for two different DIEs but didn't check either DW_TAG? So maybe adding the DW_TAG checking would make the test more legible, to make it clear which things are being checked for rather than it sort of looking like a weird collection of attributes we wouldn't expect to see on a single DIE (ie: wouldn't expect noreturn and location on the same DIE - one is for functions, the other is for variables)
I guess fixing which globals get deferred made this test pass without any changes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108492/new/

https://reviews.llvm.org/D108492