Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coverage: Add enums to accommodate other kinds of coverage mappings #119842

Merged
merged 3 commits into from Jan 11, 2024

Conversation

Zalathar
Copy link
Contributor

Extracted from #118305.

LLVM supports several different kinds of coverage mapping regions, but currently we only ever emit ordinary “code” regions. This PR performs the plumbing required to add other kinds of regions as enum variants, but does not add any specific variants other than Code.

The main motivation for this change is branch coverage, but it will also allow separate experimentation with gap regions and skipped regions, which might help in producing more accurate and useful coverage reports.


@rustbot label +A-code-coverage

This is less elegant in some ways, since we no longer visit a BCB's spans as a
batch, but will make it much easier to add support for other kinds of coverage
mapping regions (e.g. branch regions or gap regions).
@rustbot
Copy link
Collaborator

rustbot commented Jan 11, 2024

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

@rustbot
Copy link
Collaborator

rustbot commented Jan 11, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) labels Jan 11, 2024
@oli-obk oli-obk assigned oli-obk and unassigned petrochenkov Jan 11, 2024
Comment on lines +171 to +174
let one = |a| std::iter::once(a);
match *self {
Self::Code(term) => one(term),
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will work with variants that have multiple terms unless all of them are slices and you use https://doc.rust-lang.org/std/slice/fn.from_ref.html here (in which case you could proabably return a slice instead of an iterator)

But also not important, you can refactor to make it work when you add more variants

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the variants supported by LLVM have exactly zero, one, or two terms.

So my plan is to eventually change this definition to something like:

let one = |a| [Some(a), None].into_iter().flatten()

That way the individual arms that use one won't have to change.

(But I won't bother doing this until I actually add a variant that requires 0 or 2 terms.)

@oli-obk
Copy link
Contributor

oli-obk commented Jan 11, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 11, 2024

📌 Commit 124fff0 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 11, 2024
coverage: Add enums to accommodate other kinds of coverage mappings

Extracted from  rust-lang#118305.

LLVM supports several different kinds of coverage mapping regions, but currently we only ever emit ordinary “code” regions.  This PR performs the plumbing required to add other kinds of regions as enum variants, but does not add any specific variants other than `Code`.

The main motivation for this change is branch coverage, but it will also allow separate experimentation with gap regions and skipped regions, which might help in producing more accurate and useful coverage reports.

---

`@rustbot` label +A-code-coverage
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 11, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#119448 (annotate-snippets: update to 0.10)
 - rust-lang#119813 (Silence some follow-up errors [2/x])
 - rust-lang#119836 (chore: remove unnecessary blank line)
 - rust-lang#119841 (Remove `DiagnosticBuilder::buffer`)
 - rust-lang#119842 (coverage: Add enums to accommodate other kinds of coverage mappings)
 - rust-lang#119845 (rint: further doc tweaks)
 - rust-lang#119852 (give const-err4 a more descriptive name)
 - rust-lang#119853 (rustfmt.toml: don't ignore just any tests path, only root one)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8294356 into rust-lang:master Jan 11, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 11, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 11, 2024
Rollup merge of rust-lang#119842 - Zalathar:kind, r=oli-obk

coverage: Add enums to accommodate other kinds of coverage mappings

Extracted from  rust-lang#118305.

LLVM supports several different kinds of coverage mapping regions, but currently we only ever emit ordinary “code” regions.  This PR performs the plumbing required to add other kinds of regions as enum variants, but does not add any specific variants other than `Code`.

The main motivation for this change is branch coverage, but it will also allow separate experimentation with gap regions and skipped regions, which might help in producing more accurate and useful coverage reports.

---

``@rustbot`` label +A-code-coverage
@Zalathar Zalathar deleted the kind branch January 11, 2024 22:35
@Zalathar
Copy link
Contributor Author

After some quick experiments, I'm sad to report that LLVM's gap regions appear to be unusable.

LLVM documentation suggests that they work like code regions, except that code regions always take priority over gap regions when determining line counts.

In practice, that's not accurate. For example, replacing all of our code regions with gap regions ought to have no effect on the resulting coverage reports, but in fact it causes some (but not all) of our line counts to disappear, for reasons that I haven't fully determined. (For example, gap regions that span across multiple lines appear to be handled differently.)

I suspect that the llvm-cov reporting tool is making a lot of undocumented assumptions about how clang uses gap regions, and without understanding those assumptions we probably can't predict how it will handle gap regions that we try to emit.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 25, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#119448 (annotate-snippets: update to 0.10)
 - rust-lang#119813 (Silence some follow-up errors [2/x])
 - rust-lang#119836 (chore: remove unnecessary blank line)
 - rust-lang#119841 (Remove `DiagnosticBuilder::buffer`)
 - rust-lang#119842 (coverage: Add enums to accommodate other kinds of coverage mappings)
 - rust-lang#119845 (rint: further doc tweaks)
 - rust-lang#119852 (give const-err4 a more descriptive name)
 - rust-lang#119853 (rustfmt.toml: don't ignore just any tests path, only root one)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants