[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:138-143
+  if (auto *ParentDie = getAbstractSPDies()[Scope])
+    return ParentDie;
+  else if (auto *CB = Scope ? dyn_cast<DICommonBlock>(Scope) : nullptr)
+    return getOrCreateCommonBlock(CB, GlobalExprs);
+  else
+    return getOrCreateContextDIE(Scope);
----------------
Be good to address these else-after-return issues as flagged by the linter.


================
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()) {
----------------
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)


================
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
----------------
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.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:162-163
+  if (GV->isLocalToUnit()) {
+    // The scope of a static variable can be a subprogram, but DIEs of inlined
+    // subprograms do not exist yet.
+    UnfinishedStaticVariables.push_back({GV, VariableDIE, GlobalExprs});
----------------
I'd probably flesh out this comment a bit more - it's not immediately obvious how the first part relates to the last part. Maybe something like:
```
Defer adding static variables to their parent DIE until all subprograms have been found/created - until then (without a second pass) it's not possible to know whether there will be a separate abstract origin DIE (which should be the parent for local static 'global' variables) or only one out of line subprogram (which would be the parent in that case).
```

Maybe? I'm not sure.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:494
           // symbols are (with entries in .debug_addr).
-          // For now, since we only ever use index 0, this should work as-is.       
+          // For now, since we only ever use index 0, this should work as-is.
           addUInt(*Loc, dwarf::DW_FORM_data4, FrameBase.Location.WasmLoc.Index);
----------------
Unrelated formatting should be removed or committed separately.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1029-1030
 
+  // TODO: Emit static global variables.
+
   // Skip imported directives in gmlt-like data.
----------------
This can/should probably be committed separately - feel free to commit without review. (maybe flesh out the comment a bit to explain what's missing here - I assume/think the bug this comment is alluding to is that static local variables currently always go in the root of the subprogram, rather than in their appropriate lexical scope?)


================
Comment at: llvm/test/CodeGen/ARM/2011-01-19-MergedGlobalDbg.ll:14
 ; CHECK: DW_TAG_variable
-; CHECK-NOT: DW_TAG
 ; CHECK:    DW_AT_name {{.*}} "x1"
----------------
Just removing these lines is probably not ideal - it means the DW_TAG_variable that's being checked is potentially (likely) unrelated to the attributes being checked. So perhaps the right answer is to leave the CHECK-NOT in, but add a few extra "CHECK: DW_TAG_variable"s before the first one, to skip over whatever extra variables there are in these test cases?)


================
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)
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108492