[Coverage] : introduce class hierarchy (funcRecordReader) to support multiple versions of coverage data

[davidxl]

Summary

With the planned size reduction change, the coverage format version is expected to be bumped up. This patch adds necessary support such that backward compatibility can be kept with maximal code sharing. Reading different versions of coverage data just requires instantiating the reader according to the version.

No functional change is intended.

[vsk]

Thanks! Comments inline --

lib/ProfileData/CoverageMappingReader.cpp

309

This is a nice way to reduce the amount of template parameterization needed to pass around a reader object. However, since this class only used in this c++ file, maybe it would be better to get rid of this parent class and drop the 'Versioned' from 'VersionedCovMapFuncRecordReader'. Later, you can do auto Reader = make_unique(...).

431

I'm not comfortable with *&Buf as an input-output parameter. I think readFunctionRecords should have direct access to the buffer, without needing it passed in. Could you pass StringRef Data into the Reader constructor?

433

This should be lifted out of the loop. When the version bump happens, the line if (Version > coverage::CoverageMappingCurrentVersion) will start causing strange crashes :).

[davidxl]

lib/ProfileData/CoverageMappingReader.cpp

309

Auto does not work in this case -- because version value is read at reader runtime and we do not know what version's reader to instantiate at compile time. The parent class is introduced to hide the details.

431

In the coverage format design, there is no whole program level header, but instead the coverage map for the program is a concatination of module level coverage maps -- each one of the module level coverage data has a header. (On the other hand, the first module's cov map header can be considered the whole program's covmap header).

The CovMapFuncRecordReader is actually FuncRecord reader for a module. If we pass Data to the reader, we still need to keep track of current Data offset which is basically the same as 'Buf' here. Before the call to readFunctionRecords, 'Buf' points to the start of a module's coverage data.

433

Good idea -- since we don't expect Coverage Version to be different across different modules, lift it outside the loop can simplify the code.

Also note that -- the format of the covmap header format is pretty much 'locked' -- at least the location of the 'version' field -- no matter what version is in the future, it should not change.

[davidxl]

Address vsk's comment:

Hoist header read out of loop and add assert version is consistent across different modules.

[vsk]

Thanks for the explanation, this lgtm.

lib/ProfileData/CoverageMappingReader.cpp

309–342

Got it.

431

I see, I misread this as a parser for a single module's worth of coverage mapping information.

[vsk]

r257708

include/llvm/ProfileData/CoverageMapping.h

 LLVM_PACKED_END
 
 enum CoverageMappingVersion {
   CoverageMappingVersion1 = 0,
   // The current versin is Version1
   CoverageMappingCurrentVersion = INSTR_PROF_COVMAP_VERSION
 };
 
