This is an archive of the discontinued LLVM Phabricator instance.

[llvm-profdata] Add check for text profile formats and improve error reporting
ClosedPublic

Authored by slingn on Nov 10 2015, 4:33 PM.

Details

Summary

This change addresses two possible instances of user error / confusion when
merging sampled profile data.

Previously any input that didn't match the raw or processed instrumented format
would automatically be interpreted as instrumented profile text format data.
No error would be reported during the merge.

Example:
If foo-sampled.profdata and bar-sampled.profdata are binary sampled profiles:

Old behavior:
$ llvm-profdata merge foo-sampled.profdata bar-sampled.profdata -output foobar-sampled.profdata
$ llvm-profdata show -sample foobar-sampled.profdata
error: foobar-sampled.profdata:1: Expected 'mangled_name:NUM:NUM', found lprofi

This change adds basic checks for valid input data when assuming text input.
It also makes error messages related to file format validity more specific about
the assumbed profile data type.

New behavior:
$ llvm-profdata merge foo-sampled.profdata bar-sampled.profdata -o foobar-sampled.profdata
error: foo.profdata: Unrecognized instrumentation profile encoding format
Perhaps you forgot to use the -sample option?

Diff Detail

Event Timeline

slingn updated this revision to Diff 39869.Nov 10 2015, 4:33 PM
slingn retitled this revision from to [llvm-profdata] Add check for text profile formats and improve error reporting.
slingn updated this object.
slingn added reviewers: dnovillo, bogner, davidxl.
slingn added a subscriber: llvm-commits.
davidxl edited edge metadata.Nov 10 2015, 10:31 PM
davidxl added a subscriber: davidxl.

Looks good, but if the message is augmented to be something like the
following:

error: foo.profdata: Malformed instrumented profile data. Perhaps you
forgot to use -sample option?

would make it even more friendlier.

thanks,

David

dnovillo edited edge metadata.Nov 11 2015, 7:09 AM

Could you add a test for the new behaviour that you described in the patch?

Thanks.

lib/ProfileData/SampleProfReader.cpp
257

This generic check won't let us distinguish text formats for sample vs instrumented profiles. I'd suggest actually trying to parse a single line here (factor it out of the text parser) rather than checking for ASCII characters.

slingn updated this revision to Diff 39964.Nov 11 2015, 1:16 PM
slingn edited edge metadata.

Updated for davidxl and dvoillo feedback.

slingn marked an inline comment as done.Nov 11 2015, 1:19 PM
slingn added inline comments.
lib/ProfileData/SampleProfReader.cpp
257

Good point. The sample profile text format is structured enough to enable that. Instrumented profile text format not so much.

slingn updated this object.Nov 11 2015, 1:20 PM

The only limitation here is that when a sample text file is passed to llvm-profdata tool without -sample option, the tool can still not give very useful information -- supporting that requires non trivial refactoring so that 'hasFormat' interfaces are moved to a common place. Probably not worth the effort.

Looks good to me. Watch out for more comments post-commit.

dnovillo accepted this revision.Nov 12 2015, 7:29 AM
dnovillo edited edge metadata.

LGTM. Thanks for fixing this.

This revision is now accepted and ready to land.Nov 12 2015, 7:29 AM
slingn updated this revision to Diff 40063.Nov 12 2015, 9:33 AM
slingn marked an inline comment as done.
slingn edited edge metadata.

Fix indentation.

This revision was automatically updated to reflect the committed changes.