coverage bug fixes and optimization support #83307

richkadel · 2021-03-19T21:23:41Z

Adjusted LLVM codegen for code compiled with -Zinstrument-coverage to
address multiple, somewhat related issues.

Fixed a significant flaw in prior coverage solution: Every counter
generated a new counter variable, but there should have only been one
counter variable per function. This appears to have bloated .profraw
files significantly. (For a small program, it increased the size by
about 40%. I have not tested large programs, but there is anecdotal
evidence that profraw files were way too large. This is a good fix,
regardless, but hopefully it also addresses related issues.

Fixes: #82144

Invalid LLVM coverage data produced when compiled with -C opt-level=1

Existing tests now work up to at least opt-level=3. This required a
detailed analysis of the LLVM IR, comparisons with Clang C++ LLVM IR
when compiled with coverage, and a lot of trial and error with codegen
adjustments.

The biggest hurdle was figuring out how to continue to support coverage
results for unused functions and generics. Rust's coverage results have
three advantages over Clang's coverage results:

Rust's coverage map does not include any overlapping code regions,
making coverage counting unambiguous.
Rust generates coverage results (showing zero counts) for all unused
functions, including generics. (Clang does not generate coverage for
uninstantiated template functions.)
Rust's unused functions produce minimal stubbed functions in LLVM IR,
sufficient for including in the coverage results; while Clang must
generate the complete LLVM IR for each unused function, even though
it will never be called.

This PR removes the previous hack of attempting to inject coverage into
some other existing function instance, and generates dedicated instances
for each unused function. This change, and a few other adjustments
(similar to what is required for -C link-dead-code, but with lower
impact), makes it possible to support LLVM optimizations.

Fixes: #79651

Coverage report: "Unexecuted instantiation:..." for a generic function
from multiple crates

Fixed by removing the aforementioned hack. Some "Unexecuted
instantiation" notices are unavoidable, as explained in the
used_crate.rs test, but -Zinstrument-coverage has new options to
back off support for either unused generics, or all unused functions,
which avoids the notice, at the cost of less coverage of unused
functions.

Fixes: #82875

Invalid LLVM coverage data produced with crate brotli_decompressor

Fixed by disabling the LLVM function attribute that forces inlining, if
-Z instrument-coverage is enabled. This attribute is applied to
Rust functions with `#[inline(always)], and in some cases, the forced
inlining breaks coverage instrumentation and reports.

FYI: @wesleywiser

r? @tmandry

richkadel · 2021-03-19T21:28:19Z

Important bug fixes here.

I know I have to resolve conflicts after @tmiasko 's change, but I wanted to push the validated changes I made in parallel. I'll merge in the recent changes, and then we'll have support for both opt-level and mir-opt-level !!

I haven't yet tested this on Mac or Windows. Depending on how quickly this can be reviewed, it would be helpful to do a bors try, but there's a fair chance I'll have to debug some platform specific issue.

And lastly, I may need to update the documentation, via separate PR.

Adjusted LLVM codegen for code compiled with `-Zinstrument-coverage` to address multiple, somewhat related issues. Fixed a significant flaw in prior coverage solution: Every counter generated a new counter variable, but there should have only been one counter variable per function. This appears to have bloated .profraw files significantly. (For a small program, it increased the size by about 40%. I have not tested large programs, but there is anecdotal evidence that profraw files were way too large. This is a good fix, regardless, but hopefully it also addresses related issues. Fixes: rust-lang#82144 Invalid LLVM coverage data produced when compiled with -C opt-level=1 Existing tests now work up to at least `opt-level=3`. This required a detailed analysis of the LLVM IR, comparisons with Clang C++ LLVM IR when compiled with coverage, and a lot of trial and error with codegen adjustments. The biggest hurdle was figuring out how to continue to support coverage results for unused functions and generics. Rust's coverage results have three advantages over Clang's coverage results: 1. Rust's coverage map does not include any overlapping code regions, making coverage counting unambiguous. 2. Rust generates coverage results (showing zero counts) for all unused functions, including generics. (Clang does not generate coverage for uninstantiated template functions.) 3. Rust's unused functions produce minimal stubbed functions in LLVM IR, sufficient for including in the coverage results; while Clang must generate the complete LLVM IR for each unused function, even though it will never be called. This PR removes the previous hack of attempting to inject coverage into some other existing function instance, and generates dedicated instances for each unused function. This change, and a few other adjustments (similar to what is required for `-C link-dead-code`, but with lower impact), makes it possible to support LLVM optimizations. Fixes: rust-lang#79651 Coverage report: "Unexecuted instantiation:..." for a generic function from multiple crates Fixed by removing the aforementioned hack. Some "Unexecuted instantiation" notices are unavoidable, as explained in the `used_crate.rs` test, but `-Zinstrument-coverage` has new options to back off support for either unused generics, or all unused functions, which avoids the notice, at the cost of less coverage of unused functions. Fixes: rust-lang#82875 Invalid LLVM coverage data produced with crate brotli_decompressor Fixed by disabling the LLVM function attribute that forces inlining, if `-Z instrument-coverage` is enabled. This attribute is applied to Rust functions with `#[inline(always)], and in some cases, the forced inlining breaks coverage instrumentation and reports.

compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs

tmandry · 2021-03-20T00:38:51Z

Every counter generated a new counter variable, but there should have only been one counter variable per function.

That's surprising; nice find!

nagisa · 2021-03-20T00:43:15Z

@bors try

bors · 2021-03-20T00:43:25Z

⌛ Trying commit bcf7555 with merge 47994a37de9e400e74ae445fb4b7adcaaa11dbf4...

richkadel · 2021-03-20T01:12:14Z

(UPDATE: WeakAnyLinkage appears to resolve this problem.)

FYI, below are the linker errors.

This isn't totally surprising, based on my solution to injecting synthetic unused functions (so that they aren't removed by downstream optimizations). I'm not sure how to create a sufficient test for this, but I expect/hope to solve this by tweaking the linkage/visibility settings, in declare_unused_fn(), changing or possibly removing these lines:

