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: Remove debug code from the instrumentor #115962

Merged
merged 3 commits into from
Sep 20, 2023
Merged

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Sep 19, 2023

The coverage instrumentor has an entire module full of complex code that is only used for debugging.

And as I continue to work on coverage, I keep finding that this debug code is constantly causing more trouble than it's worth. It's deeply entangled with current implementation details, such that making any non-trivial change to the instrumentor usually requires major changes to the debug code. And so far I have personally not found any of this debug code to be useful.

In light of that situation, I'd like to try just ripping all of it out. If I spend any more time dealing with coverage debug code, I want it to be because I'm writing new and useful tools, not dutifully maintaining a boat-anchor that quite plausibly isn't being used by anyone at all.


r? @ghost
@rustbot label +A-code-coverage


Zulip thread

@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 Sep 19, 2023
@Zalathar
Copy link
Contributor Author

The main things being removed here are:

  • A parser for environment variable RUSTC_COVERAGE_DEBUG_OPTIONS that configures the rest of the debug code
  • DebugCounters, which stores a mapping from Operand values to BcbCounter values, and an optional label that indicates a BCB node or BCB edge
  • GraphvizData, which stores extra data needed by graphviz dumps
  • UsedExpressions, which tries to check that the instrumentor hasn't injected any unnecessary expression nodes
  • dump_coverage_spanview, which dumps spans extracted from MIR along with additional details about where those spans came from
  • dump_coverage_graphviz, which dumps a graphviz diagram illustrating the BCB graph along with associated node/edge counter data

@Zalathar
Copy link
Contributor Author

All of that sounds useful in the abstract, but it's all so heavily tied to the current instrumentor implementation that it is easily broken by any non-trivial change to the instrumentor, often in ways that are difficult or impossible to fix without major changes to the debug code itself.

@Zalathar
Copy link
Contributor Author

Ironically, much of the information being tracked by the debug code would be much easier to extract directly from the main data structures, if only we could make changes to those data structures that are being prevented by the existence of the debug code.

@Zalathar
Copy link
Contributor Author

This does not remove the -Zdump-coverage-graphviz and -Zdump-coverage-spanview flags, which should continue to work, but will no longer produce dumps containing extra coverage-specific information.

@Zalathar Zalathar marked this pull request as ready for review September 20, 2023 07:06
@rustbot
Copy link
Collaborator

rustbot commented Sep 20, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@oli-obk
Copy link
Contributor

oli-obk commented Sep 20, 2023

r? @oli-obk

@oli-obk
Copy link
Contributor

oli-obk commented Sep 20, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 20, 2023

📌 Commit e015f5a 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 Sep 20, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 20, 2023
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#115566 (clean up unneeded `ToPredicate` impls)
 - rust-lang#115962 (coverage: Remove debug code from the instrumentor)
 - rust-lang#115988 (rustdoc: add test cases, and fix, search tabs layout jank)
 - rust-lang#115991 (Ensure `build/tmp` exists in `rustdoc_themes::get_themes`)
 - rust-lang#115997 (RELEASES.md: Add missing patch releases)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c5c4e18 into rust-lang:master Sep 20, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 20, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 20, 2023
Rollup merge of rust-lang#115962 - Zalathar:debug, r=oli-obk

coverage: Remove debug code from the instrumentor

The coverage instrumentor has an entire module full of complex code that is only used for debugging.

And as I continue to work on coverage, I keep finding that this debug code is constantly causing more trouble than it's worth. It's deeply entangled with current implementation details, such that making any non-trivial change to the instrumentor usually requires major changes to the debug code. And so far I have personally not found any of this debug code to be *useful*.

In light of that situation, I'd like to try just ripping all of it out. If I spend any more time dealing with coverage debug code, I want it to be because I'm writing new and useful tools, not dutifully maintaining a boat-anchor that quite plausibly isn't being used by anyone at all.

---
r? `@ghost`
`@rustbot` label +A-code-coverage

---

[Zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Removing.20debug.20code.20from.20the.20coverage.20instrumentor)
@Zalathar Zalathar deleted the debug branch September 20, 2023 23:28
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 1, 2023
This enum was mainly needed to track the precise origin of a span in MIR, for
debug printing purposes. Since the old debug code was removed in rust-lang#115962, we
can replace it with just the span itself.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 2, 2023
This enum was mainly needed to track the precise origin of a span in MIR, for
debug printing purposes. Since the old debug code was removed in rust-lang#115962, we
can replace it with just the span itself.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 3, 2023
This enum was mainly needed to track the precise origin of a span in MIR, for
debug printing purposes. Since the old debug code was removed in rust-lang#115962, we
can replace it with just the span itself.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 4, 2023
This enum was mainly needed to track the precise origin of a span in MIR, for
debug printing purposes. Since the old debug code was removed in rust-lang#115962, we
can replace it with just the span itself.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 4, 2023
This enum was mainly needed to track the precise origin of a span in MIR, for
debug printing purposes. Since the old debug code was removed in rust-lang#115962, we
can replace it with just the span itself.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 5, 2023
This enum was mainly needed to track the precise origin of a span in MIR, for
debug printing purposes. Since the old debug code was removed in rust-lang#115962, we
can replace it with just the span itself.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 6, 2023
This enum was mainly needed to track the precise origin of a span in MIR, for
debug printing purposes. Since the old debug code was removed in rust-lang#115962, we
can replace it with just the span itself.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 6, 2023
This enum was mainly needed to track the precise origin of a span in MIR, for
debug printing purposes. Since the old debug code was removed in rust-lang#115962, we
can replace it with just the span itself.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 6, 2023
This enum was mainly needed to track the precise origin of a span in MIR, for
debug printing purposes. Since the old debug code was removed in rust-lang#115962, we
can replace it with just the span itself.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 8, 2023
This enum was mainly needed to track the precise origin of a span in MIR, for
debug printing purposes. Since the old debug code was removed in rust-lang#115962, we
can replace it with just the span itself.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 9, 2023
This enum was mainly needed to track the precise origin of a span in MIR, for
debug printing purposes. Since the old debug code was removed in rust-lang#115962, we
can replace it with just the span itself.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 9, 2023
This enum was mainly needed to track the precise origin of a span in MIR, for
debug printing purposes. Since the old debug code was removed in rust-lang#115962, we
can replace it with just the span itself.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 10, 2023
This enum was mainly needed to track the precise origin of a span in MIR, for
debug printing purposes. Since the old debug code was removed in rust-lang#115962, we
can replace it with just the span itself.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 10, 2023
This enum was mainly needed to track the precise origin of a span in MIR, for
debug printing purposes. Since the old debug code was removed in rust-lang#115962, we
can replace it with just the span itself.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 31, 2023
Historically, these errors existed so that the coverage debug code could dump
additional information before reporting a compiler bug. That debug code was
removed by rust-lang#115962, so we can now simplify these methods by making them panic
when they detect a bug.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 31, 2023
Historically, these errors existed so that the coverage debug code could dump
additional information before reporting a compiler bug. That debug code was
removed by rust-lang#115962, so we can now simplify these methods by making them panic
when they detect a bug.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 31, 2023
Historically, these errors existed so that the coverage debug code could dump
additional information before reporting a compiler bug. That debug code was
removed by rust-lang#115962, so we can now simplify these methods by making them panic
when they detect a bug.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 31, 2023
Historically, these errors existed so that the coverage debug code could dump
additional information before reporting a compiler bug. That debug code was
removed by rust-lang#115962, so we can now simplify these methods by making them panic
when they detect a bug.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 31, 2023
Historically, these errors existed so that the coverage debug code could dump
additional information before reporting a compiler bug. That debug code was
removed by rust-lang#115962, so we can now simplify these methods by making them panic
when they detect a bug.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 31, 2023
Historically, these errors existed so that the coverage debug code could dump
additional information before reporting a compiler bug. That debug code was
removed by rust-lang#115962, so we can now simplify these methods by making them panic
when they detect a bug.
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 31, 2023
Historically, these errors existed so that the coverage debug code could dump
additional information before reporting a compiler bug. That debug code was
removed by rust-lang#115962, so we can now simplify these methods by making them panic
when they detect a bug.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 31, 2023
coverage: Replace impossible `coverage::Error` with assertions

Historically, these errors existed so that the coverage debug code could dump additional information before reporting a compiler bug. That debug code was removed by rust-lang#115962, so we can now simplify these methods by making them panic immediately when they detect a bug.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 31, 2023
coverage: Replace impossible `coverage::Error` with assertions

Historically, these errors existed so that the coverage debug code could dump additional information before reporting a compiler bug. That debug code was removed by rust-lang#115962, so we can now simplify these methods by making them panic immediately when they detect a bug.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 31, 2023
Rollup merge of rust-lang#117421 - Zalathar:error, r=oli-obk,Swatinem

coverage: Replace impossible `coverage::Error` with assertions

Historically, these errors existed so that the coverage debug code could dump additional information before reporting a compiler bug. That debug code was removed by rust-lang#115962, so we can now simplify these methods by making them panic immediately when they detect a bug.
kjetilkjeka pushed a commit to kjetilkjeka/rust that referenced this pull request Oct 31, 2023
Historically, these errors existed so that the coverage debug code could dump
additional information before reporting a compiler bug. That debug code was
removed by rust-lang#115962, so we can now simplify these methods by making them panic
when they detect a bug.
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.

5 participants