This is an archive of the discontinued LLVM Phabricator instance.

[Dwarf] Reference the correct CU when inlining
ClosedPublic

Authored by ellis on Oct 3 2022, 3:31 PM.

Details

Summary

Sometimes when a function is inlined into a different CU, llvm-dwarfdump --verify would find an inlined subroutine with an invalid abstract origin. This is because DwarfUnit::addDIEEntry() will incorrectly assume the inlined subroutine and the abstract origin are from the same CU if it can't find the CU for the inlined subroutine.

In the added test, the inlined subroutine for bar() is created before the CU for B.swift is created, so it tries to point to goo() in the wrong CU. Interestingly, if we swap the order of the two functions then we don't see a crash since the module for goo() is created first.

The fix is to give a parent DIE to ScopeDIE before calling addDIEEntry() so that its CU can be found. Luckily, constructInlinedScopeDIE() is only called once so we can pass it the DIE of the scope's parent and give it a child just after it's created.

constructInlinedScopeDIE() should always return a DIE, so assert that it is not null.

Diff Detail

Event Timeline

ellis created this revision.Oct 3 2022, 3:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2022, 3:31 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
ellis published this revision for review.Oct 3 2022, 3:50 PM
ellis edited the summary of this revision. (Show Details)
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2022, 3:51 PM
aprantl added inline comments.Oct 4 2022, 10:57 AM
llvm/test/DebugInfo/dwarfdump-inlined.ll
2

Can you add CHECKs that actually verify that the dwarfdump output is as expected? RIght now this test will also succeed if llc decides to strip out the debug info entirely ;-)

(this is fine by me - I will say this code is all pretty subtle, but the fix is good - the sooner we can add a DIE to its parent, the better for precisely these sort of reasons)
I'll leave the final approval for @aprantl once he's good with it. (& yeah, I agree - the test should verify that the DIE ended up in the right unit, etc)

dexonsmith resigned from this revision.Oct 4 2022, 1:09 PM
ellis updated this revision to Diff 465146.Oct 4 2022, 1:51 PM

Move test to DebugInfo/Generic and add checks to verify dwarfdump output.

aprantl accepted this revision.Oct 4 2022, 3:36 PM
aprantl added inline comments.
llvm/test/DebugInfo/Generic/cross-cu-inlining-2.ll
61 ↗(On Diff #465146)

I'm not sure if the FORMs will be identical on all targets, so you may need to move this back into a subdirectory where you can restrict this to one specific target and object file format. But the bots will let you know if that's the case.

This revision is now accepted and ready to land.Oct 4 2022, 3:36 PM
kyulee added a comment.Oct 4 2022, 4:44 PM

Thanks for quickly fixing and approving this!

ellis added inline comments.Oct 4 2022, 6:58 PM
llvm/test/DebugInfo/Generic/cross-cu-inlining-2.ll
61 ↗(On Diff #465146)

Thanks for the heads up. I'll land in the morning and make fixes as necessary.

ellis updated this revision to Diff 465433.Oct 5 2022, 9:17 AM

fix whitespace

This revision was landed with ongoing or failed builds.Oct 5 2022, 9:19 AM
This revision was automatically updated to reflect the committed changes.
DianQK added a subscriber: DianQK.Oct 5 2022, 5:47 PM