[PATCH] D108492: [DebugInfo] Place static variable DIEs under the correct parent
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Aug 22 11:47:34 PDT 2021
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
More information about the llvm-commits
mailing list