[Coverage] Support loading multiple files into a CoverageMapping

[vsk]

Summary

Add support for loading multiple coverage readers into a CoverageMapping
instance. This should make it easier to prepare coverage reports for
multiple binaries.

This should make it possible to substantially simplify D25086.

[arphaman]

Thanks for working on this!

unittests/ProfileData/CoverageMappingTest.cpp

294

I noticed that you have a bunch CoverageMappingTest renaming changes in this patch. Do you think it would be better to do the rename in a separate commit before this patch? So get rid of MaybeSparseCoverageMappingTest and make CoverageMappingTest a bool parameter test, and then in the second commit make CoverageMappingTest a std::pair<bool, bool> test? You don't have to split this patch, just the commits.

[vsk]

unittests/ProfileData/CoverageMappingTest.cpp

294

Makes sense -- I went ahead and committed the renaming changes in r284135.

[vsk]

• Split out renaming changes, rebase to master.

[arphaman]

LGTM

include/llvm/ProfileData/Coverage/CoverageMapping.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_PROFILEDATA_COVERAGEMAPPING_H_
 #define LLVM_PROFILEDATA_COVERAGEMAPPING_H_
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Hashing.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/ADT/iterator.h"
 #include "llvm/ProfileData/InstrProf.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/raw_ostream.h"
 #include <system_error>
 #include <tuple>
