Page MenuHomePhabricator

[ProfData] Detect if zlib is available
ClosedPublic

Authored by chenwj on Jul 18 2017, 4:17 PM.

Details

Summary

As discussed on [1], if the profile is compressed and llvm-profdata is not built with zlib support, the error message is not informative. Give a better error message if zlib is not available.

[1] http://lists.llvm.org/pipermail/llvm-dev/2017-July/115571.html

Diff Detail

Repository
rL LLVM

Event Timeline

chenwj created this revision.Jul 18 2017, 4:17 PM
davidxl accepted this revision.Jul 18 2017, 4:30 PM

lgtm

lib/ProfileData/InstrProf.cpp
435 ↗(On Diff #107199)

remove {} here.

This revision is now accepted and ready to land.Jul 18 2017, 4:30 PM
chenwj updated this revision to Diff 107273.Jul 19 2017, 3:05 AM

address review comment.

@davidxl I have no commit access. Could you help me commit it if everything looks fine?

dblaikie edited edge metadata.Jul 19 2017, 6:09 AM

Test case?

Test case?

@dblaikie Would you like me to upload a compressed profile data, or ?

Test case?

@dblaikie Would you like me to upload a compressed profile data, or ?

Yep, that'd probably be my approach - a checked in binary profile file with compressed data (including instructions in the .test file about how to reproduce the file in case anyone needs to maintain/modify/etc). (& you'll need "REQUIRES: nozlib" to ensure the test is only run when zlib is not available)

chenwj updated this revision to Diff 107382.Jul 19 2017, 2:34 PM

@dblaikie I upload the profile data and test case. However, the error message I check including the full path of the profile data. Any idea on how can I improve this?

davidxl added inline comments.Jul 19 2017, 2:50 PM
test/tools/llvm-profdata/nocompress.test
5 ↗(On Diff #107382)

CHECK: error: {{.*}}: Profile uses zlib compression ...

dblaikie added inline comments.Jul 19 2017, 3:18 PM
test/tools/llvm-profdata/nocompress.test
1–5 ↗(On Diff #107382)

Include some text here describing how to produce the input file - in case anyone needs to regenerate it later, etc.

dblaikie accepted this revision.Jul 19 2017, 7:32 PM
dblaikie added inline comments.
test/tools/llvm-profdata/nocompress.test
1–10 ↗(On Diff #107405)

You don't need to use a prefix for these lines - since this file isn't parsed by anything except Lit and FileCheck (which both only handle lines with prefixes) any non-prefix lines are ignored/irrelevant and can be freeform.

chenwj added inline comments.Jul 20 2017, 3:43 AM
test/tools/llvm-profdata/nocompress.test
1–10 ↗(On Diff #107405)

I just copy and modify the instructions from c-general.test. Do you still want me remove those REGENERATE prefix?

@dblaikie ping? :-)

Oh, sorry - I already approved this. Approval with comments usually means "I assume the requested changes are simple/clear/obvious enough to not require another round of review", etc.

In any case - looks good to commit after the "REGENERATE:" prefixes are removed. (did you see the discussion on the mailing list? Not all emails get reflected in Phabricator, and I replied to your question about the REGENERATE Prefix directly on the mailing list - I removed it in the other test that used it, so please remove it from this one before committing)

chenwj updated this revision to Diff 107714.Jul 21 2017, 1:53 PM

@dblaikie Done. I need your help here since I don't have commit access. Thanks. :)

This revision was automatically updated to reflect the committed changes.