Page MenuHomePhabricator

[Coverage] Fix an issue where improper coverage mapping data could be loaded for an inline function.
ClosedPublic

Authored by ikudrin on May 16 2016, 8:37 AM.

Details

Summary

If an inline function is observed but unused in a translation unit, a dummy coverage mapping data with a zero hash is stored for this function. If such a coverage mapping section came earlier than real one, the latter was ignored. As a result, llvm-cov was unable to show coverage information for those functions.

Diff Detail

Repository
rL LLVM

Event Timeline

ikudrin updated this revision to Diff 57349.May 16 2016, 8:37 AM
ikudrin retitled this revision from to [Coverage] Fix an issue where improper coverage mapping data could be loaded for an inline function..
ikudrin updated this object.
ikudrin added reviewers: bogner, davidxl, vsk.
ikudrin added a subscriber: llvm-commits.

The motivation sample for this patch is the following:

$ cat > sample.h << EOF
inline int sample_func(int A) {
  if (A > 0)
    return A;
  return 0;
}
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
warning: 1 functions have mismatched data. 
sample.cpp:
       |    1|#include "sample.h"
       |    2|
      1|    3|int main() {
      1|    4|  return sample_func(5);
      1|    5|}

After applying the patch, the output is the following:

$ llvm-cov show a.out -instr-profile default.profdata
sample.h:
      1|    1|inline int sample_func(int A) {
      1|    2|  if (A > 0)
      1|    3|    return A;
      0|    4|  return 0;
      1|    5|}

sample.cpp:
       |    1|#include "sample.h"
       |    2|
      1|    3|int main() {
      1|    4|  return sample_func(5);
      1|    5|}
vsk edited edge metadata.May 16 2016, 9:39 AM

Thanks, just two comments.

lib/ProfileData/Coverage/CoverageMappingReader.cpp
408 ↗(On Diff #57349)

I'd find this a bit easier to read if InsertResult.first->second had its own variable.

408 ↗(On Diff #57349)

If !InsertResult.second && FuncHash, wdyt of creating an error if Records[InsertResult.first->second].FunctionHash != FuncHash?

ikudrin added inline comments.May 16 2016, 9:45 AM
lib/ProfileData/Coverage/CoverageMappingReader.cpp
408 ↗(On Diff #57349)

It makes sense, but I think it would be better to make that change in another patch.

vsk accepted this revision.May 16 2016, 10:43 AM
vsk edited edge metadata.

This lgtm with the nit I had above, then. Please wait a bit in case there are other comments.

This revision is now accepted and ready to land.May 16 2016, 10:43 AM
davidxl edited edge metadata.May 16 2016, 11:00 AM

It is possible to handle this without requiring changing the hash value.

What is needed is a helper method to check if the coverageMapping record is 'dummy' -- i.e. has any execution count. It not, it can be skipped or replaced.

See CoverageMapping::load method:

ErrorOr<int64_t> ExecutionCount = Ctx.evaluate(Region.Count);
 if (!ExecutionCount)
   break;
ikudrin updated this revision to Diff 57632.May 18 2016, 8:23 AM
ikudrin updated this object.
ikudrin edited edge metadata.
  • Added the class to check if the coverage mapping data is dummy or real.
  • New implementations also works for simple functions with zero hash, so D20287 is not needed anymore.
  • The test case was extended to check simple inline functions in addition to regular ones.
davidxl added inline comments.May 18 2016, 11:59 AM
lib/ProfileData/Coverage/CoverageMappingReader.cpp
445 ↗(On Diff #57632)

It is better to have another wrapper function to check dumminess which also takes FuncHash. The code can be simplified to

If (NewIsDummy || !OldIsDummy)

continue
474 ↗(On Diff #57632)

I think it is better to move duplicate resolution code above here (with a helper function) to improve readabiltity. The helper function can be something like:

InsertFunctionRecordIfNeeded (...)
which does all the work of duplicate detection and existing entry update

{

 ..
 AlreadySeen = ..
 if (!AlreadySeen) {
     Records.push_back(..);
     return;
 }
 IsOldDummy = ..
 IsNewDummy = ..
 if (IsNewDummy || !IsOldDummy) 
    return;
// Now replace old entry
Records[OldIndex] = NewRecord;

}

ikudrin updated this revision to Diff 57787.May 19 2016, 7:45 AM
  • Rebased to the top
  • Extracted the code of filling the mapping records collection, including duplicate resolution logic, into a separate function.
vsk added a comment.May 19 2016, 9:54 AM

I just have nitpicks. Still looks good :).

lib/ProfileData/Coverage/CoverageMappingReader.cpp
297 ↗(On Diff #57787)

nit, could you use Expected<bool> here? That should let you simplify this a bit.

347 ↗(On Diff #57787)

Same Expected<bool> nit.

408 ↗(On Diff #57787)

nit, existing

davidxl added inline comments.May 19 2016, 9:55 AM
lib/ProfileData/Coverage/CoverageMappingReader.cpp
351 ↗(On Diff #57787)

Should this be moved above the if(Hash) so that caller does not need to initialize it.

test/tools/llvm-cov/Inputs/prefer_used_to_unused.cpp
5 ↗(On Diff #57787)

do you need another source file that includes prefer_used_to_unused.h but does not call sampleFunc nor simpleFunc in order to force creation of dummies?

ikudrin added inline comments.May 19 2016, 10:11 AM
lib/ProfileData/Coverage/CoverageMappingReader.cpp
297 ↗(On Diff #57787)

Thanks! I'll update the patch tomorrow.

351 ↗(On Diff #57787)

The caller doesn't need to initialize it. If Hash is 0, RawCoverageMappingDummyChecker::isDummy() does its part of work.

408 ↗(On Diff #57787)

Thanks!

test/tools/llvm-cov/Inputs/prefer_used_to_unused.cpp
5 ↗(On Diff #57787)

I use prefer_used_to_unused.h itself for this, see the comment at line 6 there.

davidxl accepted this revision.May 19 2016, 10:25 AM
davidxl edited edge metadata.

lgtm

This revision was automatically updated to reflect the committed changes.