This is an archive of the discontinued LLVM Phabricator instance.

Fix cloning of debug info
ClosedPublic

Authored by yuyichao on Feb 11 2016, 12:46 PM.

Details

Summary

Export function to clone function debug info and autmatically clone the debug info in CloneModule.

This currently causes a regression in the debug info of the generated code we saves to a file since some of the debug info are not copied. (especially after http://reviews.llvm.org/D14265)

Test done: The code copied into julia fixes the missing debug info on both x64 and aarch64.

Diff Detail

Repository
rL LLVM

Event Timeline

yuyichao updated this revision to Diff 47703.Feb 11 2016, 12:46 PM
yuyichao retitled this revision from to Fix cloning of debug info.
yuyichao updated this object.
yuyichao added reviewers: pcc, loladiro.
yuyichao added subscribers: vtjnash, carnaval, llvm-commits.

While it's nice that you've verified this with julia, it really should have an in-tree test.

include/llvm/Transforms/Utils/Cloning.h
134 ↗(On Diff #47703)

Please use triple-slash so doxygen will pick this up.

I'll fix the comment.

Any pointer to a similar test and/or how/where I should add such test?

loladiro edited edge metadata.Feb 11 2016, 3:32 PM

unittests/Transforms/Utils/Cloning.cpp is probably the right place.

yuyichao updated this revision to Diff 47744.Feb 11 2016, 4:20 PM
yuyichao edited edge metadata.

Fix comment/doc in the header.
Add unit test. Verified that the added test only passes when the CloneDebugInfoMetadata line in CloneModule is added.

loladiro accepted this revision.Feb 12 2016, 4:22 PM
loladiro edited edge metadata.
loladiro added a subscriber: aprantl.

LGTM, but I'd want @pcc to sign off before this gets committed. Also cc to @aprantl since this is DI related.

This revision is now accepted and ready to land.Feb 12 2016, 4:22 PM
pcc accepted this revision.Feb 12 2016, 4:36 PM
pcc edited edge metadata.

LGTM

unittests/Transforms/Utils/Cloning.cpp
471 ↗(On Diff #47744)

Maybe also test that one of the subprogram fields has an expected value?

This revision was automatically updated to reflect the committed changes.

I was looking at this patch in the context of a related debug info cloning patch (D17338). If I understand how this and the CloneDebugInfoMetadata routine works, I think this may have some unintended consequences.

Specifically, CloneDebugInfoMetadata will not only clone the debug info metadata for the old function and place it on the new function, but will also add the cloned subprogram metadata to the list of subprograms hanging off the old module's DICompileUnit via AddOperand. This subprogram list will eventually be cloned to the dest module when named metadata is cloned.

Won't this leave the new cloned subprogram on the old module? Also, not sure what happens when the old subprogram list is mapped into the new module when the named metadata is cloned. Will the new subprogram end up in that list twice (once when mapping the old subprogram and once when mapping the new subprogram which probably uses the identify mapping since it isn't in the value map)?

If my understanding is incorrect, what am I missing?

pcc added a subscriber: eugenis.Mar 29 2016, 4:09 PM

@tejohnson I think you are correct. We (me and @eugenis) also observed another problem: this change introduces O(n^2) complexity in the module cloner. I have sent out D18583 that should solve both problems.

In D17165#386322, @pcc wrote:

@tejohnson I think you are correct. We (me and @eugenis) also observed another problem: this change introduces O(n^2) complexity in the module cloner. I have sent out D18583 that should solve both problems.

Yep, I could imagine that since the cloned debug is continually added to the source module. Thanks for the fix, will take a look at D18583.