Update coverage docs #1122

richkadel · 2021-05-02T09:29:48Z

Before landing this PR, we should wait for:

Update coverage docs and command line help rust#84815 - because this new version of the dev guide document refers to a revised path to the Rust Unstable Book page on the compiler flag -Z instrument-coverage
Report coverage 0 of dead blocks rust#84797 - because this new version of the document refers to coverage tests in src/test/run-make, the new location as of that PR

jyn514 · 2021-05-02T13:49:24Z

src/llvm-coverage-instrumentation.md

-[Coverage Map]: https://llvm.org/docs/CoverageMappingFormat.html
-[unstable-book-sbcc]: https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/source-based-code-coverage.html
+[coverage map]: https://llvm.org/docs/CoverageMappingFormat.html
+[unstable-book-instrument-coverage]: https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/instrument-coverage.html


This link is a 404. https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/instrument-coverage.html

Why did you change it? The old link works fine.

We'll need to land this after landing rust-lang/rust#84815

I'll convert this PR to "draft" until then.

When I reviewed that documentation, I realized the page name was non-standard. (I'm not sure why we didn't catch it before.) The page is supposed to match the compiler flag name.

Some external links still point to the old name, so I just converted that old page to include a link to the new one. But for the documentation we control, we should use the correct link (once it's active).

https://github.com/rust-lang/rust/pull/84815/files#diff-766876fc4110b68305d45c7c034b471975796591292f859ae61a6a5d1caf361b

jyn514 · 2021-05-02T13:49:37Z

src/llvm-coverage-instrumentation.md

@@ -82,7 +83,7 @@ a span of code ([`CodeRegion`][code-region]). It counts the number of times a
 branch is executed, and also specifies the exact location of that code span in
 the Rust source code.

-Note that many of these `Coverage` statements will *not* be converted into
+Note that many of these `Coverage` statements will _not_ be converted into


Why change this?

Sorry. I didn't change this intentionally. My editor seems to have aggressive re-formatting options turned on, and I assumed these were from some Rust-specific settings (like how the editor picks up rust formatting from the committed rustfmt.toml).

I'll revert any unnecessary changes.

No worries, thanks for taking care of it :)

Just FYI, I reverted the unintended changes made by the formatter, but I also compared the style in this doc to the style in the rest of the Rustc Dev Guide.

