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

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/ProfileData/CoverageMappingReader.cpp:309
@@ -308,11 +308,3 @@
 
-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;
-
-  // Read the records in the coverage data section.
-  for (const char *Buf = Data.data(), *End = Buf + Data.size(); Buf < End;) {
+struct CovMapFuncRecordReader {
+  virtual std::error_code readFunctionRecords(const char *&Buf,
----------------
vsk wrote:
> 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(...)`.
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.

================
Comment at: lib/ProfileData/CoverageMappingReader.cpp:431
@@ +430,3 @@
+  // 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;) {
----------------
vsk wrote:
> 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?
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.

================
Comment at: lib/ProfileData/CoverageMappingReader.cpp:433
@@ +432,3 @@
+  for (const char *Buf = Data.data(), *End = Buf + Data.size(); Buf < End;) {
+    auto CovHeader = reinterpret_cast<const coverage::CovMapHeader *>(Buf);
+    CoverageMappingVersion Version =
----------------
vsk wrote:
> This should be lifted out of the loop. When the version bump happens, the line `if (Version > coverage::CoverageMappingCurrentVersion)` will start causing strange crashes :).
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.


http://reviews.llvm.org/D16133