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

[Ellis Hoag via Phabricator via llvm-commits]

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


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113741