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

[Gulfem Savrun Yeniceri via Phabricator via llvm-commits]

gulfem added inline comments.


================
Comment at: llvm/tools/llvm-cov/TestingSupport.cpp:132
+    // to support multi-source covmapping.
+    CovMapHeader NewCovHeader = *CovHeader;
+    NewCovHeader.CoverageSize =
----------------
I don't think this is the right way of fixing the issue. There are two sizes that I want to differentiate:
1) `CoverageMappingSize` which is per module in `CovMapHeader`
2) `TotalCoverageMappingSize`, which is the sum of `CoverageMappingSize` in all modules

`CoverageMappingData.size()` at line 135 gives you `TotalCoverageMappingSize`, and this code overwrites the first module's `CoverageMappingSize` with `TotalCoverageMappingSize`. This is the first incorrect thing here. Second incorrect thing is that even if we reuse the `CoverageMappingSize` field in `CovMapHeader`, this file is not the right place to write it. This code overwrites a value instead of writing the correct value in the first place. This is where `CoverageMappingSize` is written in the first place in `CodeGen`. 
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CoverageMappingGen.cpp#L1785

We need to encode `TotalCoverageMappingSize` to fix this issue, and that's why I suggest using the earlier version that you implemented which encodes `CoverageMappingData.size()`. I don't think using `CoverageMappingSize` per module would be helpful here because in order to calculate `TotalCoverageMappingSize`, you need to read `CoverageMappingSize` by reading every `CovMapHeader` here, which we probably do not want to do.
https://github.com/llvm/llvm-project/blob/main/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp#L910
 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156611