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

[Pirama Arumuga Nainar via Phabricator via llvm-commits]

pirama added a subscriber: richkadel.
pirama added a comment.

In D101780#2744912 <https://reviews.llvm.org/D101780#2744912>, @vsk wrote:

> Thanks for digging into this. Could you explain why it's necessary to use more coverage counters than there are regions? If there's no longer a reserved counter (counter #0), then are counters N >= Regions.size() somehow redundant (and if so, are they worth deduplicating in the Rust FE)?

I'm not very familiar with the Rust FE. Here's one example I see when we build this for Android:

> Counter in file 0 520:20 -> 520:21, #1
> Counter in file 0 520:24 -> 520:25, (#6 + ((((#5 + #4) + #3) + #2) + (#1 - (#6 + (#5 + (#4 + (#3 + #2)))))))

There are only two regions in this function which seems to be a generated function for a `map`.

  ok_or(
      LOG_LEVEL_NAMES
          .iter()
          .position(|&name| eq_ignore_ascii_case(name, level))
          .into_iter()
          .filter(|&idx| idx != 0)
          .map(|idx| Level::from_usize(idx).unwrap())
          .next(),

Line 520 corresponds to the `map` call.

Adding @richkadel in case he has more details.  IIUC, Rust FE adds counters before frontend DCE.  The coverage mapping is written after DCE, so is skipped for any deleted regions.  But the counter expressions may remain and refer to these deleted counters.  Also, this example is from an older version of Rust FE.  There may be new fixes in the Rust nightly builds but not sure if this pass ordering issue would still be present.

At a high level, we don't currently document this assumption or requirement for LLVM coverage and there's no error/warning when it is violated.  So other frontends can make similar errors as well.  Making `llmv-cov` accept this would prevent this assumption from tripping other FEs.  An alternate to this change would be detect violations and have LLVM issue a warning/error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101780