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

[Ellis Hoag via Phabricator via llvm-commits]

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:
> 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.
No I still need to fix this. I just pushed the work I had done so far. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108492