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

Sat Aug 5 19:10:05 PDT 2023

AtomicGu marked an inline comment as done.
AtomicGu added inline comments.

================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:908-910
-    uint32_t FilenamesSize =
-        CovHeader->getFilenamesSize<support::endianness::little>();
-    uint32_t CoverageMappingSize = sizeof(CovMapHeader) + FilenamesSize;
----------------
phosek wrote:
> AtomicGu wrote:
> > phosek wrote:
> > > 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?
> > I have inspected this variable. The value is not right. It is always zero, as the documentation says:
> > 
> > > The length of the string in the third field of __llvm_coverage_mapping that contains any encoded coverage mapping data affixed to the coverage header. Always 0, but present for backwards compatibility.
> If it's not being used, could we reuse it rather than introducing a new field?
There is an old test `llvm-project/llvm/test/tools/llvm-cov/Inputs/combine_expansions.covmapping` which is `Version1`. In this case, this variable is used and the value is still not right. If we reuse this field, at least we will lose the compatibility for `Version1`. However I think we can add a version check and only support the multi-source covmapping after `Version4`. So... I think this may work!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156611