Page MenuHomePhabricator

[compiler-rt] Add more diagnostic to InstrProfError
ClosedPublic

Authored by gulfem on Aug 30 2021, 1:14 PM.

Details

Summary

If profile data is malformed for any kind of reason, we generate
an error that only reports "malformed instrumentation profile data"
without any further information. This patch extends InstrProfError
class to receive an optional error message argument, so that we can
do better error reporting.

Diff Detail

Event Timeline

gulfem created this revision.Aug 30 2021, 1:14 PM
gulfem requested review of this revision.Aug 30 2021, 1:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2021, 1:14 PM
phosek added a comment.Sep 7 2021, 1:03 PM

I think this is a great change but the messages could be improved. Saying that value is corrupted in every case doesn't really say much plus it's not clear if the value has really been corrupted, there are other ways in which the data can become invalid (like using incompatible runtime version or compiler error). Rather I'd try to explain what's the issue in concise terms. I left a few suggestions, hopefully you'll get the point.

llvm/include/llvm/ProfileData/InstrProf.h
325

I'd call this just getMessage which is what StringError does as well.

342

I'd call this just Msg which is what StringError does as well.

llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
572

I'd omit the CovMapFuncRecordReader which may not be useful for most users.

llvm/lib/ProfileData/InstrProf.cpp
79

I'd use raw_string_ostream to assemble the message. This would like like this:

std::string Msg;
raw_string_ostream OS(Msg);
switch (Err) {
case instrprof_error::success:
  OS << "success";
  break;
...
}
...
return OS.str();
152

Some of the error messages above already include details in parenthesis and having multiple parenthesis might look awkward so I'd use : as a separator instead.

914

These are not full sentences so they shouldn't be capitalized, the same for all the other messages below.. I'd also consider moving the Integrity check failure at the end in parenthesis as an additional qualifier.

Regarding this particular error message, I think you can make it even more specific (corrupted is too generic) by saying something like "number of value profile kinds is invalid".

919

Rather than corrupted, I'd say something like "total size is not congruent modulo quardword size".

925

I'd use "value kind is invalid" not corrupted.

929

Maybe something like "address of the next value profile record is greater than the total size".

Can some test be added?

gulfem updated this revision to Diff 378374.Oct 8 2021, 4:13 PM
  1. Addressed Petr's comments
  2. Add two malformed profile tests
  3. Change error messages in existing malformed profile tests

@davidxl and @phosek did you have a chance to look at the latest revision?

I find it cleaner just to expand known malformed errors into its own error category. WIth that the change can also be minimized.

phosek added inline comments.Oct 26 2021, 10:09 AM
llvm/lib/ProfileData/InstrProf.cpp
153

I'd omit the space here.

llvm/lib/ProfileData/InstrProfReader.cpp
273–274

We could provide some additional details.

281–282
294
344

There might still be some padding, just not the right amount.

452–453
463–482
573
582
phosek added inline comments.Oct 26 2021, 10:17 AM
llvm/lib/ProfileData/InstrProfReader.cpp
463–482

Actually here it could be either the counter offset that's out of bounds, or the number of counters that's out of bounds, so perhaps we should split this into two separate checks.

gulfem updated this revision to Diff 385842.Nov 9 2021, 8:49 AM

Addressed reviwer comments

gulfem added a comment.Nov 9 2021, 8:57 AM

I find it cleaner just to expand known malformed errors into its own error category. WIth that the change can also be minimized.

@davidxl, adding an optional diagnostic error message to InstrProfError gives us the flexibility to do that not only for malformed profiles, but for other kind of profile errors.
I only added for malformed profiles because it was our current use case. But, we can later extend it for other kind of profile issues.
This is why I would prefer not to have another error category for malformed profiles.
Please let me know what you think about that.

davidxl accepted this revision.Nov 9 2021, 9:05 AM

lgtm

This revision is now accepted and ready to land.Nov 9 2021, 9:05 AM
This revision was landed with ongoing or failed builds.Nov 9 2021, 10:08 AM
This revision was automatically updated to reflect the committed changes.

I missed the latest patchset. I have a few more comments, feel free to address those in follow up changes.

llvm/lib/ProfileData/InstrProf.cpp
153

I'd omit the extra quotes, that's more consistent with the way Clang and other tools report errors

llvm/lib/ProfileData/InstrProfReader.cpp
466

In error messages, we usually prefer spelling things out fully, so this should be counter offset X is negative, I'd omit the parenthesis.

470–472

Here I'd say counter offset X is greater that the maximum number of counters Y.

477–481

Similarly here I'd say just number of counters X is greater than the maximum number of counters Y where X would be a sum of CounterOffset and NumCounters, no need to report those separately since you already check the offset above.

@gulfem This is failing on some buildbots: https://lab.llvm.org/buildbot/#/builders/109/builds/25948 please can you take a look?

dyung added a subscriber: dyung.Nov 9 2021, 12:54 PM

@gulfem This is failing on some buildbots: https://lab.llvm.org/buildbot/#/builders/109/builds/25948 please can you take a look?

The test seems to require zlib which means either that dependency needs to be removed, or a "REQUIRES: zlib" needs to be added.

gulfem added a comment.Nov 9 2021, 1:13 PM

@gulfem This is failing on some buildbots: https://lab.llvm.org/buildbot/#/builders/109/builds/25948 please can you take a look?

The test seems to require zlib which means either that dependency needs to be removed, or a "REQUIRES: zlib" needs to be added.

@RKSimon and @dyung,
Thanks for reporting, and I have already pushed a change that removed the dependency to zlib on those tests:
https://github.com/llvm/llvm-project/commit/126e7611c70ca41782aa851c2bec132607eb8127
After the fix, bot turned green.
https://lab.llvm.org/buildbot/#/builders/109/builds/25973

gulfem added a comment.Nov 9 2021, 1:14 PM

I missed the latest patchset. I have a few more comments, feel free to address those in follow up changes.

@phosek, I also addressed your comments in:
https://github.com/llvm/llvm-project/commit/126e7611c70ca41782aa851c2bec132607eb8127