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

[Kristina Bessonova via Phabricator via llvm-commits]

krisb added a comment.

@dblaikie thank you for looking at this!

> if the cases where types are not reachable through the CU's retainedTypes list get the same behavior they've always got - can you confirm that's the case?

For //local// types that are not reachable through the CU's retainedTypes list there are two cases:

- if type's direct parent scope was emitted (because it contains some other declarations), this type will be placed to this scope (as expected),
- if type's direct parent scope was omitted (since there are no other decls in it), the type will go to a first available (emitted) parent scope (in the worst case - to the parent subprogram).

So, with this patch, local types will have the closest/most suitable parent from available. Before this patch, local types were placed directly to subprogram no matter which scope they really have (and actually, frontend never emits types with lexical block scope), so the old behavior is only kept as a fallback for the worst case.
For all non function-local types, this patch changes nothing.



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



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDie.cpp:675-676
       return;
+    if (D.getTag() == DW_TAG_lexical_block)
+      return;
     D = D.resolveTypeUnitReference();
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113741