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

Add new -Z dump-mir-spanview option #76074

Merged
merged 1 commit into from
Sep 2, 2020

Conversation

richkadel
Copy link
Contributor

Similar to -Z dump-mir-graphviz, this adds the option to write
HTML+CSS files that allow users to analyze the spans associated with MIR
elements (by individual statement, just terminator, or overall basic
block).

This PR was split out from PR #76004, and exposes an API for spanview
HTML+CSS files that is also used to analyze code regions chosen for
coverage instrumentation (in a follow-on PR).

Rust compiler MCP rust-lang/compiler-team#278

Relevant issue: #34701 - Implement support for LLVMs code coverage
instrumentation

r? @tmandry
FYI @wesleywiser

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 29, 2020
@bors
Copy link
Contributor

bors commented Aug 30, 2020

☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

@tmandry tmandry left a comment

Choose a reason for hiding this comment

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

Mind leaving a note about this flag on this page of the rustc dev guide? (sources here)

I also liked the gif you made on #76004, is that still what this looks like?

compiler/rustc_mir/src/util/spanview.rs Outdated Show resolved Hide resolved
compiler/rustc_session/src/config.rs Outdated Show resolved Hide resolved
compiler/rustc_session/src/options.rs Outdated Show resolved Hide resolved
@tmandry
Copy link
Member

tmandry commented Sep 1, 2020

I checked this out and ran it locally. Some observations:

  • It's really useful to view alongside the MIR to see how each basic block maps back to the source!
  • I can't see the tooltip I saw in your screen capture, did you take that out?
  • Toggling between the expanded and unexpanded view on mouseover is a bit annoying since you might want to leave it expanded while scrolling the source. A click toggle might be nicer.
  • Some of the lines can get quite long when expanded and go off the edge; it might be nice to have an option that wraps lines, if it's easy to do. (I was viewing it in an HTML preview in vscode, so only had the width I'd have in a normal text editor.)

It might also be useful to have the "reverse" view one day (see the MIR with the original source in a tooltip). Of course, this view makes perfect sense for source code coverage.

@tmandry
Copy link
Member

tmandry commented Sep 1, 2020

Also, can you rebase and remove the merge commits? We try to keep merge commits out of PRs generally.

@richkadel
Copy link
Contributor Author

I can't see the tooltip I saw in your screen capture, did you take that out?

No, I definitely included the tooltip. It shows up for me. I don't think I have any special extension for viewing the HTML preview. One way I bring it up from the HTML file is I right-click it's tab in VSCode, and click the first menu option, "Open Preview".

The tooltip just works.

You can also try opening the file inside your Chrome browser.

As for the other comments, those are all good suggestions, but I tried things like this and they either involved adding more complexity that seemed unnecessary (for a first release), or required JavaScript (which I'm not against, but it adds more compatibility risk).

There are other things I'd like to improve as well, but none of them are show stoppers IMO.

Can you log these as a feature request?

@richkadel
Copy link
Contributor Author

Also, can you rebase and remove the merge commits? We try to keep merge commits out of PRs generally.

Ugh, yeah. I didn't noticed those leaked in when I had to rebase yesterday morning. Will fix.

@richkadel
Copy link
Contributor Author

Also, can you rebase and remove the merge commits? We try to keep merge commits out of PRs generally.

Ugh, yeah. I didn't noticed those leaked in when I had to rebase yesterday morning. Will fix.

done

@tmandry
Copy link
Member

tmandry commented Sep 1, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Sep 1, 2020

📌 Commit 5acd7eb25cda0efef746b7ca4a3a9f0b99a3c490 has been approved by tmandry

@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 1, 2020
Similar to `-Z dump-mir-graphviz`, this adds the option to write
HTML+CSS files that allow users to analyze the spans associated with MIR
elements (by individual statement, just terminator, or overall basic
block).

This PR was split out from PR rust-lang#76004, and exposes an API for spanview
HTML+CSS files that is also used to analyze code regions chosen for
coverage instrumentation (in a follow-on PR).

Rust compiler MCP rust-lang/compiler-team#278

Relevant issue: rust-lang#34701 - Implement support for LLVMs code coverage
instrumentation
@richkadel
Copy link
Contributor Author

@tmandry @wesleywiser - I had to rebase after the dependency landed.

It might require re-submitting to bors.

@wesleywiser
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 1, 2020

📌 Commit 6b5869a has been approved by wesleywiser

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 2, 2020
Rollup of 14 pull requests

Successful merges:

 - rust-lang#74880 (Add trailing comma support to matches macro)
 - rust-lang#76074 (Add new `-Z dump-mir-spanview` option)
 - rust-lang#76088 (Add more examples to lexicographic cmp on Iterators.)
 - rust-lang#76099 (Add info about `!` and `impl Trait`)
 - rust-lang#76126 (Use "Fira Sans" for crate list font)
 - rust-lang#76132 (Factor out StmtKind::MacCall fields into `MacCallStmt` struct)
 - rust-lang#76143 (Give a better error message for duplicate built-in macros)
 - rust-lang#76158 (Stabilise link-self-contained option)
 - rust-lang#76201 (Move to intra-doc links for library/core/src/panic.rs)
 - rust-lang#76206 (Make all methods of `std::net::Ipv6Addr` const)
 - rust-lang#76207 (# Move to intra-doc links for library/core/src/clone.rs)
 - rust-lang#76212 (Document lint missing_doc_code_examples is nightly-only)
 - rust-lang#76218 (lexer: Tiny improvement to shebang detection)
 - rust-lang#76221 (Clean up header in `iter` docs for `for` loops)

Failed merges:

r? @ghost
@bors bors merged commit 5f28831 into rust-lang:master Sep 2, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2020
… r=tmandry

Tools, tests, and experimenting with MIR-derived coverage counters

Leverages the new mir_dump output file in HTML+CSS (from rust-lang#76074) to visualize coverage code regions
and the MIR features that they came from (including overlapping spans).

See example below.

The `run-make-fulldeps/instrument-coverage` test has been refactored to maximize test coverage and reduce code duplication. The new tests support testing with and without `-Clink-dead-code`, so Rust coverage can be tested on MSVC (which, currently, only works with `link-dead-code` _disabled_).

New tests validate coverage region generation and coverage reports with multiple counters per function. Starting with a simple `if-else` branch tests, coverage tests for each additional syntax type can be added by simply dropping in a new Rust sample program.

Includes a basic, MIR-block-based implementation of coverage injection,
available via `-Zexperimental-coverage`. This implementation has known
flaws and omissions, but is simple enough to validate the new tools and
tests.

The existing `-Zinstrument-coverage` option currently enables
function-level coverage only, which at least appears to generate
accurate coverage reports at that level.

Experimental coverage is not accurate at this time. When branch coverage
works as intended, the `-Zexperimental-coverage` option should be
removed.

This PR replaces the bulk of PR rust-lang#75828, with the remaining parts of
that PR distributed among other separate and indentpent PRs.

This PR depends on two of those other PRs: rust-lang#76002, rust-lang#76003 and rust-lang#76074

Rust compiler MCP rust-lang/compiler-team#278

Relevant issue: rust-lang#34701 - Implement support for LLVMs code coverage
instrumentation

![Screen-Recording-2020-08-21-at-2](https://user-images.githubusercontent.com/3827298/90972923-ff417880-e4d1-11ea-92bb-8713c6198f6d.gif)

r? @tmandry
FYI: @wesleywiser
@cuviper cuviper added this to the 1.48.0 milestone Nov 17, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 5, 2024
…em,Nilstrieb

Remove `-Zdump-mir-spanview`

The `-Zdump-mir-spanview` flag was added back in rust-lang#76074, as a development/debugging aid for the initial work on what would eventually become `-Cinstrument-coverage`. It causes the compiler to emit an HTML file containing a function's source code, with various spans highlighted based on the contents of MIR.

When the suggestion was made to [triage and remove unnecessary `-Z` flags (Zulip)](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60-Z.60.20option.20triage), I noted that this flag could potentially be worth removing, but I wanted to keep it around to see whether I found it useful for my own coverage work.

But when I actually tried to use it, I ran into various issues (e.g. it crashes on `tests/coverage/closure.rs`). If I can't trust it to work properly without a full overhaul, then instead of diving down a rabbit hole of trying to fix arcane span-handling bugs, it seems better to just remove this obscure old code entirely.

---

`@rustbot` label +A-code-coverage
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 5, 2024
…em,Nilstrieb

Remove `-Zdump-mir-spanview`

The `-Zdump-mir-spanview` flag was added back in rust-lang#76074, as a development/debugging aid for the initial work on what would eventually become `-Cinstrument-coverage`. It causes the compiler to emit an HTML file containing a function's source code, with various spans highlighted based on the contents of MIR.

When the suggestion was made to [triage and remove unnecessary `-Z` flags (Zulip)](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60-Z.60.20option.20triage), I noted that this flag could potentially be worth removing, but I wanted to keep it around to see whether I found it useful for my own coverage work.

But when I actually tried to use it, I ran into various issues (e.g. it crashes on `tests/coverage/closure.rs`). If I can't trust it to work properly without a full overhaul, then instead of diving down a rabbit hole of trying to fix arcane span-handling bugs, it seems better to just remove this obscure old code entirely.

---

``@rustbot`` label +A-code-coverage
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 5, 2024
…em,Nilstrieb

Remove `-Zdump-mir-spanview`

The `-Zdump-mir-spanview` flag was added back in rust-lang#76074, as a development/debugging aid for the initial work on what would eventually become `-Cinstrument-coverage`. It causes the compiler to emit an HTML file containing a function's source code, with various spans highlighted based on the contents of MIR.

When the suggestion was made to [triage and remove unnecessary `-Z` flags (Zulip)](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60-Z.60.20option.20triage), I noted that this flag could potentially be worth removing, but I wanted to keep it around to see whether I found it useful for my own coverage work.

But when I actually tried to use it, I ran into various issues (e.g. it crashes on `tests/coverage/closure.rs`). If I can't trust it to work properly without a full overhaul, then instead of diving down a rabbit hole of trying to fix arcane span-handling bugs, it seems better to just remove this obscure old code entirely.

---

```@rustbot``` label +A-code-coverage
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jan 5, 2024
…em,Nilstrieb

Remove `-Zdump-mir-spanview`

The `-Zdump-mir-spanview` flag was added back in rust-lang#76074, as a development/debugging aid for the initial work on what would eventually become `-Cinstrument-coverage`. It causes the compiler to emit an HTML file containing a function's source code, with various spans highlighted based on the contents of MIR.

When the suggestion was made to [triage and remove unnecessary `-Z` flags (Zulip)](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60-Z.60.20option.20triage), I noted that this flag could potentially be worth removing, but I wanted to keep it around to see whether I found it useful for my own coverage work.

But when I actually tried to use it, I ran into various issues (e.g. it crashes on `tests/coverage/closure.rs`). If I can't trust it to work properly without a full overhaul, then instead of diving down a rabbit hole of trying to fix arcane span-handling bugs, it seems better to just remove this obscure old code entirely.

---

````@rustbot```` label +A-code-coverage
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 5, 2024
Rollup merge of rust-lang#119566 - Zalathar:remove-spanview, r=Swatinem,Nilstrieb

Remove `-Zdump-mir-spanview`

The `-Zdump-mir-spanview` flag was added back in rust-lang#76074, as a development/debugging aid for the initial work on what would eventually become `-Cinstrument-coverage`. It causes the compiler to emit an HTML file containing a function's source code, with various spans highlighted based on the contents of MIR.

When the suggestion was made to [triage and remove unnecessary `-Z` flags (Zulip)](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60-Z.60.20option.20triage), I noted that this flag could potentially be worth removing, but I wanted to keep it around to see whether I found it useful for my own coverage work.

But when I actually tried to use it, I ran into various issues (e.g. it crashes on `tests/coverage/closure.rs`). If I can't trust it to work properly without a full overhaul, then instead of diving down a rabbit hole of trying to fix arcane span-handling bugs, it seems better to just remove this obscure old code entirely.

---

````@rustbot```` label +A-code-coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants