[Coverage] Take filenames into account when loading function records.

[Dor1s]

Summary

Don't skip functions with the same name but from different files.

That change makes it possible to generate code coverage reports from
different binaries compiled from different sources even if there are functions
with non-unique names. Without that change, code coverage for such functions is
missing except of the first function processed.

[Dor1s]

Please take a look. That doesn't seem to break any tests. If there aren't any concerns, I'll add a test that would pass with this change and would fail without it.

[vsk]

No concerns from me, as we appear to deduplicate function records at a lower level (in VersionedCovMapReader).

I'll note that we appear to take this early return a few times while running tests: http://lab.llvm.org:8080/coverage/coverage-reports/llvm/coverage/Users/buildslave/jenkins/workspace/clang-stage2-coverage-R/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp.html#L212

I'd be curious to know which tests trigger the path, and whether they are simply too relaxed?

[Dor1s]

Thanks Vedant! The following tests trigger that branch:

LLVM-Unit :: ProfileData/./ProfileDataTests/ParameterizedCovMapTest/CoverageMappingTest.skip_duplicate_function_record/0
LLVM-Unit :: ProfileData/./ProfileDataTests/ParameterizedCovMapTest/CoverageMappingTest.skip_duplicate_function_record/1
LLVM-Unit :: ProfileData/./ProfileDataTests/ParameterizedCovMapTest/CoverageMappingTest.skip_duplicate_function_record/2
LLVM-Unit :: ProfileData/./ProfileDataTests/ParameterizedCovMapTest/CoverageMappingTest.skip_duplicate_function_record/3
LLVM :: tools/llvm-cov/load-multiple-objects.test
LLVM :: tools/llvm-cov/multiple-objects.test
LLVM :: tools/llvm-cov/universal-binary.c

And the first 4 actually fail with my change. Looks like I've tested something wrong previously (I think I did only check-llvm-tools rather then check-llvm), my bad. I'm looking into those tests now to see their purpose.

[Dor1s]

Added an example of test that would fail without the change and pass with it.

[Dor1s]

Well, it doesn't look like skip_duplicate_function_record make a lot of sense...

You're right that other tests might be hardened in order to test that functionality. Please see my update to multiple-objects.test, I think it makes sense. Do you agree?

[Dor1s]

fix a typo

[Dor1s]

update skip_duplicate_function_record to reflect a change necessary to fix the issue

[Dor1s]

Another option would be to implement loadFunctionRecord in a way that it would pass the updated CoverageMappingTest.cpp test.

[vsk]

In D46478#1090502, @Dor1s wrote:

LLVM-Unit :: ProfileData/./ProfileDataTests/ParameterizedCovMapTest/CoverageMappingTest.skip_duplicate_function_record/0

This test was added in r284251 along with the initial support for loading coverage data from multiple object files. The basic idea behind seems reasonable, as it makes sense to skip records which are exact duplicates. Do you think the duplicate detection could be enhanced by taking filenames and function hashes into account?

Ah, I see your update to the unit test. Yes, I think a version of loadFunctionRecord which satisfies this test would be great.

[Dor1s]

In D46478#1090568, @vsk wrote:

In D46478#1090502, @Dor1s wrote:

LLVM-Unit :: ProfileData/./ProfileDataTests/ParameterizedCovMapTest/CoverageMappingTest.skip_duplicate_function_record/0

This test was added in r284251 along with the initial support for loading coverage data from multiple object files. The basic idea behind seems reasonable, as it makes sense to skip records which are exact duplicates. Do you think the duplicate detection could be enhanced by taking filenames and function hashes into account?

Ah, I see your update to the unit test. Yes, I think a version of loadFunctionRecord which satisfies this test would be great.

Thanks for taking a look. I hope so, but can't figure out a proper way to implement that. Continue looking into that.

[vsk]

In D46478#1090585, @Dor1s wrote:

In D46478#1090568, @vsk wrote:

In D46478#1090502, @Dor1s wrote:

LLVM-Unit :: ProfileData/./ProfileDataTests/ParameterizedCovMapTest/CoverageMappingTest.skip_duplicate_function_record/0

