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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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()) {
----------------
ellis wrote:
> 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.
Did this issue get resolved? Looks like this code is still creating VariableDIE and not immediately giving it a parent.


================
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
----------------
ellis wrote:
> 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.
Not sure - Yeah, looks like DILexicalBlockScope and DISubprogram are both kinds of DILocalScope, so what you've got here should probably cover the necessary cases I can think of.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108492