This is an archive of the discontinued LLVM Phabricator instance.

Fixed debug info emitting for imported entity defined directly in a function scope.
Needs ReviewPublic

Authored by aaboud on Sep 16 2015, 2:31 PM.

Details

Reviewers
dblaikie
samsonov
Summary

Imported entity defined in a lexical block is handled correctly by emitting a debug info entry in each lexical block instance (abstract, inline and concrete), if it has code.
However, if the imported entity was defined in a function scope (top level), it is emitted only in the concrete function, even if it has no code!

This patch solve the issue by handling imported entity defined in a function scope similar to the one defined in a lexical block scope.

Diff Detail

Repository
rL LLVM

Event Timeline

aaboud updated this revision to Diff 34926.Sep 16 2015, 2:31 PM
aaboud retitled this revision from to Fixed debug info emitting for imported entity defined directly in a function scope..
aaboud updated this object.
aaboud added a reviewer: dblaikie.
aaboud set the repository for this revision to rL LLVM.
aaboud added a subscriber: llvm-commits.
aaboud added a subscriber: echristo.

Hi,
Can I get approval or feedback on this patch?
Thanks

aprantl added inline comments.Sep 24 2015, 8:27 AM
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
336

HasNoneScopeChild is uninitialized here.

538

This should probably be a reference like Children.

552

This looks like a use of an undef variable?

Thanks for your comments.
Answered below.

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
336

I guess it won't hurt to initialize it to "false", though the "createScopeChildrenDIE" will always initialize it.

538

It is a pointer and not a reference because not all callers need this information, so they can simply not send a place holder for this return info.

Notice that I followed the previous implementation, just changed the counter with an indicator, as the counter meant to be used as indication for if the lexical scope has some children that are not other lexical scopes.

552

Not exactly.
This checks that the pointer is not "nullptr".
HasNoneScopeChild is a pointer with a nullptr default value.
As not all callers interested in this information.

Ah yes. Sorry for the noise!

dblaikie edited edge metadata.Sep 24 2015, 4:00 PM

Just glanced at the tests, haven't looked at the code yet.

(not sure if the test filecheck lines should be further simplified by dropping some of the attributes - the basics of line/file location on imported entities is tested elsewhere, perhaps this test should just focus on where the imported entities are placed in the DIE tree)

test/DebugInfo/imported_entities.ll
13

Why do you need SROA here?

20

I usually write these tests by using attribute((always_inline)) then even at -O0 clang will run LLVM's AlwaysInliner and inline this, rather than relying on the normal inliner choosing to inline the function.

27

Is the parameter necessary? If you need to leave breadcrumbs to ensure that foo is inlined and not optimized away, you can add a call to an external function in the inlined function:

void f1();
__attribute__((alwaysinline)) void f2() {
  using ...;
  f1();
}
void f3() {
  f2();
}
aaboud marked an inline comment as done.Sep 29 2015, 7:43 AM
aaboud added inline comments.
test/DebugInfo/imported_entities.ll
13

It makes the LLVM IR simpler by removing the "lifetime" intrinsic that is added by the "-inline" pass.
Other than that it is not needed.

Anyway, once I use the "always_inline" attribute you suggested, there is no need for "-inline" nor "-soa" passes.

20

Turned to be a better way to write the test.
I just needed to keep the static, so clang can get rid of the inlined function once it inline it.

aaboud updated this revision to Diff 35977.Sep 29 2015, 7:44 AM
aaboud edited edge metadata.

Following David comments, I managed to reduced the new test and make it much simpler.

aaboud updated this revision to Diff 36460.Oct 3 2015, 11:47 PM

Changed the "static" keyword with "inline" keyword in the LIT test.

David, do I have an approval to commit? Or you would like to take another look on the code?

Thanks

"Imported entity defined in a lexical block is handled correctly by emitting a debug info entry in each lexical block instance (abstract, inline and concrete), if it has code."

Is that the right behavior? (maybe an intermediate step towards consistency, with the end goal being to put the imported entities only in the abstract origin, if available, otherwise in the concrete function?)

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
336

I'm OK leaving it uninitialized if that's the intent - it means tools like MSan and the like can catch cases where the intent is violated (if we initialize it, then fail to set it where we intend, then use it - these tools can't help us because they can't distinguish the intentional from the unintentional)

340

What's the purpose of the "HasNoneScopeChild" parameter compared to just testing if Children is non-empty after the call?

I take it it's to detect whether Children has /immediate/ children, or only more scopes? (hmm, I don't remember that out parameter "ChildScopeCount" going in - not sure when that happened, maybe I did that for all I know/recall... it's been a while since I've played with this code)

Anyway, I think I follow/seems reasonable enough...

343

Why did this need to sink into createScopeChildrenDIE?

549

check formatting (run clang-format over the patch?)

553

prefer !empty over size != 0

Also could you explain why the change from from using a size difference (size - child count without scopes) to an absolute non-empty test? I'm not quite following.

test/DebugInfo/imported_entities.ll
35

This is the abstract definition, yes?

42

These are imported entities without any scope parenting? & are probably useless at the moment, I imagine? (but not a regression caused by your change, just the current state of affairs - maybe a comment in the test case describing what they should be in the end (either removed, or adding scope information/removing the imported entities from the actual inlined and concrete instances, etc))

Hi David,
Thanks for the comments, I tried to answer them.
But, I feels that you are not following the change I am trying to make in this patch.
I described it in the summary, please let me know if the summary is not clear enough.

Thanks

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
343

Because, we want to make imported entities defined in the function top level scope behave as those defined in a lexical scope.
"createScopeChildrenDIE" is the only function that is called for both lexical scope and function.
"constructScopeDIE" is not called for function, only for lexical block (or inlined function - not relevant).

549

Will do.
Though, I think I already did and this was the result, but I will double check.

553

I will use the "empty()" method.

Why I changed that to an indicator instead of a counter?

  1. Because this is the right behavior.
  2. The reason it was a counter, is that we had to add the imported entities outside this function, so we could not calculate the indicator at this function, now we can.
  3. The indicator is needed for next patch, and I really do not want to change to indicator in the next patch to prevent force unrelated (but needed) changes there.

Is there any reason why you would prefer a counter that will never be used as a counter?

test/DebugInfo/imported_entities.ll
35

Yes.

42

I am not quite following.
These imported entities are defined inside the abstract function under the abstract function scope.
According to my plan, this is where they will keep being defined.
So, how do you see the final solution looks like?
Where do you want to define imported entities with respect to inline and abstract functions/lexical scopes?

aaboud updated this revision to Diff 37345.Oct 14 2015, 6:37 AM

Applied David comments.

David, do you have other comments or last minute thoughts?

David,
Any more comments?

aaboud added a comment.Nov 5 2015, 5:51 AM

Can I commit this patch?
Is there any more comments?

Thanks,
Amjad

aaboud updated this revision to Diff 42851.Dec 15 2015, 7:09 AM
aaboud removed rL LLVM as the repository for this revision.

Updated code to top of trunk.
This commit is still needed as first patch for complete implementation of "RFC: Supporting all entities declared in lexical scope in LLVM debug info"
http://lists.llvm.org/pipermail/llvm-dev/2015-December/093313.html