[PATCH] D113741: [RFC][DwarfDebug][AsmPrinter] Support emitting function-local declaration for a lexical block

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Sounds like this generally moves things forward, thanks for your patience!



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1094-1097
+  for (auto *Ty : CUNode->getRetainedTypes())
+    if (DIType *RT = dyn_cast<DIType>(Ty))
+      if (auto *LScope = getLocalScope(RT->getScope()))
+        CU.recordLocalScopeWithDecls(LScope);
----------------
krisb wrote:
> dblaikie wrote:
> > This is fairly problematic to the strategy - generally we don't put types in the "retained types" list because the point is to lazily reference types/other content as much as possible (so if the middle end optimizes away a function and that's the only place where the type is referenced, then the type gets optimized away too).
> > 
> > Perhaps this points to some other direction (perhaps in addition) being needed? Perhaps local entities could go on the `DISubprogram`'s `retainedNodes` attribute?
> > 
> > (this would address a test failure/FIXME you mentioned in one of the test cases - that a local type was missing from the intended scope)
> > Perhaps this points to some other direction (perhaps in addition) being needed? Perhaps local entities could go on the DISubprogram's retainedNodes attribute?
> Unless I'm missing something retainedNodes semantic is to keep entities even if they can be optimized out (as of retainedTypes is for types, but only if we have corresponding compile option passed). So, I'm not sure we should use those fields to keep some local entities that are okay to be completely removed, if they unused/ get optimized away.
> I was thinking about a kind of a new field for DISubprogram (and maybe for DILexicalBlock), something like `localDecls` which would keep references for static locals, local imported entities, and records/typedefs that belongs to this local scope (and stop referencing local entities from `globals` and `imports` fields of a compile unit), but have not come to a comprehensive solution yet.
> 
Yep, complicated tradeoffs. By having the list we'd avoid miscoping these entities (because they're otherwise intractible to find - they could appear in any part of the DWARF) - but any attempt to keep them in a list risks them being preserved too much too.

keeping the list on the DILexicalBlock would be pretty good - that way if the lexical block became unreferenced (all instructions in it were optimized away) the lexical block could be removed and the list along with it - hmm, but there are still cases where the type could be alive despite the function being totally optimized away (eg: a trivial function that returns a stateless local type, like a lambda - so the local type escapes the scope, and the function gets inlined and optimized away - no scope, but the type is still out there in the rest of the DWARF) - in which case we'd still scope the type incorrectly... 


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDie.cpp:675-676
       return;
+    if (D.getTag() == DW_TAG_lexical_block)
+      return;
     D = D.resolveTypeUnitReference();
----------------
krisb wrote:
> dblaikie wrote:
> > This change could probably be committed separately with a llvm-dwarfdump+assembly test case demonstrating this fix. Then the LLVM code generation changes that depend on this would come in the second patch. Not a huge deal, though.
> Before this patch, AsmPrinter would crash on types scoped within a lexical block (cause DwarfCompileUnit would not able to create a context for such types), so I'm not sure how to reproduce the case. Do you have some ideas in mind?
What you could do is using this patch, generate the assembly for a type scoped in a lexical block - but then commit that assembly test case ahead of the rest of the patch. It's still a good/general improvement to llvm-dwarfdump/libDebugInfoDWARF that applies even if LLVM never generates this DWARF (because llvm-dwarfdump is intended as a tool to investigate any DWARF, not just the DWARF that llvm generates).

This is true for most llvm-dwarfdump/libDebugInfoDWARF changes - they can come before the LLVM change that generates them, because they should/can be tested without any LLVM support at all, through assembly test cases .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113741