https://github.com/richkadel/rust/blob/bcf755562a1e7c2ed4f6a292b3c82afc9b5b5fe6/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs#L200-L203:

    unsafe {
        llvm::LLVMRustSetLinkage(llfn, llvm::Linkage::ExternalLinkage);
        llvm::LLVMRustSetVisibility(llfn, llvm::Visibility::Hidden);
    }

This is just a snippet of the linker errors. There are 100s of these errors.

Note, this is only using -Zinstrument-coverage, without changing the opt-level.

$ cd json5format
$ RUSTC=$HOME/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc \
    RUSTFLAGS="-Zinstrument-coverage" \
    RUSTDOCFLAGS="-Zinstrument-coverage -Zunstable-options --persist-doctests target/debug/doctestbins" \
    LLVM_PROFILE_FILE="json5format-%m.profraw" \
    cargo test

/usr/bin/ld: /usr/local/google/home/richkadel/json5format/target/debug/deps/libproptest-dd692d5cdf3d7e8c.rlib(proptest-dd692d5cdf3d7e8c.proptest.byf9pn9c-cgu.14.rcgu.o): in function `_RINvYpNtNtNtCsamzieRK4Cy0_8proptest8strategy6traits8Strategy15prop_filter_mappppEBa_':
proptest.byf9pn9c-cgu.14:(.text._RINvYpNtNtNtCsamzieRK4Cy0_8proptest8strategy6traits8Strategy15prop_filter_mappppEBa_+0x0): multiple definition of `_RINvYpNtNtNtCsamzieRK4Cy0_8proptest8strategy6traits8Strategy15prop_filter_mappppEBa_'; /usr/local/google/home/richkadel/json5format/target/debug/deps/libproptest-dd692d5cdf3d7e8c.rlib(proptest-dd692d5cdf3d7e8c.proptest.byf9pn9c-cgu.11.rcgu.o):proptest.byf9pn9c-cgu.11:(.text._RINvYpNtNtNtCsamzieRK4Cy0_8proptest8strategy6traits8Strategy15prop_filter_mappppEBa_+0x0): first defined here
/usr/bin/ld: /usr/local/google/home/richkadel/json5format/target/debug/deps/libproptest-dd692d5cdf3d7e8c.rlib(proptest-dd692d5cdf3d7e8c.proptest.byf9pn9c-cgu.14.rcgu.o): in function `_RINvYpNtNtNtCsamzieRK4Cy0_8proptest8strategy6traits8Strategy18prop_ind_flat_map2ppEBa_':
proptest.byf9pn9c-cgu.14:(.text._RINvYpNtNtNtCsamzieRK4Cy0_8proptest8strategy6traits8Strategy18prop_ind_flat_map2ppEBa_+0x0): multiple definition of `_RINvYpNtNtNtCsamzieRK4Cy0_8proptest8strategy6traits8Strategy18prop_ind_flat_map2ppEBa_'; /usr/local/google/home/richkadel/json5format/target/debug/deps/libproptest-dd692d5cdf3d7e8c.rlib(proptest-dd692d5cdf3d7e8c.proptest.byf9pn9c-cgu.11.rcgu.o):proptest.byf9pn9c-cgu.11:(.text._RINvYpNtNtNtCsamzieRK4Cy0_8proptest8strategy6traits8Strategy18prop_ind_flat_map2ppEBa_+0x0): first defined here
/usr/bin/ld: /usr/local/google/home/richkadel/json5format/target/debug/deps/libproptest-dd692d5cdf3d7e8c.rlib(proptest-dd692d5cdf3d7e8c.proptest.byf9pn9c-cgu.14.rcgu.o): in function `_RNvXININtNtCsamzieRK4Cy0_8proptest8strategy6traits0pEINtB5_8NoShrinkpENtB5_8Strategy8new_treeB9_':
proptest.byf9pn9c-cgu.14:(.text._RNvXININtNtCsamzieRK4Cy0_8proptest8strategy6traits0pEINtB5_8NoShrinkpENtB5_8Strategy8new_treeB9_+0x0): multiple definition of `_RNvXININtNtCsamzieRK4Cy0_8proptest8strategy6traits0pEINtB5_8NoShrinkpENtB5_8Strategy8new_treeB9_'; /usr/local/google/home/richkadel/json5format/target/debug/deps/libproptest-dd692d5cdf3d7e8c.rlib(proptest-dd692d5cdf3d7e8c.proptest.byf9pn9c-cgu.11.rcgu.o):proptest.byf9pn9c-cgu.11:(.text._RNvXININtNtCsamzieRK4Cy0_8proptest8strategy6traits0pEINtB5_8NoShrinkpENtB5_8Strategy8new_treeB9_+0x0): first defined here