+template <int CovMapVersion, class IntPtrT> struct CovMapTraits {
+  typedef CovMapFunctionRecord<IntPtrT> CovMapFuncRecordType;
+  typedef IntPtrT NameRefType;
+};
+
 } // end namespace coverage
 
 /// \brief Provide DenseMapInfo for CounterExpression
 template<> struct DenseMapInfo<coverage::CounterExpression> {
   static inline coverage::CounterExpression getEmptyKey() {
     using namespace coverage;
     return CounterExpression(CounterExpression::ExprKind::Subtract,
                              Counter::getCounter(~0U),

lib/ProfileData/CoverageMappingReader.cpp

StringRef InstrProfSymtab::getFuncName(uint64_t Pointer, size_t Size) {
   if (Pointer < Address)
     return StringRef();
   auto Offset = Pointer - Address;
   if (Offset + Size > Data.size())
     return StringRef();
   return Data.substr(Pointer - Address, Size);
 }
 
-template <typename T, support::endianness Endian>
-static std::error_code readCoverageMappingData(
-    InstrProfSymtab &ProfileNames, StringRef Data,
-    std::vector<BinaryCoverageReader::ProfileMappingRecord> &Records,
-    std::vector<StringRef> &Filenames) {
-  using namespace support;
-  llvm::DenseSet<T> UniqueFunctionMappingData;
+struct CovMapFuncRecordReader {
+  virtual std::error_code readFunctionRecords(const char *&Buf,
+                                              const char *End) = 0;
+  template <class IntPtrT, support::endianness Endian>
+  static std::unique_ptr<CovMapFuncRecordReader>
+  get(coverage::CoverageMappingVersion Version, InstrProfSymtab &P,
+      std::vector<BinaryCoverageReader::ProfileMappingRecord> &R,
+      std::vector<StringRef> &F);
+};
+
+template <coverage::CoverageMappingVersion CovMapVersion, class IntPtrT,
+          support::endianness Endian>
+class VersionedCovMapFuncRecordReader : public CovMapFuncRecordReader {
+  typedef typename coverage::CovMapTraits<
+      CovMapVersion, IntPtrT>::CovMapFuncRecordType FuncRecordType;
+  typedef typename coverage::CovMapTraits<CovMapVersion, IntPtrT>::NameRefType
+      NameRefType;
+
+  llvm::DenseSet<NameRefType> UniqueFunctionMappingData;
+  InstrProfSymtab &ProfileNames;
+  std::vector<StringRef> &Filenames;
+  std::vector<BinaryCoverageReader::ProfileMappingRecord> &Records;
+
+public:
+  VersionedCovMapFuncRecordReader(
+      InstrProfSymtab &P,
+      std::vector<BinaryCoverageReader::ProfileMappingRecord> &R,
+      std::vector<StringRef> &F)
+      : ProfileNames(P), Filenames(F), Records(R) {}
 
-  // Read the records in the coverage data section.
-  for (const char *Buf = Data.data(), *End = Buf + Data.size(); Buf < End;) {
+  std::error_code readFunctionRecords(const char *&Buf,
+                                      const char *End) override {
+    using namespace support;
     if (Buf + sizeof(CovMapHeader) > End)
       return coveragemap_error::malformed;
     auto CovHeader = reinterpret_cast<const coverage::CovMapHeader *>(Buf);
     uint32_t NRecords = CovHeader->getNRecords<Endian>();
     uint32_t FilenamesSize = CovHeader->getFilenamesSize<Endian>();
     uint32_t CoverageSize = CovHeader->getCoverageSize<Endian>();
-    uint32_t Version = CovHeader->getVersion<Endian>();
-    Buf = reinterpret_cast<const char *>(++CovHeader);
-
-    if (Version > coverage::CoverageMappingCurrentVersion)
-      return coveragemap_error::unsupported_version;
+    Buf = reinterpret_cast<const char *>(CovHeader + 1);
 
     // Skip past the function records, saving the start and end for later.
     const char *FunBuf = Buf;
-    Buf += NRecords * sizeof(coverage::CovMapFunctionRecord<T>);
+    Buf += NRecords * sizeof(FuncRecordType);
     const char *FunEnd = Buf;
 
     // Get the filenames.
     if (Buf + FilenamesSize > End)
       return coveragemap_error::malformed;
     size_t FilenamesBegin = Filenames.size();
     RawCoverageFilenamesReader Reader(StringRef(Buf, FilenamesSize), Filenames);
     if (auto Err = Reader.read())
       return Err;
     Buf += FilenamesSize;
 
     // We'll read the coverage mapping records in the loop below.
     const char *CovBuf = Buf;
     Buf += CoverageSize;
     const char *CovEnd = Buf;
 
     if (Buf > End)
       return coveragemap_error::malformed;
     // Each coverage map has an alignment of 8, so we need to adjust alignment
     // before reading the next map.
     Buf += alignmentAdjustment(Buf, 8);
 
-    auto CFR =
-        reinterpret_cast<const coverage::CovMapFunctionRecord<T> *>(FunBuf);
+    auto CFR = reinterpret_cast<const FuncRecordType *>(FunBuf);
     while ((const char *)CFR < FunEnd) {
       // Read the function information
       uint32_t DataSize = CFR->template getDataSize<Endian>();
       uint64_t FuncHash = CFR->template getFuncHash<Endian>();
 
       // Now use that to read the coverage data.
       if (CovBuf + DataSize > CovEnd)
         return coveragemap_error::malformed;
       auto Mapping = StringRef(CovBuf, DataSize);
       CovBuf += DataSize;
 
       // Ignore this record if we already have a record that points to the same
       // function name. This is useful to ignore the redundant records for the
       // functions with ODR linkage.
-      T NameRef = CFR->template getFuncNameRef<Endian>();
+      NameRefType NameRef = CFR->template getFuncNameRef<Endian>();
       if (!UniqueFunctionMappingData.insert(NameRef).second)
         continue;
 
       StringRef FuncName;
       if (std::error_code EC =
               CFR->template getFuncName<Endian>(ProfileNames, FuncName))
         return EC;
       Records.push_back(BinaryCoverageReader::ProfileMappingRecord(
-          CoverageMappingVersion(Version), FuncName, FuncHash, Mapping,
-          FilenamesBegin, Filenames.size() - FilenamesBegin));
+          CovMapVersion, FuncName, FuncHash, Mapping, FilenamesBegin,
+          Filenames.size() - FilenamesBegin));
       CFR++;
     }
+    return std::error_code();
   }
+};
 
-  return std::error_code();
+template <class IntPtrT, support::endianness Endian>
+std::unique_ptr<CovMapFuncRecordReader> CovMapFuncRecordReader::get(
+    coverage::CoverageMappingVersion Version, InstrProfSymtab &P,
+    std::vector<BinaryCoverageReader::ProfileMappingRecord> &R,
+    std::vector<StringRef> &F) {
+  using namespace coverage;
+  switch (Version) {
+  case CoverageMappingVersion1:
+    return make_unique<VersionedCovMapFuncRecordReader<CoverageMappingVersion1,
+                                                       IntPtrT, Endian>>(P, R,
+                                                                         F);
+  default:
+    break;
+  }
+  llvm_unreachable("Unsupported version");
 }
 
+template <typename T, support::endianness Endian>
+static std::error_code readCoverageMappingData(
+    InstrProfSymtab &ProfileNames, StringRef Data,
+    std::vector<BinaryCoverageReader::ProfileMappingRecord> &Records,
+    std::vector<StringRef> &Filenames) {
+  using namespace coverage;
+  // Read the records in the coverage data section.
+  std::unique_ptr<CovMapFuncRecordReader> Reader(nullptr);
+  for (const char *Buf = Data.data(), *End = Buf + Data.size(); Buf < End;) {
+    auto CovHeader = reinterpret_cast<const coverage::CovMapHeader *>(Buf);
+    CoverageMappingVersion Version =
+        (CoverageMappingVersion)CovHeader->getVersion<Endian>();
+    if (Version > coverage::CoverageMappingCurrentVersion)
+      return coveragemap_error::unsupported_version;
+    if (!Reader)
+      Reader = CovMapFuncRecordReader::get<T, Endian>(Version, ProfileNames,
+                                                      Records, Filenames);
+    if (std::error_code EC = Reader->readFunctionRecords(Buf, End))
+      return EC;
+  }
+  return std::error_code();
+}
 static const char *TestingFormatMagic = "llvmcovmtestdata";
 
 static std::error_code loadTestingFormat(StringRef Data,
                                          InstrProfSymtab &ProfileNames,
                                          StringRef &CoverageMapping,
                                          uint8_t &BytesInAddress,
                                          support::endianness &Endian) {
   BytesInAddress = 8;