Page MenuHomePhabricator

[DebugInfo] Place static variable DIEs under the correct parent
Needs ReviewPublic

Authored by ellis on Aug 20 2021, 3:14 PM.

Details

Reviewers
dblaikie
Summary

If a static variable (which is also a global variable) is defined in a function that is inlined and removed, then the concrete DIE of the function will not be created and the variable's DIE won't have the correct parent. Instead, delay the construction of static variable DIEs until finalizeModuleInfo() so that the abstract origin SP DIEs are available. This depends on https://reviews.llvm.org/D110294 because getOrCreateGlobalVariableDIE() should look for an abstract origin SP DIE in this case.

This fixes https://bugs.llvm.org/show_bug.cgi?id=30637

Some of this work was inspired by @krisb's work in https://reviews.llvm.org/D109703.

Diff Detail

Event Timeline

ellis created this revision.Aug 20 2021, 3:14 PM
ellis published this revision for review.Aug 20 2021, 3:15 PM
ellis added a reviewer: dblaikie.
ellis added a project: debug-info.
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2021, 3:16 PM
ellis edited the summary of this revision. (Show Details)Aug 20 2021, 3:19 PM
dblaikie added inline comments.Aug 20 2021, 6:07 PM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
100

Rather than creating then deferring them - would it work if the whole creation were deferred until needed? (ie: move this loop:

DenseSet<DIGlobalVariable *> Processed;
for (auto *GVE : CUNode->getGlobalVariables()) {
  DIGlobalVariable *GV = GVE->getVariable();
  if (Processed.insert(GV).second)
    CU.getOrCreateGlobalVariableDIE(GV, sortGlobalExprs(GVMap[GV]));
}

from beginModule to finalizeModuleInfo?

ellis added inline comments.Aug 20 2021, 6:40 PM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
100

The issue is getOrCreateGlobalVariableDIE() is call in constructImportedEntityDIE() before finalizeModuleInfo(). That's why I made sure to include DIImportedEntity() in the test case.

dblaikie added inline comments.Aug 20 2021, 7:46 PM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
100

& we can't move the imported entity creation to the end of the module because they're needed for lexical scopes?

ellis added inline comments.Aug 21 2021, 10:15 PM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
100

Yeah I guess so. IMO it seems like it's simpler to just give a special case to static globals instead of deferring all globals, imported entities, and their dependencies.

dblaikie added inline comments.Aug 22 2021, 11:47 AM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
138–143

Be good to address these else-after-return issues as flagged by the linter.

154–161

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)

155

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.

156–157

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.

491

Unrelated formatting should be removed or committed separately.

1026–1027

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

llvm/test/CodeGen/ARM/2011-01-19-MergedGlobalDbg.ll
14 ↗(On Diff #367916)

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

llvm/test/DebugInfo/PowerPC/strict-dwarf.ll
18–31 ↗(On Diff #367916)

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)

ellis marked 2 inline comments as done.Sep 2 2021, 6:40 PM
ellis added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
154–161

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.

155

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.

llvm/test/DebugInfo/PowerPC/strict-dwarf.ll
18–31 ↗(On Diff #367916)

I guess fixing which globals get deferred made this test pass without any changes

ellis updated this revision to Diff 370466.Sep 2 2021, 6:41 PM

Rebase and resolve comments

dblaikie added inline comments.Sep 3 2021, 10:40 AM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
154–161

Did this issue get resolved? Looks like this code is still creating VariableDIE and not immediately giving it a parent.

155

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.

ellis added inline comments.Sep 3 2021, 10:56 AM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
154–161

No I still need to fix this. I just pushed the work I had done so far.

krisb added a subscriber: krisb.Sep 13 2021, 12:59 AM
krisb added a comment.Sep 13 2021, 9:53 AM

@ellis, @dblaikie I'm working on a patch that aims to fix a similar issue with static locals reported at https://bugs.llvm.org/show_bug.cgi?id=44695, just published the review at https://reviews.llvm.org/D109703. It should fix the issue with inlined scopes as well. Could you please take a look if you have a chance?

ellis edited the summary of this revision. (Show Details)Fri, Oct 1, 3:53 PM
ellis edited the summary of this revision. (Show Details)Fri, Oct 1, 4:00 PM
ellis updated this revision to Diff 376650.Fri, Oct 1, 4:10 PM

Add comment

ellis updated this revision to Diff 379231.Tue, Oct 12, 5:28 PM

Remove dependency on D110294 by checking for abstract SPs in getOrCreateGlobalVariableDIE() when appropriate.

ellis marked 6 inline comments as done.Tue, Oct 12, 5:29 PM