This doc was not internally consistent (italics/emphasis used both _word_ and *word* with the same result, and bulleted items used both * and -. I chose the most common usage in the dev guide and make this document consistent (_ and -, respectively).

Oh, and I rewrapped lines that were too long, compared to a standard line length used in most of the document.

jyn514 · 2021-05-02T13:49:42Z

src/llvm-coverage-instrumentation.md

@@ -111,7 +112,7 @@ fn some_func(flag: bool) {
 In this example, four contiguous code regions are counted while only
 incrementing two counters.

-CFG analysis is used to not only determine *where* the branches are, for
+CFG analysis is used to not only determine _where_ the branches are, for


Why change this?

Not intended, as explained above. Sorry.

Leaving this one as-is, to be internally consistent (see my other comment explaining what I kept and why).

jyn514 · 2021-05-02T13:50:25Z

src/llvm-coverage-instrumentation.md

@@ -150,50 +151,53 @@ MIR `Statement` into some backend-specific action or instruction.
        match statement.kind {
            ...
            mir::StatementKind::Coverage(box ref coverage) => {
-                self.codegen_coverage(&mut bx, coverage.clone());
+                self.codegen_coverage(&mut bx, coverage.clone(), statement.source_info.scope);


Do you happen to have a link to the PR changing this? Has it already landed or is the PR still open?

Yes, this landed on March 17. @tmiasko added the ability to support inlining with coverage.

rust-lang/rust#83080

See: https://github.com/rust-lang/rust/pull/83080/files#diff-f10b4e4dbb6fc3d26ea86854c45386cf59c2771c75a923f694b4c174e03e0970R114-R115

src/llvm-coverage-instrumentation.md

tmandry · 2021-05-06T23:41:43Z

src/llvm-coverage-instrumentation.md

-the `InstrumentCoverage` MIR pass are saved in `mir_dump` [Spanview][spanview-debugging]
-files and compared to expected results in [`coverage-spanview`].
+More complete testing of end-to-end coverage instrumentation and reports are
+done in the `run-make-fulldeps` tests, with sample Rust programs (to be


This should change to run-make if that change lands

I changed it to run-make originally, and then we had the CI issues, so I've changed it back. I don't know how soon that will be resolved, so I think we should land this change now, and update this when it actually does move.

tmandry · 2021-05-06T23:43:25Z

Left a couple nits, otherwise LGTM

@bors delegate+

camelid · 2021-05-06T23:51:13Z

FYI, bors isn't enabled for rustc-dev-guide; use GitHub-native approvals instead :)

tmandry · 2021-05-07T00:45:56Z

Ah, thanks :)

richkadel · 2021-05-07T01:29:36Z

Thanks @tmandry !

I don't expect any changes, but leaving it as a WIP / Draft until the changes to Rust Unstable Book land, which should resolve the 404.

Until then, we don't want to merge this.

richkadel · 2021-05-07T01:39:41Z

(I will address the review comments though... just to be clear. Thanks!)

camelid · 2021-05-08T03:44:40Z

Before landing this PR, we should wait for:

rust-lang/rust#84815 - because this new version of the dev guide document refers to a revised path to the Rust Unstable Book page on the compiler flag -Z instrument-coverage

rust-lang/rust#84797 - because this new version of the document refers to coverage tests in src/test/run-make, the new location as of that PR

Looks like these two have been merged! @rustbot label: -blocked +waiting-on-review

richkadel · 2021-05-08T03:46:32Z

@camelid - Thanks. It has been reviewed and approved, and is ready to be merged (unless I misunderstand the label you added).

Update books ## reference 4 commits in 5aa457bf1b54bd2cd5d4cf49797f29299bdf89a7..9c68af3ce6ccca2395e1868addef26a0542e9ddd 2021-05-05 08:39:22 -0700 to 2021-05-24 09:53:32 -0700 - missing parameter name in Trait Implementations (rust-lang/reference#1030) - Add more content to impl-trait.md (rust-lang/reference#1017) - Document extended key-value attributes (rust-lang/reference#1029) - Document raw pointer <-> usize casts. (rust-lang/reference#970) ## rust-by-example 1 commits in 5f8c6da200ada77760a2fe1096938ef58151c9a6..805e016c5792ad2adabb66e348233067d5ea9f10 2021-04-29 08:08:01 -0300 to 2021-05-20 17:08:34 -0300 - Update structs.md (rust-lang/rust-by-example#1440) ## rustc-dev-guide 4 commits in 1e6c7fb..50de7f0 2021-05-10 13:38:24 +0900 to 2021-05-20 15:02:20 +0200 - update rustfmt references to reflect change from submod to subtree (rust-lang/rustc-dev-guide#1129) - Remove `--stage 1` argument from `doc` invocations (rust-lang/rustc-dev-guide#1125) - Update coverage docs (rust-lang/rustc-dev-guide#1122) - Document -Zunpretty=thir-tree (rust-lang/rustc-dev-guide#1128) ## edition-guide 1 commits in 1da3c411f17adb1ba5de1683bb6acee83362b54a..302a115e8f71876dfc884aebb0ca5ccb02b8a962 2021-02-16 16:46:40 -0800 to 2021-05-21 10:46:11 -0400 - Minimize the edition guide (rust-lang/edition-guide#232) ## embedded-book 1 commits in 569c3391f5c0cc43433bc77831d17f8ff4d76602..7349d173fa28a0bb834cf0264a05286620ef0923 2021-04-07 08:32:11 +0000 to 2021-05-25 13:59:05 +0000 - Remove $ from cargo-binutils (rust-embedded/book#292)

richkadel force-pushed the coverage-docs-update-2021-05 branch from b94950d to e1bb3ea Compare May 2, 2021 09:35

jyn514 reviewed May 2, 2021

View reviewed changes

richkadel marked this pull request as draft May 2, 2021 17:42

richkadel mentioned this pull request May 2, 2021

Update coverage docs and command line help rust-lang/rust#84815

Merged

jyn514 added the S-blocked Status: this PR is blocked waiting for something label May 2, 2021

richkadel force-pushed the coverage-docs-update-2021-05 branch 3 times, most recently from 59bdfe0 to 9c9553f Compare May 6, 2021 18:25

tmandry reviewed May 6, 2021

View reviewed changes

src/llvm-coverage-instrumentation.md Outdated Show resolved Hide resolved

tmandry reviewed May 6, 2021

View reviewed changes

tmandry approved these changes May 7, 2021

View reviewed changes

Update coverage docs

8c065b2

richkadel force-pushed the coverage-docs-update-2021-05 branch from 9c9553f to 8c065b2 Compare May 7, 2021 16:01

richkadel marked this pull request as ready for review May 7, 2021 16:04

rustbot added S-waiting-on-review Status: this PR is waiting for a reviewer to verify its content and removed S-blocked Status: this PR is blocked waiting for something labels May 8, 2021

tmandry merged commit c989a4d into rust-lang:master May 14, 2021

ehuss mentioned this pull request May 25, 2021

Update books rust-lang/rust#85699

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update coverage docs #1122

Update coverage docs #1122

richkadel commented May 2, 2021 •

edited

Loading

jyn514 May 2, 2021

richkadel May 2, 2021

jyn514 May 2, 2021

richkadel May 2, 2021

jyn514 May 2, 2021

richkadel May 2, 2021

richkadel May 2, 2021

jyn514 May 2, 2021

richkadel May 2, 2021

richkadel May 2, 2021

jyn514 May 2, 2021

richkadel May 2, 2021

tmandry May 6, 2021

richkadel May 7, 2021

tmandry commented May 6, 2021

camelid commented May 6, 2021

tmandry commented May 7, 2021

richkadel commented May 7, 2021

richkadel commented May 7, 2021

camelid commented May 8, 2021 •

edited

Loading

richkadel commented May 8, 2021

Update coverage docs #1122

Update coverage docs #1122

Conversation

richkadel commented May 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmandry commented May 6, 2021

camelid commented May 6, 2021

tmandry commented May 7, 2021

richkadel commented May 7, 2021

richkadel commented May 7, 2021

camelid commented May 8, 2021 • edited Loading

richkadel commented May 8, 2021

richkadel commented May 2, 2021 •

edited

Loading

camelid commented May 8, 2021 •

edited

Loading