Page MenuHomePhabricator

[llvm-cov] Add support for loading coverage from multiple objects
ClosedPublic

Authored by vsk on Sep 30 2016, 2:33 AM.

Details

Summary

llvm-cov currently requires the user to pass in a single object filename via a positional arg.

To retain backwards compatibility, I've kept the original positional arg but made it optional. I've also added an optional "-object" flag (a cl::list). You can specify the "-object" flag multiple times, or a single time using a comma-separated list.

Diff Detail

Repository
rL LLVM

Event Timeline

MaggieYi updated this revision to Diff 73017.Sep 30 2016, 2:33 AM
MaggieYi retitled this revision from to [llvm-cov] Allow llvm-cov to report the coverage counters across multiple elf files..
MaggieYi updated this object.
MaggieYi added reviewers: vsk, davidxl, bogner.

It looks like phabricator stripped the first component of the filenames ('tools' and 'test'). Can you please re-upload the diff?

arphaman added inline comments.Oct 12 2016, 8:32 AM
tools/llvm-cov/multiELFsShare.cpp
53 ↗(On Diff #73017)

This test file is missing a newline at the end.

vsk edited edge metadata.Oct 12 2016, 10:34 AM

I think this patch could be simplified if we taught the llvm::CoverageMapping class how to load coverage data from multiple CoverageMappingReaders / multiple filenames. This would require less work in the long run compared to updating all of the reporting logic.

I have a handful of patches that move us in this direction ready locally. The last patch is still WIP, but I hope to have it uploaded for review soon.

vsk added a comment.Oct 17 2016, 2:50 PM

@MaggieYi ping, do you have the bandwidth to update this patch to use the new CoverageMapping::load API? If not, I can take a shot at it.

In D25086#572183, @vsk wrote:

@MaggieYi ping, do you have the bandwidth to update this patch to use the new CoverageMapping::load API? If not, I can take a shot at it.

Hi Vedant,

Since I have been moved to another project for a while, my colleague (Sean Eveson) is looking at this review. Since you are more familiar with the coverage, please go ahead and update the patch if you want to.

Thanks

vsk commandeered this revision.Oct 24 2016, 5:18 PM
vsk edited reviewers, added: MaggieYi; removed: vsk.

Ok. I'll upload my patch for review.

vsk updated this revision to Diff 75662.Oct 24 2016, 5:25 PM
vsk retitled this revision from [llvm-cov] Allow llvm-cov to report the coverage counters across multiple elf files. to [llvm-cov] Add support for loading coverage from multiple objects.
vsk updated this object.
vsk edited reviewers, added: arphaman; removed: bogner, davidxl.
vsk removed a subscriber: arphaman.
  • Update the patch to use the new CoverageMapping::load() API.
arphaman accepted this revision.Oct 25 2016, 7:36 AM
arphaman edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 25 2016, 7:36 AM
This revision was automatically updated to reflect the committed changes.