[PATCH] D125693: [DebugInfo][WIP] Support types, imports and static locals declared in a lexical block (3/5)

Mon May 23 05:13:41 PDT 2022

krisb added a comment.

Thank you, @asavonic for your comments!

I've added some more details about the issues this is going to fix, hope it'll help to review the patchset.

In D125693#3523278 <https://reviews.llvm.org/D125693#3523278>, @asavonic wrote:

> Thanks a lot for the patch! It would be great to get this issue finally fixed. I assume that this is the main patch, other patches in the stack seem like just preparation/adjustments needed for this one to work.

Correct. The first two patches were extracted to just make this one a bit smaller and easier to review.

================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1382
+      assert((!GV->getScope() || !isa<DILocalScope>(GV->getScope())) &&
+             "Unexpected function-local declaration!");
       if (Processed.insert(GV).second)
----------------
asavonic wrote:
> So here we discard LLVM IR metadata that have local declarations tied to a CU, right? This will break compatibility with old LLVM IR. Can we do some upgrade to convert this "old style" metadata the way we expect it now? Does it make sense to support both variants?
Good question. I haven't thought about backward compatibility, not even sure we have some requirements for this. The easiest option seems to increase debug metadata version and drop debug info if it isn't compatible with this one as it done in AutoUpgrade. This prevents producing incorrect DWARF for ill-formed metadata.

Upgrading "old style" metadata would be a bit tricky. Current implementation assumes that all the local declarations (excluding regular local vars and labels) are referenced by parent's localDecls field, including types. Before this patchset there were no ways to find types except by references from their uses. So, to find all the function-local types we may need to iterate over all instructions and parse all debug metadata, which likely increases compile time. Not sure this trade-off is a worth one.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125693