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

[David Blaikie]

REPOSITORY
  rL LLVM

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:454
@@ +453,3 @@
+    for (auto *GV : CUNode->getGlobalVariables()) {
+      if (CU.getOrCreateContextDIE(GV->getScope()))
+        CU.getOrCreateGlobalVariableDIE(GV);
----------------
aaboud wrote:
> 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?
The code in constructAndAddImportedEntityDIE is a bit more legible because the context is used. In this case we call this "getOrCreate" but then discard the result - it looks weird/confusing.

We could change the other APIs to take the context and we could pass it in to the getOrCreate functions. I still feel like maybe a comment would be warranted... something like "// defer until the context is created lazily"... 

But the thing is, we only look in the ScopedWtihLocalDeclNodes when creating local scopes - so it's incorrect to imply that if we change the things that getOrCreateScope builds, the code will work. 

... I'm inclined to switch this to a scope check (& possibly include the same condition as an assert in findLocalDeclsForScope). Could an an assert that in the non-local case the entity is actually created - but we don't do that already, so I'm not sure we need to do that after the change?

================
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
+
----------------
aaboud wrote:
> 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.
Looks like you managed to simplify and rename the test - looks good to me

http://reviews.llvm.org/D9758

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