Page MenuHomePhabricator

Ignore object files that lack coverage information. Before this change, if multiple binary files were presented, all of them must have been instrumented or the load would fail with coverage_map_error::no_data_found.
ClosedPublic

Authored by deansturtevant on Aug 26 2019, 11:20 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2019, 11:20 AM
vsk added a comment.Aug 26 2019, 1:30 PM

Thanks for the patch. I'd like for the test to be minimized as far as possible, e.g.:

obj1.o: not instrumented, no functions
obj2.o: instrumented, just contains 'void f1() {}'

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
317 ↗(On Diff #217211)

Could you simply check whether 'Readers' is empty after the loop, and return data_not_found if so? Then there's no need to update an Error object.

llvm/lib/ProfileData/InstrProfReader.cpp
45 ↗(On Diff #217211)

Perhaps this could be a follow-up change? I'm not sure what it has to do with the main change.

deansturtevant marked 2 inline comments as done.Aug 26 2019, 2:10 PM

We can simplify by not propagating data_not_found, but rather manufacturing one if desired.
However, that would result in any information in the original payload being lost.
Let me know if that is ok.

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
317 ↗(On Diff #217211)

That would change the logic so that passing in an empty ObjectFilenames would result in a data_not_found error. I'm not convinced that's the desired behavior, but would be willing to implement it if you think it is.

Please let me know what your preference is.

llvm/lib/ProfileData/InstrProfReader.cpp
45 ↗(On Diff #217211)

I will remove it.

vsk added a comment.Aug 26 2019, 2:29 PM

Thanks, it should be fine to ignore the payload of the data_not_found error.

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
317 ↗(On Diff #217211)

Sure, this should just create an error if ObjectFilenames is non-empty.

deansturtevant marked 3 inline comments as done.Aug 27 2019, 7:39 AM

Thanks. Please take another look.

vsk added a comment.Aug 27 2019, 8:56 AM

Thanks. Please take another look.

Hm, perhaps the diff update was dropped somehow? I don’t see a change yet.

Responded to review comments. Simplified the code and the tests.

Sorry. User error. Should be up now.

vsk accepted this revision.Aug 27 2019, 1:04 PM

Looks great. Thank you!

This revision is now accepted and ready to land.Aug 27 2019, 1:04 PM

Thanks for the review including some great suggestions.
I do not have a commit account. Could you commit this for me? Thanks again.

This revision was automatically updated to reflect the committed changes.