Page MenuHomePhabricator

[IRSim][IROutliner] Adding DebugInfo handling for IR outlined functions.
Needs ReviewPublic

Authored by AndrewLitteken on Sep 8 2020, 10:33 AM.

Details

Reviewers
paquette
Summary

Including debug instructions from different places in the program could cause issues with debuggers. This makes sure that any debug information about original location in outlined functions is stripped accordingly, and adds new debug information to the outlined functions.

Tests Changed:

  • llvm/test/Transforms/IROutliner/legal-debug.ll

New Tests:

  • llvm/test/DebugInfo/AArch64/ir-outliner.ll

Diff Detail

Event Timeline

AndrewLitteken created this revision.Sep 8 2020, 10:33 AM

I know almost nothing about how DebugInfo is represented in llvm/dwarf, but is there not a way to represent what this transformation is doing in DI? It's always a little sad when you have to drop it, since doing so makes the debug experience worse.

llvm/lib/Transforms/IPO/IROutliner.cpp
408

is the StringRef() ctor call necessary?

447

where does moveFunctionData get called with MoveDebug set the other way?

aprantl added a subscriber: vsk.

Updating for string refs.

vsk added a comment.Sep 16 2020, 9:58 AM

CodeExtractor contains a utility to fix up debug info after extraction occurs (I believe it's called fixupDebugInfoPostExtraction). It's solving the same problem - is there any way to generalize/reuse that logic?

llvm/test/DebugInfo/AArch64/ir-outliner.ll
3

Please validate the debug info metadata in the IR after -iroutliner runs. Validating the final DWARF production is nice, but checking the IR takes precedent (why? well DWARF may not represent everything we care about at the IR level, and DWARF isn't the only debug info format llvm supports).

In D87302#2277131, @vsk wrote:

CodeExtractor contains a utility to fix up debug info after extraction occurs (I believe it's called fixupDebugInfoPostExtraction). It's solving the same problem - is there any way to generalize/reuse that logic?

I believe it already uses it since the CodeExtractor is used to create the new functions in each case, but this is mainly for the function that moves them all together. The problem is that fixupDebugInfoPostExtraction transfers the information from the original function, which would not be correct in this case since we are merging the information from several different code locations into one. This also addresses @jroelofs disappointment, but since I'm not aware of any mechanism to say that one particular line in IR is from two different source code locations, I think it has to be dropped altogether for correctness.

Right now the current mechanism in lldb is simply skipping over the section of code, which isn't ideal, but it is consistent.

vsk added a comment.Jan 3 2021, 5:08 PM
In D87302#2277131, @vsk wrote:

CodeExtractor contains a utility to fix up debug info after extraction occurs (I believe it's called fixupDebugInfoPostExtraction). It's solving the same problem - is there any way to generalize/reuse that logic?

I believe it already uses it since the CodeExtractor is used to create the new functions in each case, but this is mainly for the function that moves them all together. The problem is that fixupDebugInfoPostExtraction transfers the information from the original function, which would not be correct in this case since we are merging the information from several different code locations into one. This also addresses @jroelofs disappointment, but since I'm not aware of any mechanism to say that one particular line in IR is from two different source code locations, I think it has to be dropped altogether for correctness.

I see, thanks for explaining.

Right now the current mechanism in lldb is simply skipping over the section of code, which isn't ideal, but it is consistent.

Sgtm, please carry on!

This updates the diff with all the recent IROutliner updates, and adds testing for verifying the IR DebugInfo.

@vsk Is there other verification that needs to happen to the outlined IR code besides just making sure with Filecheck that it is correct?

vsk added a comment.Feb 1 2021, 9:43 AM

No, that looks reasonable to me. (Although, it is unfortunate that update_test_checks.py hardcodes metadata numbers: this creates more of a maintenance burden than is ideal. It'd be nice to revamp the way the script emits metadata check lines, but that's outside the scope of this patch.)