[PATCH] D11180: Fixed debug info generation for function static variables, typedef, and records

[David Blaikie via llvm-commits]

dblaikie added a comment.

Thanks again for your reminders and patience...


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:340-341
@@ +339,4 @@
+    if (!includeMinimalInlineScopes()) {
+      auto LocalDeclNodeRangeForScope = DD->findLocalDeclNodesForScope(DS);
+      for (const auto &DI : LocalDeclNodeRangeForScope) {
+        if (auto *IE = dyn_cast<DIImportedEntity>(DI.second))
----------------
Could probably just roll the local variable into the for loop header.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:342-343
@@ +341,4 @@
+      for (const auto &DI : LocalDeclNodeRangeForScope) {
+        if (auto *IE = dyn_cast<DIImportedEntity>(DI.second))
+          Children.push_back(constructImportedEntityDIE(IE));
+        else if (RegularScope) {
----------------
Should we be adding imported entity DIEs in inlined or abstract definitions? Not sure... I suppose maybe.

What does it mean not to add the types and static variables to these cases? (if we inline away all uses of a function - won't we lose the local types and static variables & the user won't be able to print/interact with them?)

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:351-352
@@ +350,4 @@
+                getOrCreateGlobalVariableDIE(GV, /* SkipContext */ true));
+          }
+          else if (auto *RT = dyn_cast<DIType>(DI.second)) {
+            if (getDIE(RT))
----------------
Formatting (run clang-format over your change?)

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:353-355
@@ +352,5 @@
+          else if (auto *RT = dyn_cast<DIType>(DI.second)) {
+            if (getDIE(RT))
+              // Type was already created, nothing to do.
+              continue;
+            Children.push_back(getOrCreateTypeDIE(RT, /* SkipContext */ true));
----------------
Doesn't "getOrCreateTypeDIE" already do the getDIE check? (that would be implied by the "get" part of "get or create" in the name, I would hope)

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:377-389
@@ -353,4 +376,15 @@
     }
-    ScopeDIE = constructLexicalScopeDIE(Scope);
-    assert(ScopeDIE && "Scope DIE should not be null.");
+    // Lexical block of non-inline function is stored in the lexical scope map.
+    if (RegularScope)
+      ScopeDIE = getDIE(DS);
+    if (ScopeDIE) {
+      // Mapped lexical scope should be already attached to its parent scope.
+      assert(ScopeDIE->getParent() && "Scope DIE was not attached to parent.");
+      AddScopeDieToFinalChildren = false;
+    } else {
+      ScopeDIE = constructLexicalScopeDIE(Scope);
+      assert(ScopeDIE && "Scope DIE should not be null.");
+      if (RegularScope)
+        insertDIE(Scope->getScopeNode(), ScopeDIE);
+    }
   }
----------------
I'm somewhat confused about what this code is about/for. Could you help explain it further?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:742-743
@@ +741,4 @@
+  if (!ContextDIE && !SkipContext) {
+    // Can reach this point when the parent scope is a lexical block.
+    // Which was not created yet, or that does not contain code, create it.
+    assert(isa<DILexicalBlockBase>(Context) && "Expected lexical block scope");
----------------
When do we do this while not skipping context?

(also - creating a lexical block that doesn't contain any code may be rather useless - the debugger will never be at a location inside that scope, so it may be difficult to access the variables, etc, there if the debugger implements correct scoping rules... - but I'm not sure. What do other compilers do here?)

================
Comment at: test/DebugInfo/X86/lexical-block-inline.ll:20
@@ +19,3 @@
+;; This test was generated by running following command:
+;; clang -cc1 -O2 -g -emit-llvm foo.cpp 
+;; Where foo.cpp
----------------
Rather than using -O2 which changes a bunch of other stuff in the debug info too, it would be preferable to use no optimizations and add __attribute__((alwaysinline)) (or is it forceinline? I always forget the name) to the function that must be inlined.


Repository:
  rL LLVM

http://reviews.llvm.org/D11180