[PATCH] D156611: [llvm-cov] Fix a bug about using `convert-for-testing` on multi-source object files

[Petr Hosek via Phabricator via llvm-commits]

phosek added a comment.

This is a breaking change since it modifies the `.covmapping` format in a backwards incompatible way. For `.profdata` format we always try to avoid breaking changes and provide backwards compatibility, but I don't know if the same holds for the `.covmapping` format which is used only for testing. @davidxl do you have any thoughts on that?



================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:908-910
-    uint32_t FilenamesSize =
-        CovHeader->getFilenamesSize<support::endianness::little>();
-    uint32_t CoverageMappingSize = sizeof(CovMapHeader) + FilenamesSize;
----------------
It looks like in the current implementation, we're using `getFilenameSize` to compute the `CoverageMappingSize`, but `CovMapHeader` also contains `CoverageMappingSize`, accessible through `getCoverageSize()`. Shouldn't we be using that instead? Isn't that the issue with the current implementation?


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:921-922
-    CoverageRecords = MemoryBuffer::getMemBuffer(Data.substr(Pad));
-    if (CoverageRecords->getBufferSize() == 0)
-      return make_error<CoverageMapError>(coveragemap_error::truncated);
-  }
----------------
Can you preserve this check in the new implementation?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156611/new/

https://reviews.llvm.org/D156611