public:
   ArrayRef<ExpansionRecord> getExpansions() const { return Expansions; }
 };
 
 /// \brief The mapping of profile information to coverage data.
 ///
 /// This is the main interface to get coverage information, using a profile to
 /// fill out execution counts.
 class CoverageMapping {
+  StringSet<> FunctionNames;
   std::vector<FunctionRecord> Functions;
   unsigned MismatchedFunctionCount;
 
   CoverageMapping() : MismatchedFunctionCount(0) {}
 
   CoverageMapping(const CoverageMapping &) = delete;
   const CoverageMapping &operator=(const CoverageMapping &) = delete;
 
   /// \brief Add a function record corresponding to \p Record.
   Error loadFunctionRecord(const CoverageMappingRecord &Record,
                            IndexedInstrProfReader &ProfileReader);
 
 public:
   /// \brief Load the coverage mapping using the given readers.
   static Expected<std::unique_ptr<CoverageMapping>>
   load(CoverageMappingReader &CoverageReader,
        IndexedInstrProfReader &ProfileReader);
 
+  static Expected<std::unique_ptr<CoverageMapping>>
+  load(ArrayRef<std::unique_ptr<CoverageMappingReader>> CoverageReaders,
+       IndexedInstrProfReader &ProfileReader);
+
   /// \brief Load the coverage mapping from the given files.
   static Expected<std::unique_ptr<CoverageMapping>>
   load(StringRef ObjectFilename, StringRef ProfileFilename,
+       StringRef Arch = StringRef()) {
+    return load(ArrayRef<StringRef>(ObjectFilename), ProfileFilename, Arch);
+  }
+
+  static Expected<std::unique_ptr<CoverageMapping>>
+  load(ArrayRef<StringRef> ObjectFilenames, StringRef ProfileFilename,
        StringRef Arch = StringRef());
 
   /// \brief The number of functions that couldn't have their profiles mapped.
   ///
   /// This is a count of functions whose profile is out of date or otherwise
   /// can't be associated with any coverage information.
   unsigned getMismatchedCount() { return MismatchedFunctionCount; }
 

lib/ProfileData/Coverage/CoverageMapping.cpp

while (Current != Records.end() && !Filename.empty() &&
     ++Current;
   if (Current == Records.end())
     *this = FunctionRecordIterator();
 }
 
 Error CoverageMapping::loadFunctionRecord(
     const CoverageMappingRecord &Record,
     IndexedInstrProfReader &ProfileReader) {
+  StringRef OrigFuncName = Record.FunctionName;
+  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) {
       MismatchedFunctionCount++;
       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");
 
-  StringRef OrigFuncName = Record.FunctionName;
-  if (Record.Filenames.empty())
-    OrigFuncName = getFuncNameWithoutPrefix(OrigFuncName);
-  else
-    OrigFuncName = getFuncNameWithoutPrefix(OrigFuncName, Record.Filenames[0]);
   FunctionRecord Function(OrigFuncName, Record.Filenames);
   for (const auto &Region : Record.MappingRegions) {
     Expected<int64_t> ExecutionCount = Ctx.evaluate(Region.Count);
     if (auto E = ExecutionCount.takeError()) {
       llvm::consumeError(std::move(E));
       return Error::success();
     }
     Function.pushRegion(Region, *ExecutionCount);
CoverageMapping::load(CoverageMappingReader &CoverageReader,
 
   for (const auto &Record : CoverageReader)
     if (Error E = Coverage->loadFunctionRecord(Record, ProfileReader))
       return std::move(E);
 
   return std::move(Coverage);
 }
 
-Expected<std::unique_ptr<CoverageMapping>>
-CoverageMapping::load(StringRef ObjectFilename, StringRef ProfileFilename,
-                      StringRef Arch) {
-  auto CounterMappingBuff = MemoryBuffer::getFileOrSTDIN(ObjectFilename);
-  if (std::error_code EC = CounterMappingBuff.getError())
-    return errorCodeToError(EC);
-  auto CoverageReaderOrErr =
-      BinaryCoverageReader::create(CounterMappingBuff.get(), Arch);
-  if (Error E = CoverageReaderOrErr.takeError())
+Expected<std::unique_ptr<CoverageMapping>> CoverageMapping::load(
+    ArrayRef<std::unique_ptr<CoverageMappingReader>> CoverageReaders,
+    IndexedInstrProfReader &ProfileReader) {
+  auto Coverage = std::unique_ptr<CoverageMapping>(new CoverageMapping());
+
+  for (const auto &CoverageReader : CoverageReaders)
+    for (const auto &Record : *CoverageReader)
+      if (Error E = Coverage->loadFunctionRecord(Record, ProfileReader))
         return std::move(E);
-  auto CoverageReader = std::move(CoverageReaderOrErr.get());
+
+  return std::move(Coverage);
+}
+
+Expected<std::unique_ptr<CoverageMapping>>
+CoverageMapping::load(ArrayRef<StringRef> ObjectFilenames,
+                      StringRef ProfileFilename, StringRef Arch) {
   auto ProfileReaderOrErr = IndexedInstrProfReader::create(ProfileFilename);
   if (Error E = ProfileReaderOrErr.takeError())
     return std::move(E);
   auto ProfileReader = std::move(ProfileReaderOrErr.get());
-  return load(*CoverageReader, *ProfileReader);
+
+  SmallVector<std::unique_ptr<CoverageMappingReader>, 4> Readers;
+  SmallVector<std::unique_ptr<MemoryBuffer>, 4> Buffers;
+  for (StringRef ObjectFilename : ObjectFilenames) {
+    auto CovMappingBufOrErr = MemoryBuffer::getFileOrSTDIN(ObjectFilename);
+    if (std::error_code EC = CovMappingBufOrErr.getError())
+      return errorCodeToError(EC);
+    auto CoverageReaderOrErr =
+        BinaryCoverageReader::create(CovMappingBufOrErr.get(), Arch);
+    if (Error E = CoverageReaderOrErr.takeError())
+      return std::move(E);
+    Readers.push_back(std::move(CoverageReaderOrErr.get()));
+    Buffers.push_back(std::move(CovMappingBufOrErr.get()));
+  }
+  return load(Readers, *ProfileReader);
 }
 
 namespace {
 /// \brief Distributes functions into instantiation sets.
 ///
 /// An instantiation set is a collection of functions that have the same source
 /// code, ie, template functions specializations.
 class FunctionInstantiationSetCollector {

unittests/ProfileData/CoverageMappingTest.cpp

 #include "llvm/ProfileData/Coverage/CoverageMappingReader.h"
 #include "llvm/ProfileData/Coverage/CoverageMappingWriter.h"
 #include "llvm/ProfileData/InstrProfReader.h"
 #include "llvm/ProfileData/InstrProfWriter.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
 
 #include <ostream>
+#include <utility>
 
 using namespace llvm;
 using namespace coverage;
 
 static ::testing::AssertionResult NoError(Error E) {
   if (!E)
     return ::testing::AssertionSuccess();
   return ::testing::AssertionFailure() << "error: " << toString(std::move(E))
InputFunctionCoverageData(InputFunctionCoverageData &&IFCD)
         Regions(std::move(IFCD.Regions)) {}
 
   InputFunctionCoverageData(const InputFunctionCoverageData &) = delete;
   InputFunctionCoverageData &
   operator=(const InputFunctionCoverageData &) = delete;
   InputFunctionCoverageData &operator=(InputFunctionCoverageData &&) = delete;
 };
 
-struct CoverageMappingTest : ::testing::TestWithParam<bool> {
+struct CoverageMappingTest : ::testing::TestWithParam<std::pair<bool, bool>> {
+  bool UseMultipleReaders;
   StringMap<unsigned> Files;
   std::vector<InputFunctionCoverageData> InputFunctions;
   std::vector<OutputFunctionCoverageData> OutputFunctions;
 
   InstrProfWriter ProfileWriter;
   std::unique_ptr<IndexedInstrProfReader> ProfileReader;
 
   std::unique_ptr<CoverageMapping> LoadedCoverage;
 
   void SetUp() override {
-    ProfileWriter.setOutputSparse(GetParam());
+    ProfileWriter.setOutputSparse(GetParam().first);
+    UseMultipleReaders = GetParam().second;
   }
 
   unsigned getGlobalFileIndex(StringRef Name) {
     auto R = Files.find(Name);
     if (R != Files.end())
       return R->second;
     unsigned Index = Files.size();
     Files.try_emplace(Name, Index);
struct CoverageMappingTest : ::testing::TestWithParam<std::pair<bool, bool>> {
 
   void readProfCounts() {
     auto Profile = ProfileWriter.writeBuffer();
     auto ReaderOrErr = IndexedInstrProfReader::create(std::move(Profile));
     ASSERT_TRUE(NoError(ReaderOrErr.takeError()));
     ProfileReader = std::move(ReaderOrErr.get());
   }
 
+  Expected<std::unique_ptr<CoverageMapping>> readOutputFunctions() {
+    if (!UseMultipleReaders) {
+      CoverageMappingReaderMock CovReader(OutputFunctions);
+      return CoverageMapping::load(CovReader, *ProfileReader);
+    }
+
+    std::vector<std::unique_ptr<CoverageMappingReader>> CoverageReaders;
+    for (const auto &OF : OutputFunctions) {
+      ArrayRef<OutputFunctionCoverageData> Funcs(OF);
+      CoverageReaders.push_back(make_unique<CoverageMappingReaderMock>(Funcs));
+    }
+    return CoverageMapping::load(CoverageReaders, *ProfileReader);
+  }
+
   void loadCoverageMapping(bool EmitFilenames = true) {
     readProfCounts();
     writeAndReadCoverageRegions(EmitFilenames);
-
-    CoverageMappingReaderMock CovReader(OutputFunctions);
-    auto CoverageOrErr = CoverageMapping::load(CovReader, *ProfileReader);
+    auto CoverageOrErr = readOutputFunctions();
     ASSERT_TRUE(NoError(CoverageOrErr.takeError()));
     LoadedCoverage = std::move(CoverageOrErr.get());
   }
 };
 
 TEST_P(CoverageMappingTest, basic_write_read) {
   startFunction("func", 0x1234);
   addCMR(Counter::getCounter(0), "foo", 1, 1, 1, 1);
TEST_P(CoverageMappingTest, correct_deserialize_for_more_than_two_files) {
   for (unsigned I = 0; I < N; ++I) {
     ASSERT_GT(N, Output.Regions[I].FileID);
     ASSERT_GT(N, Output.Regions[I].LineStart);
     EXPECT_EQ(FileNames[Output.Regions[I].LineStart],
               Output.Filenames[Output.Regions[I].FileID]);
   }
 }
 
 TEST_P(CoverageMappingTest, load_coverage_for_more_than_two_files) {
   InstrProfRecord Record("func", 0x1234, {0});
   NoError(ProfileWriter.addRecord(std::move(Record)));
 
   const char *FileNames[] = {"bar", "baz", "foo"};
   static const unsigned N = array_lengthof(FileNames);
 
   startFunction("func", 0x1234);
   for (unsigned I = 0; I < N; ++I)
TEST_P(CoverageMappingTest, load_coverage_for_expanded_file) {
 
   CoverageData Data = LoadedCoverage->getCoverageForFile("expanded");
   std::vector<CoverageSegment> Segments(Data.begin(), Data.end());
   ASSERT_EQ(2U, Segments.size());
   EXPECT_EQ(CoverageSegment(1, 1, 10, true), Segments[0]);
   EXPECT_EQ(CoverageSegment(1, 10, false), Segments[1]);
 }
 
+TEST_P(CoverageMappingTest, skip_duplicate_function_record) {
+  InstrProfRecord Record("func", 0x1234, {1});
+  NoError(ProfileWriter.addRecord(std::move(Record)));
+
+  startFunction("func", 0x1234);
+  addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9);
+
+  startFunction("func", 0x1234);
+  addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9);
+
+  loadCoverageMapping();
+
+  auto Funcs = LoadedCoverage->getCoveredFunctions();
+  unsigned NumFuncs = std::distance(Funcs.begin(), Funcs.end());
+  ASSERT_EQ(1U, NumFuncs);
+}
+
+// FIXME: Use ::testing::Combine() when llvm updates its copy of googletest.
 INSTANTIATE_TEST_CASE_P(ParameterizedCovMapTest, CoverageMappingTest,
-                        ::testing::Bool());
+                        ::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