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

[Kristina Bessonova via Phabricator via llvm-commits]

krisb added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1213-1218
+  // Collect functions with inlined subprograms.
+  for (const auto &F : M->functions())
+    for (const BasicBlock &BB : F)
+      for (const Instruction &I : BB)
+        if (auto DbgLoc = I.getDebugLoc())
+          recordInlinedSubprogram(DbgLoc);
----------------
ellis wrote:
> ellis wrote:
> > krisb wrote:
> > > ellis wrote:
> > > > This is really great. Previously in https://reviews.llvm.org/D113736 I recorded concrete functions, but that is limited because we aren't sure if there will be an abstract origin. Here we record exactly which subprograms have abstract origins. Also, thanks for including my tests from that diff!
> > > It's nice to know whether the given SP will have an abstract origin or not, but unfortunately, this way of obtaining such information isn't fully reliable. The problem is that having some DILocations with inlinedAt field doesn't guarantee that the inlined/abstract LexicalScope will be created (especially in optimized code). So, I'm not sure this is the right way to go. Maybe I should avoid doing this and collect all the scope DIEs instead to have more chances to emit accurate dwarf.
> > > 
> > > > Also, thanks for including my tests from that diff!
> > > It's me who should thank you for the tests :) By the way, do you plan to commit them separately? 
> > > By the way, do you plan to commit them separately?
> > Let's just commit them here assuming they pass.
> > It's nice to know whether the given SP will have an abstract origin or not, but unfortunately, this way of obtaining such information isn't fully reliable. The problem is that having some DILocations with inlinedAt field doesn't guarantee that the inlined/abstract LexicalScope will be created (especially in optimized code). So, I'm not sure this is the right way to go. Maybe I should avoid doing this and collect all the scope DIEs instead to have more chances to emit accurate dwarf.
> If I understand correctly, that means `HasInlinedInstances()` returns true for inlined SPs even if their DIEs will never be emitted (because they were optimized away for example). In that case we won't construct the scope anyway, so I think that's ok.
Well, yet I decided to try a different approach. Instead of guessing which SPs have inlined instances and which have not in order to decide do we need to store the given DIE to the map, we simply can store all the DIEs (but those that are known to belong inlined instances). So later we can choose which tree to use: an abstract (if it exists) or a concrete one. It looks a bit better to me and we can use a single map for all abstract scopes and avoid duplcation. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113741