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

[Aboud, Amjad]

Dave, can you answer my questions below?
Do you still want me to try reduce the tests?

Thanks,
Amjad

-----Original Message-----
From: Amjad Aboud [mailto:amjad.aboud at intel.com] 
Sent: Sunday, June 07, 2015 11:14
To: Aboud, Amjad; echristo at gmail.com; benny.kra at gmail.com; dblaikie at gmail.com
Cc: friss at apple.com; Paul_Robinson at playstation.sony.com; llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH] Fixed debug info generation for function static variables, typedef, and records

================
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/


---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.