tmandry

Didn't get too far yet, but excited for this change :)

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

tmandry · 2021-03-20T01:23:21Z

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

+    // Add a new `FunctionCoverage` to the `function_coverage_map`, with unreachable code regions
+    // for each region in it's MIR.
+
+    // Convert the `HashSet` of `codegenned_def_ids` to a sortable vector, and sort them.
    let mut sorted_codegenned_def_ids: Vec<DefId> =
        codegenned_def_ids.iter().map(|def_id| *def_id).collect();
    sorted_codegenned_def_ids.sort_unstable();

    let mut first_covered_def_id_by_file: FxHashMap<Symbol, DefId> = FxHashMap::default();


Looking at this with fresh eyes (maybe we discussed this before).. This will only work if there is a covered def in each file containing an uncovered def.

I'm wondering if we can do it a different way by sorting the Symbol (which is just an index IIRC), partitioning by the number of codegen units, and using the index of the current codegen unit to determine which files to generate uncovered symbols for. In fact this could use the DefIds directly instead of Symbols.

Of course, that assumes there's a way to get an index of the current codegen unit and the number of codegen units.. not sure if there is or if we can add one.

Yeah, it's better than it used to be. Previously the file had to have at least one non-generic instance. Now it's fine to have only generics.

I thought about this a lot this week. Yes, I agree, I'd love to be able to do this differently, but I don't understand enough about the CGU structure to visualize a better solution.

