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

Wed Nov 17 16:03:36 PST 2021

ellis added inline comments.

================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1626
+  // Check if we have an abstract tree.
+  if (getAbstractScopeDIEs().count(cast<DILocalScope>(S)->getSubprogram()))
+    return ::findLocalScopeDIE(LScope, getAbstractScopeDIEs());
----------------

================
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:
> > krisb wrote:
> > > 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?
> > I think any solution needs to be able to determine if an SP will have an abstract origin before processing any functions, i.e., this needs to be computed in `beginModule()`. Suppose a function `f()` has a concrete instance and is also inlined at the end of the module. It should have an abstract origin and all references should be made to the abstract origin DIE instead of the concrete DIE. That means `getOrCreateSubprogramContextDIE()` needs to know if it should get/create an abstract or concrete DIE even before the abstract origin is created.
> > 
> > I guess the `import-inlined-declaration.ll` test was not complete because it doesn't test if `ns::foo()` references the abstract origin if the concrete DIE exists. Could you update this test to handle that? Here is what I'm thinking, but I haven't actually tested this on your code yet.
> > https://godbolt.org/z/j1xdv3KWG
> > 
> > This would also be a bug when an imported entity tries to reference an SP before its abstract origin was created, but I don't have a concrete example of that.
> > 
> > Let's follow up on https://groups.google.com/g/llvm-dev/c/mlNUyj-Q5To if you have any more questions. Thanks!
> > Suppose a function f() has a concrete instance and is also inlined at the end of the module. It should have an abstract origin and all references should be made to the abstract origin DIE instead of the concrete DIE. 
> This is why I finally decided to move the emission of imported entities (together with types and global variables) to DwarfDebug::endModule() instead of trying to handle these cases earlier. By endModule() we have all the local context emitted, so we don't need to force emitting abstract DIEs ahead of normal flow.
> >Could you update this test to handle that?
> Sure, I will do. But I believe this case should be handled properly cause we never try to emit imported declarations before all the subprograms get processed.
> This is why I finally decided to move the emission of imported entities (together with types and global variables) to DwarfDebug::endModule() instead of trying to handle these cases earlier. By endModule() we have all the local context emitted, so we don't need to force emitting abstract DIEs ahead of normal flow.
Ah ok makes sense!

================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:625-628
+  // For function-local type we may not yet set parents properly.
+  if (!ContextDIE->getUnit())
+    return createTypeDIE(Context, *ContextDIE, Ty);
+
----------------
Where are we creating DIEs without giving them a parent. In a previous attempt @dblaikie mentioned we should avoid even temporarily creating DIEs without a parent because of the `getUnit()` function.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113741