Page MenuHomePhabricator

Fixed debug info generation for function static variables, typedef, and records
ClosedPublic

Authored by aaboud on May 13 2015, 11:59 AM.

Details

Summary

A Fix for Bug 19238.
Function static variable defined inside a lexical scope, same as typedef and record (class, struct or union), which are declared inside a lexical scope, were all associated to the function as its parent scope, rather than to the lexical scope they are defined or declared inside.

This patch provides a fix for all these three cases in the LLVM DWARF debug info generation.

Diff Detail

Repository
rL LLVM

Event Timeline

aaboud updated this revision to Diff 25719.May 13 2015, 11:59 AM
aaboud retitled this revision from to Fixed debug info generation for function static variables, typedef, and records.
aaboud updated this object.
aaboud edited the test plan for this revision. (Show Details)
aaboud added reviewers: echristo, bkramer, dblaikie.
aaboud set the repository for this revision to rL LLVM.
aaboud added a subscriber: Unknown Object (MLST).

I did not get feedback on the fix, yet.
Can you please approve or comment if you disagree on this code change?

I don't claim to be deeply familiar with this code, but it looks like your patch does two things: first, refactor how we eliminate redundant/useless lexical blocks (doing it after processing everything else, instead of right away); second, make sure global (static) variables are attached to the correct scope.
The refactoring part should probably be done as its own patch first; that patch should refer to a test that shows we eliminate redundant scopes correctly, or add one if we don't already have such a test.
Then a follow-on patch would correct how globals are attached to their scopes.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
550 ↗(On Diff #25719)

"Dies" is a vector of compile units, which will never have DW_TAG_lexical_block. You would need to be doing a depth-first traversal of lexical blocks.

aaboud updated this revision to Diff 26382.May 24 2015, 9:02 AM

Split the fix into two change lists as Paul suggested.
You may find the other change list in D9960.

aaboud added a parent revision: Restricted Differential Revision.May 24 2015, 9:03 AM
aaboud updated this revision to Diff 26676.May 28 2015, 6:03 AM

Applied changes according to David Blaikie commits.
Lexical Block DIE are created as before (no change to the order).
However, global variables and retained types are now handled similar to imported entities and captured in a map indexed by their lexical scope context.
Later, this map will be used to add these entries to the lexical block DIE once it is created.

aaboud removed a parent revision: Restricted Differential Revision.May 28 2015, 1:42 PM
friss added a subscriber: friss.May 28 2015, 3:40 PM
friss added inline comments.
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
101 ↗(On Diff #26676)

A comment for the new argument would be nice (here or in the header).

341 ↗(On Diff #26676)

I think the local types and static vars should respect this flag like the imported entities does. If we go the way of unifying the 3 maps (se comments bellow), then everything can be handled inside that if.

354–355 ↗(On Diff #26676)

This could just query .empty() on the containers.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
459 ↗(On Diff #26676)

Is that condition always safe to check if the scope is function-local? Just looking at the name of the function, it looks like we could change that one day to do something different for local scope (like create them right away like the name implies) and this would break the logic here.

lib/CodeGen/AsmPrinter/DwarfDebug.h
261–262 ↗(On Diff #26676)

Not related to your patch, but this look like an orphaned comment.

265–271 ↗(On Diff #26676)

Couldn't we just merge the 3 maps. That are all used only in one and same place, and their handling is really similar. The fundamental difference between the 2 new ones and the existing one is that the DIEs created by getOrCreate{Type,Variable}DIE are automatically added to their parents, which doesn't happen for the imported entities. Maybe we should just unify all that? (I think David has written this part, so he might have an opinion on this)

lib/CodeGen/AsmPrinter/DwarfUnit.cpp
744–745 ↗(On Diff #26676)

Why is that new shortcut needed?

aaboud added inline comments.May 28 2015, 11:09 PM
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
341 ↗(On Diff #26676)

I see no reason to add them to the minimal flag.
We did not skip emitting local variable due to this flag, did we?
Imported Entities should respect the flag, but you will need to explain why other should!

354–355 ↗(On Diff #26676)

I wish it have.
the iterator_range container has no "empty()" method.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
459 ↗(On Diff #26676)

If this function would be able to create a lexical block DIE, I would not need to implement this patch from the first place, right?
Also, if you check line 490. when we construct the Imported Entities, it does the same check. Actually, I copied this from their.

lib/CodeGen/AsmPrinter/DwarfDebug.h
261–262 ↗(On Diff #26676)

OK, will remove.

265–271 ↗(On Diff #26676)

I do not mind merging them, just need some good ideas on how to do that.

lib/CodeGen/AsmPrinter/DwarfUnit.cpp
744–745 ↗(On Diff #26676)

Yes, I thought it is new, it is not. I just moved it from below.

  1. I see no good reason to create the context if we already have the DIE itself.
  2. For Type DIEs with lexical block context, the function ContextDIE from below will be nullptr at this point.
friss added inline comments.May 29 2015, 9:51 AM
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
341 ↗(On Diff #26676)

Actually we do not emit locals in that case. If you lookup the function includeMinimalInlineScopes() you'll see that it is true in line-tables-only mode in which we do not emit locals.

354–355 ↗(On Diff #26676)

righto, I thought this returned a container and I didn't come back to it when reading the function definitions below.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
459 ↗(On Diff #26676)

Sorry, you didn't get my point. You are no just calling function that looks up the scope DIE. As its name implies, it can create things. I'm asking if the side effects of that function are safe to apply here. And if they ever change to return a scope where they don't return any today would it break the logic in getOrCreateGlobalVariable()? Looking at the code there, it seems safe on both fronts because that function is called anyway as one the the very first things (and this also applies to the getOrCreateType() call below).

lib/CodeGen/AsmPrinter/DwarfDebug.h
265–271 ↗(On Diff #26676)

Well, I'd try to put everything in the same map (renaming it beforehand to something like 'LocalDeclMap'. Then in constructScopeDIE, discriminate between the different types using dyn_cast (and handle everything in the same loop under the includeMinimalInlineScopes() condition). Is there anything I'm missing that makes this impractical?

lib/CodeGen/AsmPrinter/DwarfUnit.cpp
744–745 ↗(On Diff #26676)

The comment a few lines below ("Construct the context before querying for the existence of the DIE in case such construction creates the DIE") makes it seem like this is deliberate. Anyway if it's not needed for the patch it shouldn't be here (if you think it is a good idea, it can be the subject of another patch).

aaboud updated this revision to Diff 26856.May 31 2015, 4:31 AM
aaboud removed rL LLVM as the repository for this revision.

Applied more changes following comments from Frederic and Dave.
Thanks guys for the comments.

dblaikie added inline comments.Jun 4 2015, 3:36 PM
include/llvm/ADT/iterator_range.h
42 ↗(On Diff #26856)

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); }
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
357 ↗(On Diff #26856)

Please include the '*' for auto bindings to pointer type. ie:

if (auto *IE = dyn_cast<DIImportedEntity>(DI.second))

etc

lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
81 ↗(On Diff #26856)

I think we have auto-brief on now, so there's no need to put \brief there.

Also, capitalize 'g' in get.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
454 ↗(On Diff #26856)

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?

471 ↗(On Diff #26856)

same here

test/DebugInfo/LB-class.ll
1 ↗(On Diff #26856)

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?

aaboud added inline comments.Jun 7 2015, 1:14 AM
include/llvm/ADT/iterator_range.h
42 ↗(On Diff #26856)

Done.

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
357 ↗(On Diff #26856)

Done.

lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
81 ↗(On Diff #26856)

Done.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
454 ↗(On Diff #26856)

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?

test/DebugInfo/LB-class.ll
1 ↗(On Diff #26856)

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.

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

Thanks,
Amjad

aaboud updated this revision to Diff 28088.Jun 21 2015, 6:55 AM
aaboud set the repository for this revision to rL LLVM.

I updated the patch according to Dave comments.
I also merged all the 4 LIT test into one LIT test and tried minimized it as much as possible why still be able to check all the corner cases I intended to check in the first place. The test also contains description on what it tries to check.

Please, let me know if you have more comments, or approve the patch for commit.

Thanks

dblaikie added inline comments.Jun 25 2015, 2:14 PM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
454 ↗(On Diff #26856)

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?

test/DebugInfo/LB-class.ll
1 ↗(On Diff #26856)

Looks like you managed to simplify and rename the test - looks good to me

aaboud updated this revision to Diff 28643.Jun 29 2015, 12:15 AM

Applied Dave last comments.
Also, updated the LLVM version to 240893 (close to top of trunk preparing for commit).

aaboud updated this revision to Diff 28771.Jun 30 2015, 5:42 AM

Followed David suggestion.
Now, "findLocalDeclNodesForScope()" is called only if "includeMinimalInlineScopes" is not present.

This, should improve performance and clarity of code.

dblaikie edited edge metadata.Jun 30 2015, 9:39 AM

Looks good - thanks for your patience and perseverance!

Please commit, or, if you don't have commit access, I can commit for you.

This revision was automatically updated to reflect the committed changes.