This test was added in r284251 along with the initial support for loading coverage data from multiple object files. The basic idea behind seems reasonable, as it makes sense to skip records which are exact duplicates. Do you think the duplicate detection could be enhanced by taking filenames and function hashes into account?

Ah, I see your update to the unit test. Yes, I think a version of loadFunctionRecord which satisfies this test would be great.

Thanks for taking a look. I hope so, but can't figure out a proper way to implement that. Continue looking into that.

Maybe you could change the FunctionNames StringSet to some kind of filename-based map.

[Dor1s]

Added filenames into consideration whether a record should be loaded or skipped

[Dor1s]

In D46478#1090589, @vsk wrote:

...
Maybe you could change the FunctionNames StringSet to some kind of filename-based map.

Thanks Vedant, that's a great idea! I haven't realized that FunctionNames isn't used for anything else.

I've updated the change + sophisticated tests a little bit, everything seems to work fine:

$ ninja check-llvm check-clang check-lld check-polly
[139/151] Running polly regression tests
Testing Time: 7.20s
  Expected Passes    : 1018
  Expected Failures  : 24
  Unsupported Tests  : 95
[140/151] Running lld test suite
Testing Time: 2.61s
  Expected Passes    : 1309
  Unsupported Tests  : 330
[143/151] Running the LLVM regression tests
Testing Time: 27.77s
  Expected Passes    : 14856
  Expected Failures  : 49
  Unsupported Tests  : 10501
[150/151] Running the Clang regression tests
llvm-lit: ~/Projects/llvm/llvm/utils/lit/lit/llvm/config.py:334: note: using clang: ~/Projects/llvm/build/bin/clang
Testing Time: 33.98s
  Expected Passes    : 11999
  Expected Failures  : 18
  Unsupported Tests  : 283

[vsk]

lib/ProfileData/Coverage/CoverageMapping.cpp

223

Given that this should just be an optimization, it might be cheaper/simpler to use a map from the hash of the record's filenames to a set of stripped function names. This would preserve the proposed behavior where a record which introduces a duplicate symbol in at least one previously-unseen file is still loaded. Wdyt? I imagine something like AlreadyLoaded = !RecordProvenance[hash_combine_range(Record.Filenames)].insert(OrigFuncName).second. Hashing OrigFuncName might decrease the storage requirements even further.

[Dor1s]

Use hash-based DenseMap for filenames and hash-based DenseSet for function names

[Dor1s]

lib/ProfileData/Coverage/CoverageMapping.cpp

223

Thanks for the hints, Vedant! That sounds better to me. Please take a look at the updated CL, what do you think about it?

unittests/ProfileData/CoverageMappingTest.cpp

873

I've been confused by this behavior at first, but then realized that it's probably right, as this record must be something different than the one previously loaded at line 866

[vsk]

LGTM, thanks!

include/llvm/ProfileData/Coverage/CoverageMapping.h

36 ↗

