This is an archive of the discontinued LLVM Phabricator instance.

Add DWARFContext generation from LinkGraphs, use in perf support
ClosedPublic

Authored by pchintalapudi on Mar 19 2023, 1:54 PM.

Details

Summary

This patch adds line numbers to perf jitdump records emitted by the PerfSupportPlugin, by parsing and using a DWARFContext from preserved debug sections.

Depends on D146169

Diff Detail

Event Timeline

pchintalapudi created this revision.Mar 19 2023, 1:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2023, 1:54 PM
pchintalapudi requested review of this revision.Mar 19 2023, 1:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2023, 1:54 PM

Thanks very much @pchintalapudi!

Is it possible to write a testcase for this?

llvm/lib/ExecutionEngine/Orc/DebugInfoSupport.cpp
1 ↗(On Diff #506420)

Needs LLVM licensing header.

llvm/lib/ExecutionEngine/Orc/Debugging/PerfSupportPlugin.cpp
13

Needs LLVM licensing header.

120–124

I probably missed this in the previous review, but any debug lines should be wrapped in an LLVM_DEBUG macro prior to commit.

223

This is a good question.

Right now errors can be sent to ExecutionSession::reportError and they won't automatically take down the session. If you give this error a discernible type ("BadDWARF"?) then clients could choose to ignore completely or log based on that.

  • Address review comments

For testing, there are a couple of options that I think should be possible.

Right now D146169's test is only that the jitdump file exists, not on the accuracy of the content. I've been verifying the content of that file using my own custom python script, so one option is to upload that script (to where?), output to json, and check that json for accuracy (with what tools?).

Another option is to take advantage of the cross-process support and replace the default called addresses in the plugin with calls to verification functions. This would probably take the form of a new cpp unit test (don't know how to do this) and has the advantage of being able to test the state on structured data rather than reading a binary file that's sensitive to bugs in the writer.

Diff against parent revision

Propagate stacked change

lhames accepted this revision.Mar 20 2023, 7:11 PM

For testing, there are a couple of options that I think should be possible.

Right now D146169's test is only that the jitdump file exists, not on the accuracy of the content. I've been verifying the content of that file using my own custom python script, so one option is to upload that script (to where?), output to json, and check that json for accuracy (with what tools?).

If you were going to go this way then I think the right thing would be to add your test tool to llvm/utils and then register it along with the other known tools in llvm/test/lit.cfg.py. It's probably worth workshopping that idea on discord / discourse though.

Another option is to take advantage of the cross-process support and replace the default called addresses in the plugin with calls to verification functions. This would probably take the form of a new cpp unit test (don't know how to do this) and has the advantage of being able to test the state on structured data rather than reading a binary file that's sensitive to bugs in the writer.

Yeah -- there are a few ways to approach this, but they're all a bit awkward with the tools we have available. I'll have a think about what we can do to improve this.

I don't think the testcase discussion should prevent this landing. LGTM. Thanks again Prem!

This revision is now accepted and ready to land.Mar 20 2023, 7:11 PM
This revision was landed with ongoing or failed builds.Sep 28 2023, 4:14 PM
This revision was automatically updated to reflect the committed changes.