Can we keep this good, but challenging (for me), suggestion for a separate PR?

FYI, as an experiment, I added a library crate called unused_crate.rs, and added one pub const, then referenced only the const in the uses_crate.rs test. If the unused_crate defines any public non-generic functions, at least one of them will be eagerly generated into an Instance, and that's enough to have coverage for all of them (even private or generic). But if the crate ONLY has private non-generic and/or public generics, nothing gets coverage. So that's the only case.

HOWEVER! Even when they do get coverage in the coverage map (because there is one public non-generic), llvm-cov show STILL doesn't generate a coverage report for that file for whatever reason.

Maybe that's a bug, or just the way it works, but it looks like even if we change the way mapgen.rs works, it will apparently have no net effect or improvement.

One clarification, if any function in the crate actually is used, then you should get coverage (including in reports). My previous comment is only talking about limitations with reporting unused coverage in a crate that has no used functions.

bors · 2021-03-20T01:33:39Z

☀️ Try build successful - checks-actions
Build commit: 47994a37de9e400e74ae445fb4b7adcaaa11dbf4 (47994a37de9e400e74ae445fb4b7adcaaa11dbf4)

tmandry · 2021-03-20T01:33:49Z

@richkadel I think you could declare the symbols as weak, since their definitions should all be the same. That'll get rid of the multiple definition errors.

richkadel · 2021-03-20T01:41:15Z

@richkadel I think you could declare the symbols as weak, since their definitions should all be the same. That'll get rid of the multiple definition errors.

I'll give that a try. Thanks!

The sample json5format tests produce coverage results again (and work with opt-level 3!)

richkadel · 2021-03-20T03:53:54Z

Good news:

The PR passes on my Windows and my MacOS environments (in addition to Linux), when I run the run-make-fulldeps/coverage tests locally.
Changing the linkage for the synthetic unused functions from External to WeakAnyLinkage appears to still work, and fixed the linker errors when running the full json5format test suite (including doctests).

I think this addresses all of the known issues in this PR, but @tmandry still plans to continue reviewing.

@wesleywiser - You may be interested in at least skimming these changes if you have time. Any feedback is appreciated. Thanks!

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

@wesleywiser

Per @wesleywiser's comment: rust-lang#83307 (comment)

compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs

compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs

compiler/rustc_middle/src/mir/mono.rs

compiler/rustc_typeck/src/collect.rs

richkadel · 2021-03-23T21:31:36Z

@tmandry - I made all of your suggested changes, and replied to your comment expressing concern about inlining across CGUs. I'm not sure that I can do much, or if it really is a problem, but the change I added was required (tests break without it).

