Page MenuHomePhabricator

Fix for "DICompileUnit not listed in llvm.dbg.cu" verification error after cloning a function from a different module
ClosedPublic

Authored by OlegPliss on Tue, Aug 20, 6:33 PM.

Details

Summary

Currently when a function with debug info is cloned from a different module, the cloned function may have hanging DICompileUnits, so that the module with the cloned function fails debug info verification. For a simple example, please see the supplied CloneFunctionToDifferentModule test.

The proposed fix inserts all DICompileUnits reachable from the cloned function to "llvm.dbg.cu" metadata operands of the cloned function module. Duplication of operands is carefully avoided.

Diff Detail

Repository
rL LLVM

Event Timeline

OlegPliss created this revision.Tue, Aug 20, 6:33 PM

Why are the changes to CloneModule.cpp necessary?

Can you provide a testcase showing that we don't copy unnecessary compile units?

Why are the changes to CloneModule.cpp necessary?

CloneModule calls CloneFunction for all functions. These calls insert all DICompileUnits reachable from the functions to "llvm.dbg.cu" metadata. Without the changes in CloneModule those DICompileUnits are inserted again. Duplicate named metadata operands make output of some tests dependent on number of passes of the optimizer pipeline, and the tests fail.

Can you provide a testcase showing that we don't copy unnecessary compile units?

This property is guaranteed by correctness of DIFinder which is not touched by my changes. Only DIFinder.compile_units() are inserted to "llvm.dbg.cu". DIFinder.compile_units() is a set of compile units reachable from original function, and they are shared with the clone.

At a first glance, this seems reasonable, I'm just wondering why this hasn't come up before. @rtereshin, any ideas?

aprantl added inline comments.Wed, Aug 21, 8:25 AM
lib/Transforms/Utils/CloneModule.cpp
199 ↗(On Diff #216306)

Should this be an insertCU helper function in DebugInfo.h?

OlegPliss added inline comments.Wed, Aug 21, 11:58 AM
lib/Transforms/Utils/CloneModule.cpp
199 ↗(On Diff #216306)

If you mean insertCU(DICompileUnit*, SmallPtrSet&), it would only be called twice and would increase number of code lines in the project. The worst, it would not always be called for insertion of DI CUs to "llvm.dbg.cu". Often the code deals with generic operands of generic NMD.

What would really help is proper abstractions for different NMD kinds. The doc says NMD operands is a collection. In general a collection may contain duplicates and is not ordered (and so is not indexable). For "llvm.dbg.cu" and many others a set would be more desired. Here the duplicates just complicate debug info comparison, waste CPU cycles and consume some space in DWARF. Meanwhile CloneModule tries to preserve the operand ordering, and other functions do not.

OlegPliss updated this revision to Diff 216501.Wed, Aug 21, 3:19 PM

Check for unnecessary DI compile units is added to the test on Eli's request.

Can you provide a testcase showing that we don't copy unnecessary compile units?

Done.

Why are the changes to CloneModule.cpp necessary?

Can you provide a testcase showing that we don't copy unnecessary compile units?

Ping. I need an approval to commit the changes.

At a first glance, this seems reasonable, I'm just wondering why this hasn't come up before. @rtereshin, any ideas?

Ping. I need an approval to commit the changes.

aprantl accepted this revision.Tue, Aug 27, 1:14 PM
This revision is now accepted and ready to land.Tue, Aug 27, 1:14 PM
This revision was automatically updated to reflect the committed changes.