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

[David Blaikie via llvm-commits]

dblaikie added a comment.

This is looking really good and/or I'm starting to get the hang of what's going on here (& I think our design discussions have converged a lot too - I can see all the code I expect to see, delaying adding entities to the scopes until we know whether to add it to the concrete or abstract, adding in the abstract_origin as well, etc)

A few practical comments, but I think we're pretty close to having this sorted out. Thanks, as always, for your patience!


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:573
@@ +572,3 @@
+
+  if (HasNoneScopeChild)
+    *HasNoneScopeChild = !Children.empty() || HasLocalDclDie;
----------------
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 :)

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:713
@@ +712,3 @@
+    // otherwise attach it to concrete local scope.
+    DIE *LBDie = (LSInfo.AbstractLSDie) ? (LSInfo.AbstractLSDie)
+                                        : (LSInfo.ConcreteLSDie);
----------------
drop all the parens around the variables here

  DIE *LBDie = LSInfo.AbstractLSDie ? LSInfo.AbstractLSDie : LSInfo.ConcreteLSDie;

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:716
@@ +715,3 @@
+    assert(LBDie || LSInfo.LocalDclDies.empty());
+    for (auto &D : LSInfo.LocalDclDies) {
+      LBDie->addChild(std::move(D));
----------------
Drop {} from single line loop

================
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);
----------------
Drop {} from single line loop

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

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:467
@@ +466,3 @@
+  }
+  assert(false && "Unexpected scope.");
+  return nullptr;
----------------
replace assert(false) with llvm_unreachable (& drop the "return" after it - it's dead/not needed)

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:500
@@ -477,5 +499,3 @@
     DwarfCompileUnit &CU = constructDwarfCompileUnit(CUNode);
-    for (auto *IE : CUNode->getImportedEntities())
-      CU.addImportedEntity(IE);
-    for (auto *GV : CUNode->getGlobalVariables())
-      CU.getOrCreateGlobalVariableDIE(GV);
+    for (auto *GV : CUNode->getGlobalVariables()) {
+      if (!collectLocalScopedNode(GV->getScope(), GV, CU))
----------------
Can drop the {} here if you like (as noted below, this isn't a certain/consistent rule in the LLVM codebase)

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:523
@@ -498,3 +522,3 @@
     // available.
-    for (auto *IE : CUNode->getImportedEntities())
-      constructAndAddImportedEntityDIE(CU, IE);
+    for (auto *IE : CUNode->getImportedEntities()) {
+      if (!collectLocalScopedNode(IE->getScope(), IE, CU))
----------------
Can drop the {} if you like - this one is less clear. Some parts/developers of LLVM drop {} around any single statement block (anywhere it's valid, basically), some only drop them around single /line/ blocks.

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

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1297
@@ +1296,3 @@
+    // Assure abstract local scope created for each one contains local DIEs.
+    for (const DILocalScope *LS : getLocalScopes(SP)) {
+      LScopes.getOrCreateAbstractScope(LS);
----------------
Drop {} from single line block

================
Comment at: lib/CodeGen/AsmPrinter/DwarfFile.h:66
@@ +65,3 @@
+
+    LocalScopeDieInfo() : ConcreteLSDie(nullptr), AbstractLSDie(nullptr) {}
+  };
----------------
You can just use in class initializers for this:

  DIE *ConcreteLSDie = nullptr;
  DIE *AbstractLSDie = nullptr;


Repository:
  rL LLVM

http://reviews.llvm.org/D15976