[PATCH] D156611: [llvm-cov] Fix a bug about using `convert-for-testing` on multi-source object files
Yuhao Gu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 9 17:43:26 PDT 2023
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
More information about the llvm-commits
mailing list