Page MenuHomePhabricator

[Coverage] Ensure that the hash for a used function is non-zero.
AbandonedPublic

Authored by ikudrin on May 16 2016, 9:11 AM.

Details

Reviewers
davidxl
bogner
vsk
Summary

A valid function might not have any statement which affects the hash value, so the hash for that function was zero. The hash value for an unused function is also zero, so the loader could not distinguish the definitions and might load the incorrect one. This patch addresses the issue by assigning a special reserved value as a hash for such a functions.

Diff Detail

Event Timeline

ikudrin updated this revision to Diff 57353.May 16 2016, 9:11 AM
ikudrin retitled this revision from to [Coverage] Ensure that the hash for a used function is non-zero..
ikudrin updated this object.
ikudrin added reviewers: bogner, davidxl, vsk.
ikudrin added a subscriber: cfe-commits.

The motivation sample (using llvm-cov with D20286 applied):

$ cat > sample.h << EOF
inline int sample_func(int A) {
  return A;
}
EOF
$ cat > dummy.cpp << EOF
#include "sample.h"
EOF
$ cat > sample.cpp << EOF
#include "sample.h"

int main() {
  return sample_func(5);
}
EOF
$ clang++ -fprofile-instr-generate -fcoverage-mapping dummy.cpp sample.cpp
$ ./a.out
$ llvm-profdata merge -o default.profdata default.profraw
$ llvm-cov show a.out -instr-profile default.profdata
sample.h:
      0|    1|inline int sample_func(int A) {
      0|    2|  return A;
      0|    3|}

sample.cpp:
       |    1|#include "sample.h"
       |    2|
      1|    3|int main() {
      1|    4|  return sample_func(5);
      1|    5|}

And if this patch is applied:

$ clang++ -fprofile-instr-generate -fcoverage-mapping dummy.cpp sample.cpp
$ ./a.out
$ llvm-profdata merge -o default.profdata default.profraw
$ llvm-cov show a.out -instr-profile default.profdata
sample.h:
      1|    1|inline int sample_func(int A) {
      1|    2|  return A;
      1|    3|}

sample.cpp:
       |    1|#include "sample.h"
       |    2|
      1|    3|int main() {
      1|    4|  return sample_func(5);
      1|    5|}

Does anyone known, why we need dummy coverage mapping records for unused functions? How are they used? Isn't it better to remove these dummy records to prevent confusion with the real ones?

davidxl edited edge metadata.May 16 2016, 10:18 AM

Strictly speaking, this patch requires a version bump of the indexed format. The profile reader also needs to adjust the FunctionHash computation (either using 0 or simple function hash) based on the version of the profile data.

Check with Justin/vsk to see if it is important to keep backward compatability for these simple functions in older profile data.

On the other hand, I wonder what is the real root cause of the problem. The dummy function record does not have its 'own' profile counts, so

if (std::error_code EC = ProfileReader.getFunctionCounts(

Record.FunctionName, Record.FunctionHash, Counts)) {

call in CoverageMapping::load (..)

method should return the the counts of the used function even with zero functionhash. What did I miss?

On the other hand, I wonder what is the real root cause of the problem. The dummy function record does not have its 'own' profile counts, so

if (std::error_code EC = ProfileReader.getFunctionCounts(
            Record.FunctionName, Record.FunctionHash, Counts)) {

call in CoverageMapping::load (..)

method should return the the counts of the used function even with zero functionhash. What did I miss?

There are no problems in ProfileReader, because only real functions have their profile counts. The problem is in VersionedCovMapFuncRecordReader::readFunctionRecords(...), which has difficulties in distinguishing real and dummy coverage mapping records, so that it might load improper ones in some cases.

ikudrin abandoned this revision.May 20 2016, 2:26 AM

This change is not needed anymore because the whole issue was fixed in D20286.