[PATCH] D15976: Supporting all entities declared in lexical scope in LLVM debug info

[David Blaikie via llvm-commits]

dblaikie added inline comments.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:20
@@ -18,1 +19,3 @@
 
+static cl::opt<bool> AbstractOriginOnLS(
+    "use-abstract-origin-on-ls", cl::Hidden,
----------------
I'd really prefer not to have this option/backcompat feature.

Do you have a need to support older GDBs (or LLDB) with this change? Otherwise I think this is a sufficiently narrow case we could probably take the change optimistically and let other debuggers deal with it as a bug. I don't think it'll disrupt that many users. (though I could be wrong/happy to deal with it differently if we have contributing developers who feel strongly that they need a workaround for their users until their platform debuggers catch up)

I realize it doesn't cost too much (your implementation has made it quite simple to support both) - this isn't all teh cost. We'd have to plumb the option up through into the frontend, or make it a debugger tuning feature (how would we tune it for GDB if we want to support old GDBs here? I would imagine this should be a "do the good thing by default, but fall back to the bad thing if we're specifically targeting known bad compilers, like LLDB" - though I think Adrian mentioned he'd be willing to just take this as an LLDB bug, but I'm not 100% sure)

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:573
@@ +572,3 @@
+      else
+        assert(false && "Unexpected DI node.");
+      DU->addLocalDclDieToLexicalScope(Scope, D);
----------------
This should be llvm_unreachable rather than assert(false)

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:733
@@ +732,3 @@
+      // to the corresponding abstract local scope.
+      for (auto &L : LSInfo.InlineLSDies) {
+        addDIEEntry(*L, dwarf::DW_AT_abstract_origin, *LSInfo.AbstractLSDie);
----------------
aaboud wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > Drop {} from single line loop
> > It's possible that this part of the code (& the InlineLSDies member of LSInfo) is not needed - whenever we create an inline instance we /know/ there's a concrete instance and we know (I think) that it will have the abstract-only DIEs on it that mean it needs an abstract_origin, right? So if we put it on up-front, it'll save us having to cache the list of inlined instances, etc, perhaps?
> Could work, assuming that we created the abstract lexical scope before we create the inline lexical scope.
> However, to figure out that the lexical scope is an inline, we need to add a check that would not make the code clean.
> 
> So, I suggest that I implement this suggestion as a follow up, after commenting this implementation.
> Then, you can give it a second thought to decide if you still want to stick with the suggestion, or stay with the current code.
Sure, that can wait

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:467
@@ +466,3 @@
+  }
+  assert(false && "Unexpected scope.");
+  return nullptr;
----------------
aaboud wrote:
> dblaikie wrote:
> > replace assert(false) with llvm_unreachable (& drop the "return" after it - it's dead/not needed)
> I just figure out that I do not need this function.
> Just replaced it with DILocalScope::getSubprogram().
Great!

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:561
@@ +560,3 @@
+  for (const auto &I : CUMap)
+    forBothCUs(*I.second, [&](DwarfCompileUnit &CU) {
+      CU.finishLocalScopeDefinitions();
----------------
aaboud wrote:
> dblaikie wrote:
> > What does the output of this loop look like for split-dwarf? I would expect that this change should have no impact on skeleton DIEs (including the GMLT-like DIEs produced as children of the skeleton CU DIE), yes?
> > 
> > Indeed, it looks like DwarfCompileUnit::finishLocalScopeDefinitions only uses the contents of the DwarfFile anyway - so perhaps the function should be moved up to the DwarfFile level (rather than calling down into the DwarfCompileUnit, then up into the DwarfFile) & only run on the non-skeleton file since it is/should be a no-op on the skeleton unit?
> I admit, I have no clue how split-dwarf looks like.
> I guessed that I might need to handle it like I did here, but if you are convinced that it is redundant, I will follow your suggestion.
> 
> Also, the "DwarfCompileUnit ::finishLocalScopeDefinitions()" method invokes the "DwarfUnit ::addDIEEntry()". 
> If you still think that we move the "finishLocalScopeDefinitions()" method to DwarfFile, then how should we access the "DwarfUnit ::addDIEEntry()" method from there?
Could you try this out with split-dwarf? Or would you like me to take a look?

If we were to put it in DwarfFile, we would just specifically access the non-skeleton DwarfUnit to add DIEs, etc. there are other parts of the code that do this, if I recall correctly.


http://reviews.llvm.org/D15976