If you have a test case that we can use to show this doesn't work, or other ideas, let me know. I haven't seen a problem from this yet though (including compiling with -Zinstrument-coverage on Fuchsia, and compiling json5format and its dependencies (e.g., regex), and the brotli_decompressor.

tmandry · 2021-03-23T22:50:19Z

compiler/rustc_typeck/src/collect.rs

@@ -2876,14 +2876,9 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs {
                    InlineAttr::None
                } else if list_contains_name(&items[..], sym::always) {
                    if tcx.sess.instrument_coverage() {


Looking at this again, I'd prefer if this fix isn't in typeck. From a compiler architecture standpoint, that means coverage flags can change language-level semantics of the program.

Looking at all the references to InlineAttr there are a handful of places in the backend where it gets used. Maybe a helper method would be good here. I'd also accept a FIXME to address this later.

Actually a FIXME might be preferred, since this is much simpler. I'm not 100% sure what the right answer is here.

I think I'll add the FIXME. I searched a variety of locations, but this one was the most central that I could find.

added #83429

tmandry · 2021-03-23T22:52:43Z

@bors delegate+ rollup=never

r=me with comments addressed, or happy to take another look if you like.

bors · 2021-03-23T22:52:45Z

✌️ @richkadel can now approve this pull request

richkadel · 2021-03-24T00:05:27Z

@bors r=tmandry rollup=never

bors · 2021-03-24T00:05:29Z

📌 Commit 0859cec has been approved by tmandry

Dylan-DPC-zz · 2021-03-24T00:30:07Z

@bors retry

bors · 2021-03-24T06:57:10Z

⌛ Testing commit 0859cec with merge 9ebafb70ed5d3f150e2026c71dbe07ab548b6e18...

rust-log-analyzer · 2021-03-24T09:03:23Z

The job dist-aarch64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

      Memory: 14 GB
      Boot ROM Version: VMW71.00V.13989454.B64.1906190538
      Apple ROM Info: [MS_VM_CERT/SHA1/27d66596a61c48dd3dc7216fd715126e33f59ae7]Welcome to the Virtual Machine
      SMC Version (system): 2.8f0
      Serial Number (system): VME5NhY2cDo0

hw.ncpu: 3
hw.byteorder: 1234
hw.memsize: 15032385536

bors · 2021-03-24T09:03:56Z

💔 Test failed - checks-actions

richkadel · 2021-03-24T14:48:22Z

warning: spurious network error (1 tries remaining): failed to get 200 response from `https://crates.io/api/v1/crates/utf8parse/0.1.1/download`, got 503
warning: spurious network error (1 tries remaining): failed to get 200 response from `https://crates.io/api/v1/crates/libz-sys/1.1.2/download`, got 503
warning: spurious network error (1 tries remaining): failed to get 200 response from `https://crates.io/api/v1/crates/foreign-types/0.3.2/download`, got 503
error: failed to download from `https://crates.io/api/v1/crates/home/0.5.3/download`

Caused by:
  failed to get 200 response from `https://crates.io/api/v1/crates/home/0.5.3/download`, got 503
command did not execute successfully: "/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage0/bin/cargo" "build" "--target" "aarch64-apple-darwin" "-Zdual-proc-macros" "-Zbinary-dep-depinfo" "-j" "3" "--release" "--locked" "--color" "always" "--manifest-path" "/Users/runner/work/rust/rust/src/tools/cargo/Cargo.toml" "--features" "rustc-workspace-hack/all-static" "--message-format" "json-render-diagnostics"
expected success, got: exit code: 101
failed to run: /Users/runner/work/rust/rust/build/bootstrap/debug/bootstrap dist --stage 2
Build completed unsuccessfully in 1:58:00
== clock drift check ==
  local time: Wed Mar 24 09:03:12 UTC 2021
  network time: Wed, 24 Mar 2021 07:05:38 GMT
== end clock drift check ==
Error: Process completed with exit code 1.

richkadel · 2021-03-24T14:48:33Z

@bors retry

wesleywiser · 2021-03-24T19:32:32Z

I finished looking through the rest of this and this is really great work @richkadel!

bors · 2021-03-25T05:07:42Z

⌛ Testing commit 0859cec with merge dbc37a9...

bors · 2021-03-25T07:48:46Z

☀️ Test successful - checks-actions
Approved by: tmandry
Pushing dbc37a9 to master...

rust-highfive assigned tmandry Mar 19, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 19, 2021

richkadel force-pushed the cov-unused-functions-1.1 branch from 20f0dad to a71c080 Compare March 19, 2021 23:05

This comment has been minimized.

Sign in to view

richkadel force-pushed the cov-unused-functions-1.1 branch from a71c080 to 2eab02a Compare March 19, 2021 23:18

This comment has been minimized.

Sign in to view

richkadel force-pushed the cov-unused-functions-1.1 branch from 2eab02a to 171a334 Compare March 19, 2021 23:29

richkadel force-pushed the cov-unused-functions-1.1 branch from 171a334 to bcf7555 Compare March 20, 2021 00:12

richkadel commented Mar 20, 2021

View reviewed changes

compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

tmandry reviewed Mar 20, 2021

View reviewed changes

gave unused_fn WeakAnyLinkage; moved create_pgo_func_name_var

5a484a1

The sample json5format tests produce coverage results again (and work with opt-level 3!)

wesleywiser reviewed Mar 23, 2021

View reviewed changes

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs Outdated Show resolved Hide resolved

Change def_id filter to use requires_monomorphization()

94a3454

Per @wesleywiser's comment: rust-lang#83307 (comment)

tmandry reviewed Mar 23, 2021

View reviewed changes

richkadel mentioned this pull request Mar 23, 2021

Consider moving instrument-coverage-specific behavior in rustc_typeck to backend code #83429

Closed

Changes from review comments

0859cec

richkadel force-pushed the cov-unused-functions-1.1 branch from de24b34 to 0859cec Compare March 24, 2021 00:02

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 Mar 24, 2021

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 24, 2021

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 Mar 24, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 25, 2021

bors merged commit dbc37a9 into rust-lang:master Mar 25, 2021

rustbot added this to the 1.53.0 milestone Mar 25, 2021

bors mentioned this pull request Mar 25, 2021

Maintain supported sanitizers as a target property #81866

Merged

richkadel mentioned this pull request Mar 27, 2021

Binaries generate malformed coverage profile when built with -Z instrument-coverage -C link-dead-code -O #79175

Closed

richkadel mentioned this pull request Dec 2, 2021

Unable to link regex crate with "-Z instrument-coverage" on x86_64-pc-windows-msvc #85461

Closed

coverage bug fixes and optimization support #83307

coverage bug fixes and optimization support #83307

Conversation

richkadel commented Mar 19, 2021

richkadel commented Mar 19, 2021

This comment has been minimized.

This comment has been minimized.

tmandry commented Mar 20, 2021

nagisa commented Mar 20, 2021

bors commented Mar 20, 2021

This comment has been minimized.

richkadel commented Mar 20, 2021 • edited Loading

tmandry left a comment

Choose a reason for hiding this comment

tmandry Mar 20, 2021

Choose a reason for hiding this comment

richkadel Mar 20, 2021

Choose a reason for hiding this comment

richkadel Mar 22, 2021

Choose a reason for hiding this comment

richkadel Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

bors commented Mar 20, 2021

tmandry commented Mar 20, 2021

richkadel commented Mar 20, 2021

richkadel commented Mar 20, 2021

richkadel commented Mar 23, 2021

tmandry Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

tmandry Mar 23, 2021

Choose a reason for hiding this comment

richkadel Mar 23, 2021

Choose a reason for hiding this comment

richkadel Mar 24, 2021

Choose a reason for hiding this comment

tmandry commented Mar 23, 2021

bors commented Mar 23, 2021

richkadel commented Mar 24, 2021

bors commented Mar 24, 2021

Dylan-DPC-zz commented Mar 24, 2021

bors commented Mar 24, 2021

rust-log-analyzer commented Mar 24, 2021

bors commented Mar 24, 2021

richkadel commented Mar 24, 2021

richkadel commented Mar 24, 2021

wesleywiser commented Mar 24, 2021

bors commented Mar 25, 2021

bors commented Mar 25, 2021

richkadel commented Mar 20, 2021 •

edited

Loading

richkadel Mar 22, 2021 •

edited

Loading

tmandry Mar 23, 2021 •

edited

Loading