[PATCH] D101780: [CoverageMapping] Handle gaps in counter IDs for source-based coverage

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

Thanks for the context. I haven't looked at the Rust FE's coverage implementation, so I'm not familiar with any tradeoffs inherent to the implementation approach. One thing you wrote does concern me:

> Rust FE adds counters before frontend DCE. The coverage mapping is written after DCE, so is skipped for any deleted regions.

While it isn't a hard requirement that the source -> coverage mapping become immutable prior to any optimization, that is the traditional approach taken by clang & swiftc. (We nod towards this in https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#impact-of-llvm-optimizations-on-coverage-reports, but you're right that the documentation could be improved.) IIUC Rust's behavior is a bit of a departure. Although it probably saves some binary size in *.prof{raw,data} files (I wonder how much?), it does also introduce additional complexity.

I see two options for moving forward:

1. Have llvm be more permissive about the number of counters when loading sparse .profdata (this patch)
2. Have llvm be less permissive (assert that #counters < #regions), document the new requirement and change the Rust FE to emit counters before DCE

I think (2) would lead to less complexity overall in both Rust & llvm, but both sound reasonable to me. Wdyt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101780