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

[Amjad Aboud via llvm-commits]

aaboud marked 8 inline comments as done.
aaboud added a comment.

Thanks David for the comments.
I fixed most of them, see answers below.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:573
@@ +572,3 @@
+
+  if (HasNoneScopeChild)
+    *HasNoneScopeChild = !Children.empty() || HasLocalDclDie;
----------------
dblaikie wrote:
> Rename "HasNoneScopeChild" to "HasNonScopeChild" (or "HasNonScopeChildren") - that took me a while to figure out what it was trying to express, but I think that's what you were going for, "none" as in "not any of" wasn't intended, but "has non-scope child" (children that are something other than a scope) is - then it make sense :)
Yes that what I meant.
Sorry for misleading with the wrong word in the name, I keep doing this mistake between the "None" and "Non" :)

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

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:467
@@ +466,3 @@
+  }
+  assert(false && "Unexpected scope.");
+  return nullptr;
----------------
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().

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:561
@@ +560,3 @@
+  for (const auto &I : CUMap)
+    forBothCUs(*I.second, [&](DwarfCompileUnit &CU) {
+      CU.finishLocalScopeDefinitions();
----------------
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?


Repository:
  rL LLVM

http://reviews.llvm.org/D15976