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

[Yuhao Gu via Phabricator via llvm-commits]

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


================
Comment at: llvm/tools/llvm-cov/TestingSupport.cpp:132
+    // to support multi-source covmapping.
+    CovMapHeader NewCovHeader = *CovHeader;
+    NewCovHeader.CoverageSize =
----------------
gulfem wrote:
> 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
>  
Yes, that's what I thought at first. But @phosek wants the backward compatibility, and this is the only way. So do I need to revert to the previous version? Both version has passed the pre-merge checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156611