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

[Amjad Aboud]

================
Comment at: include/llvm/ADT/iterator_range.h:42
@@ -41,1 +41,3 @@
+
+  bool empty() const { return begin() == end(); }
 };
----------------
dblaikie wrote:
> I wouldn't add this here - the range concept is just "begin" and "end" - so there may be other ranges that don't have "empty". So we don't want to make our range-based algorithms overly restrictive by requiring that their ranges have 'empty'.
> 
> Instead this should be a free function:
> 
>   template<typename R>
>   bool empty(const R& r) { return begin(r) == end(r); }
Done.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:357
@@ +356,3 @@
+      for (const auto &DI : LocalDeclNodeRangeForScope) {
+        if (auto IE = dyn_cast<DIImportedEntity>(DI.second))
+          Children.push_back(constructImportedEntityDIE(IE));
----------------
dblaikie wrote:
> Please include the '*' for auto bindings to pointer type. ie:
> 
>   if (auto *IE = dyn_cast<DIImportedEntity>(DI.second))
> 
> etc
Done.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:81
@@ -80,3 +80,3 @@
 
-  /// getOrCreateGlobalVariableDIE - get or create global variable DIE.
-  DIE *getOrCreateGlobalVariableDIE(const DIGlobalVariable *GV);
+  /// \brief get or create global variable DIE.
+  /// \param GV Global Variable Node
----------------
dblaikie wrote:
> I think we have auto-brief on now, so there's no need to put \brief there.
> 
> Also, capitalize 'g' in get.
Done.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:454
@@ +453,3 @@
+    for (auto *GV : CUNode->getGlobalVariables()) {
+      if (CU.getOrCreateContextDIE(GV->getScope()))
+        CU.getOrCreateGlobalVariableDIE(GV);
----------------
dblaikie wrote:
> Do we need to actually try to build the context here or is there something cheaper/more specific we could do? Check the node type of the scope To see if it's a lexical or function scope, perhaps?
Checking if the scope is lexical scope might be cheaper, only for the cases where the answer is true.
However, I am keeping same implementation as the imported entries, see the condition used in "constructAndAddImportedEntityDIE".

Also, to make the implementation complete and prevent it from break in the future, we should try to build the context, because the return value of this function is the reason why we want to delay creation of the children DIEs.

So, right now it is only lexical blocks that this function does not handle (and returns NULL for), but if you add (or remove) more cases, then no need to go back and change the condition here, we will still be safe with calling the getOrCreateContextDIE.

Also, notice that there is no way that we are creating a context DIE that we will not be creating later, and as we are using map, we will not pay the cost twice.

Do you still think we should check for lexical scope parent instead of the current condition?

================
Comment at: test/DebugInfo/LB-class.ll:1
@@ +1,2 @@
+; RUN: llc -mtriple=x86_64-unknown-linux-gnu -filetype=obj -O0 < %s  | llvm-dwarfdump -debug-dump=info - | FileCheck %s
+
----------------
dblaikie wrote:
> What's the "LB" suffix in these tests?
> 
> It seems like these could all be in a single test, probably? The tests should probably be simpler, too. Like:
> 
>   void func() {
>     {
>     struct foo {
>     };
>     foo f;
>     typedef int bar;
>     bar b;
>     }
>   }
> 
> etc?
LB stands for Lexical Block, as all the cases are related to lexical block handling in Clang and LLVM assembler.

I do not mind simplify the tests, I just was not sure if test like the above will create any code, but will give it a shot.
However, I am not sure that having one test instead of 3 separate tests is better, it will be easier to catch the right problem when any of the 3 tests fail, rather than investigate the failure of the all in one test.

Alright, I tried to generate code using the example test above, but there was no assembly code generated for this test, and thus no debug info DIEs for "foo: and "bar".

If you still think the tests should be simplify, I will try reduce them a little.
Please, let me know if I should.

http://reviews.llvm.org/D9758

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/