(On Diff #145741)

Is <map> needed?

[Dor1s]

remove unnecessary header include

include/llvm/ProfileData/Coverage/CoverageMapping.h

36 ↗

(On Diff #145741)

Oops, leftover! Removed. Thanks!

[Dor1s]

remove the header from the proper file

lib/ProfileData/Coverage/CoverageMapping.cpp

Error CoverageMapping::loadFunctionRecord(
   if (OrigFuncName.empty())
     return make_error<CoverageMapError>(coveragemap_error::malformed);
 
   if (Record.Filenames.empty())
     OrigFuncName = getFuncNameWithoutPrefix(OrigFuncName);
   else
     OrigFuncName = getFuncNameWithoutPrefix(OrigFuncName, Record.Filenames[0]);
 
-  // Don't load records for functions we've already seen.
-  if (!FunctionNames.insert(OrigFuncName).second)
-    return Error::success();
-
   CounterMappingContext Ctx(Record.Expressions);
 
   std::vector<uint64_t> Counts;
   if (Error E = ProfileReader.getFunctionCounts(Record.FunctionName,
                                                 Record.FunctionHash, Counts)) {
     instrprof_error IPE = InstrProfError::take(std::move(E));
     if (IPE == instrprof_error::hash_mismatch) {
       FuncHashMismatches.emplace_back(Record.FunctionName, Record.FunctionHash);
       return Error::success();
     } else if (IPE != instrprof_error::unknown_function)
       return make_error<InstrProfError>(IPE);
     Counts.assign(Record.MappingRegions.size(), 0);
   }
   Ctx.setCounts(Counts);
 
   assert(!Record.MappingRegions.empty() && "Function has no regions");
 
   FunctionRecord Function(OrigFuncName, Record.Filenames);
   for (const auto &Region : Record.MappingRegions) {
     Expected<int64_t> ExecutionCount = Ctx.evaluate(Region.Count);
     if (auto E = ExecutionCount.takeError()) {
       consumeError(std::move(E));

test/tools/llvm-cov/multiple-objects.test

 RUN: llvm-cov report -instr-profile %S/Inputs/multiple_objects/merged.profdata \
 RUN:   %S/Inputs/multiple_objects/use_2.covmapping \
 RUN:   -object %S/Inputs/multiple_objects/use_1.covmapping | FileCheck -check-prefix=REPORT %s
 
 REPORT: Filename{{ +}}Regions{{ +}}Missed Regions{{ +}}Cover
 REPORT-NEXT: ---
 REPORT-NEXT: header.h{{ +}}25{{ +}}14{{ +}}44.00%
 
+# Make sure that both use_1.cc and use_2.cc have coverage reported.
+# Before https://reviews.llvm.org/D46478, only one of them used to be reported.
+REPORT-NEXT: use_1.cc{{ +}}1{{ +}}0{{ +}}100.00%
+REPORT-NEXT: use_2.cc{{ +}}2{{ +}}0{{ +}}100.00%
+
 Instructions for regenerating the test:
 
 clang -std=c++11 -mllvm -enable-name-compression=false -fprofile-instr-generate -fcoverage-mapping use_1.cc -o use_1
 clang -std=c++11 -mllvm -enable-name-compression=false -fprofile-instr-generate -fcoverage-mapping use_2.cc -o use_2
 LLVM_PROFILE_FILE="use_1.raw" ./use_1
 LLVM_PROFILE_FILE="use_2.raw" ./use_2
 llvm-profdata merge use_{1,2}.raw -o merged.profdata
 llvm-cov convert-for-testing ./use_1 -o ./use_1.covmapping
 llvm-cov convert-for-testing ./use_2 -o ./use_2.covmapping

unittests/ProfileData/CoverageMappingTest.cpp

TEST_P(CoverageMappingTest, skip_duplicate_function_record) {
   ProfileWriter.addRecord({"func", 0x1234, {1}}, Err);
 
   startFunction("func", 0x1234);
   addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9);
 
   startFunction("func", 0x1234);
   addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9);
 
+  startFunction("func", 0x1234);
+  addCMR(Counter::getCounter(0), "file2", 1, 1, 9, 9);
+
+  startFunction("func", 0x1234);
+  addCMR(Counter::getCounter(0), "file3", 1, 1, 9, 9);
+
   EXPECT_THAT_ERROR(loadCoverageMapping(), Succeeded());
 
   auto Funcs = LoadedCoverage->getCoveredFunctions();
   unsigned NumFuncs = std::distance(Funcs.begin(), Funcs.end());
-  ASSERT_EQ(1U, NumFuncs);
+  ASSERT_EQ(3U, NumFuncs);
 }
 
 // FIXME: Use ::testing::Combine() when llvm updates its copy of googletest.
 INSTANTIATE_TEST_CASE_P(ParameterizedCovMapTest, CoverageMappingTest,
                         ::testing::Values(std::pair<bool, bool>({false, false}),
                                           std::pair<bool, bool>({false, true}),
                                           std::pair<bool, bool>({true, false}),
                                           std::pair<bool, bool>({true, true})),);
 
 } // end anonymous namespace