Fixed coverage map issues; better aligned with LLVM APIs #74733

richkadel · 2020-07-25T05:43:20Z

Found some problems with the coverage map encoding when testing with more than one counter per function.

While debugging, I realized some better ways to structure the Rust implementation of the coverage mapping generator. I refactored somewhat, resulting in less code overall, expanded coverage of LLVM Coverage Map capabilities, and much closer alignment with LLVM data structures, APIs, and naming.

This should be easier to follow and easier to maintain.

r? @tmandry

Rust compiler MCP rust-lang/compiler-team#278
Relevant issue: #34701 - Implement support for LLVMs code coverage instrumentation

richkadel · 2020-07-25T05:44:46Z

@wesleywiser @tmandry

I have more to upload, but I'm going to do separate PRs for the recent changes I've made to add more counters based on MIR branching.

Found some problems with the coverage map encoding when testing with more than one counter per function. While debugging, I realized some better ways to structure the Rust implementation of the coverage mapping generator. I refactored somewhat, resulting in less code overall, expanded coverage of LLVM Coverage Map capabilities, and much closer alignment with LLVM data structures, APIs, and naming. This should be easier to follow and easier to maintain.

richkadel · 2020-07-25T15:16:48Z

r? @tmandry

richkadel · 2020-07-27T23:32:07Z

@tmandry As discussed, I just added a second commit covering the recent refactoring of MIR pass in instrument_coverage.rs.

This PR had no effect on the mir-opt or run-make-fulldeps test results.

I'll submit a "WIP"/draft PR separately, with the changes we reviewed, injecting counters before each BasicBlock. I have to regenerate the test expected results for that one.

richkadel · 2020-07-28T00:01:40Z

Interesting side note... The format check failed, even though I had run ./x.py fmt!

I had copied a snapshot of the instrument_coverage.rs source to my toplevel rust/ directory. Files not checked into git (like that file) are "ignored" by ./x.py fmt, which is good. But apparently this ignores all files with names matching the ignored file names. Not just the one at the path ./instrument_coverage.rs.

$ ./x.py fmt
Updating only changed submodules
Submodules updated in 0.06 seconds
    Finished dev [unoptimized + debuginfo] target(s) in 0.13s
skip untracked path instrument_coverage.rs during rustfmt invocations
Build completed successfully in 0:00:09

I had to delete ./instrument_coverage.rs and then run ./.x.py fmt.

wesleywiser

Left a few small comments

src/librustc_codegen_llvm/coverageinfo/mapgen.rs

src/librustc_codegen_llvm/coverageinfo/mod.rs

src/librustc_codegen_ssa/coverageinfo/map.rs

src/librustc_codegen_llvm/coverageinfo/mapgen.rs

src/librustc_codegen_llvm/coverageinfo/mod.rs

src/librustc_codegen_ssa/coverageinfo/map.rs

Lays a better foundation for injecting more counters in each function.

richkadel · 2020-07-29T00:53:48Z

I responded to all but one of the feedback comments (if a change was needed) in 20f55c1, but I pushed a separate commit b58afc0 with changes to use newtype_index! types in FunctionCoverage per Tyler's suggestion. I think the change seems OK, and probably has some type-checking advantages, but I wanted you guys to have the option of comparing the two.

Some were in librustc_codegen_llvm, but others are not tied to LLVM, so I put them in a new crate: librustc_codegen_ssa/coverageinfo/ffi.rs

src/librustc_mir/transform/instrument_coverage.rs

tmandry · 2020-07-29T19:01:42Z

I think this is ready to merge.
@bors r+

bors · 2020-07-29T19:01:44Z

📌 Commit 5b2e2b2 has been approved by tmandry

bors · 2020-07-29T20:35:59Z

⌛ Testing commit 5b2e2b2 with merge db0492a...

bors · 2020-07-29T22:24:38Z

☀️ Test successful - checks-actions, checks-azure
Approved by: tmandry
Pushing db0492a to master...

richkadel · 2020-07-30T04:13:19Z

@wesleywiser @tmandry FYI - In this version of Rust Coverage, I intentionally reintroduced the "assert_eq" that the start and end of a code region are from the same file. I know this breaks when attempting to compile larger crates with dependencies, but to ignore the problem simply produced bad results.

The good news is, I found a way to fix the problem, and I'm getting clean coverage and code regions across both local and external crates now, when I compile a crate with dependencies!

I'm going to push a small PR tomorrow with this fix.

I mentioned this to Tyler tonight as well: I know the fix works better and produces clean results. I'm not 100% sure it is producing complete results yet.

After I post the new PR so you can see what I'm doing, I'll explain the new solution, why I think it works, and what I'm not sure is covered. (I'm sure I can create some tests to confirm things, either way.)

wesleywiser · 2020-07-30T13:02:55Z

@richkadel By the way, thanks for splitting this work up into lots of PRs! I'm sure it would probably be easier for you to just iterate on the code and dump it over the fence but doing it this way makes reviewing it much more manageable.

When you have this a bit closer to working (which sounds like it may be very soon!), we might want to think about adding a lot more tests both to ensure things are working correctly and to make sure they keep working correctly in the future.

richkadel · 2020-07-30T15:37:30Z

@wesleywiser On the subject of tests, since testing the end product requires building and then running a binary, and then running llvm tools, plus the rust-demangler tool, I think run-make-fulldeps is the right place for building more tests.

Generating the expected results is manual and a bit awkward.

Do you think it would be reasonable for me to update the test framework to allow --bless to be used with run-make-fulldeps, causing it to simply set an environment variable (like, RUST_TEST_BLESS), and then Makefiles could optionally check that env setting, and optionally recreate any needed "expected" results?

I would use this if it existed.

wesleywiser · 2020-07-30T17:24:52Z

@richkadel That seems fine to me. I don't think I've ever touched those tests but I think in general, we'd like to see --bless or some equivalent be available everywhere.

Full integration tests would be great but I'd also like to see more mir-opt tests since they're going to be the most precise.

richkadel · 2020-07-30T17:55:41Z

Full integration tests would be great but I'd also like to see more mir-opt tests since they're going to be the most precise.

I completely agree. My first paragraph was poorly worded. I definitely plan to include a lot more mir-opt tests as well.

…r=tmandry Rust function-level coverage now works on external crates Follow-up to a known issue discussed (post-merge) in rust-lang#74733: Resolves a known issue in the coverage map where some regions had nonsensical source code locations. External crate functions are already included in their own coverage maps, per library, and don't need to also be added to the importing crate's coverage map. (In fact, their source start and end byte positions are not relevant to the importing crate's SourceMap.) The fix was to simply skip trying to add imported coverage info to the coverage map if the instrumented function is not "local". The injected counters are still relevant, however, and the LLVM `instrprof.increment` intrinsic call parameters will map those counters to the external crates' coverage maps, when generating runtime coverage data. Now Rust Coverage can cleanly instrument and analyze coverage on an entire crate and its dependencies. Example (instrumenting https://github.com/google/json5format): ```bash $ ./x.py build rust-demangler # make sure the demangler is built $ cd ~/json5format $ RUSTC=$HOME/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc \ RUSTFLAGS="-Zinstrument-coverage" \ cargo build --example formatjson5 $ LLVM_PROFILE_FILE="formatjson5.profraw" \ ./target/debug/examples/formatjson5 session_manager.cml $ ~/rust/build/x86_64-unknown-linux-gnu/llvm/bin/llvm-profdata merge \ -sparse formatjson5.profraw -o formatjson5.profdata $ ~/rust/build/x86_64-unknown-linux-gnu/llvm/bin/llvm-cov show --use-color \ --instr-profile=formatjson5.profdata target/debug/examples/formatjson5 \ --show-line-counts-or-regions \ --Xdemangler=$HOME/rust/build/x86_64-unknown-linux-gnu/stage0-tools-bin/rust-demangler \ --show-instantiations \ 2>&1 | less -R ``` (Scan forward for some of the non-zero coverage results, with `/^....[0-9]\| *[^ |0]`.) <img width="1071" alt="Screen Shot 2020-07-30 at 1 21 01 PM" src="https://user-images.githubusercontent.com/3827298/88970627-97e43000-d267-11ea-8e4d-fe40a091f756.png">

richkadel force-pushed the llvm-coverage-map-gen-5 branch from e4cf98d to a7053dc Compare July 25, 2020 05:54

richkadel force-pushed the llvm-coverage-map-gen-5 branch from a7053dc to 12ddd60 Compare July 25, 2020 14:40

rust-highfive assigned tmandry Jul 25, 2020

richkadel force-pushed the llvm-coverage-map-gen-5 branch from a5c2c04 to fa86fc4 Compare July 27, 2020 23:50

wesleywiser reviewed Jul 28, 2020

View reviewed changes

tmandry reviewed Jul 28, 2020

View reviewed changes

richkadel added 2 commits July 28, 2020 15:08

Refactor MIR coverage instrumentation

20f55c1

Lays a better foundation for injecting more counters in each function.

FunctionCoverage: improve type checking with newtype_index types

b58afc0

richkadel force-pushed the llvm-coverage-map-gen-5 branch from fa86fc4 to b58afc0 Compare July 29, 2020 00:48

Moved structs/enums with repr(C) to LLVM types into ffi.rs crates

5b2e2b2

Some were in librustc_codegen_llvm, but others are not tied to LLVM, so I put them in a new crate: librustc_codegen_ssa/coverageinfo/ffi.rs

richkadel force-pushed the llvm-coverage-map-gen-5 branch from 18518ae to 5b2e2b2 Compare July 29, 2020 15:22

tmandry approved these changes Jul 29, 2020

View reviewed changes

src/librustc_mir/transform/instrument_coverage.rs Show resolved Hide resolved

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 29, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 29, 2020

bors merged commit db0492a into rust-lang:master Jul 29, 2020

This was referenced Jul 29, 2020

Remove Spans from HIR #72015

Closed

Move rustllvm into compiler/rustc_llvm #74787

Merged

richkadel mentioned this pull request Jul 30, 2020

Rust function-level coverage now works on external crates #74959

Merged

jyn514 mentioned this pull request Mar 16, 2021

Remove (lots of) dead code #83185

Merged

6 tasks

matthiaskrgr mentioned this pull request Sep 7, 2023

ice: called Result::unwrap() on an Err value: Unknown(<SeStr<S, E> as std::ptr::Pointee>::Metadata) #115628

Closed

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed coverage map issues; better aligned with LLVM APIs #74733

Fixed coverage map issues; better aligned with LLVM APIs #74733

richkadel commented Jul 25, 2020

richkadel commented Jul 25, 2020

richkadel commented Jul 25, 2020

richkadel commented Jul 27, 2020

richkadel commented Jul 28, 2020 •

edited

Loading

wesleywiser left a comment

richkadel commented Jul 29, 2020

tmandry commented Jul 29, 2020

bors commented Jul 29, 2020

bors commented Jul 29, 2020

bors commented Jul 29, 2020

richkadel commented Jul 30, 2020

wesleywiser commented Jul 30, 2020

richkadel commented Jul 30, 2020

wesleywiser commented Jul 30, 2020

richkadel commented Jul 30, 2020

Fixed coverage map issues; better aligned with LLVM APIs #74733

Fixed coverage map issues; better aligned with LLVM APIs #74733

Conversation

richkadel commented Jul 25, 2020

richkadel commented Jul 25, 2020

richkadel commented Jul 25, 2020

richkadel commented Jul 27, 2020

richkadel commented Jul 28, 2020 • edited Loading

wesleywiser left a comment

Choose a reason for hiding this comment

richkadel commented Jul 29, 2020

tmandry commented Jul 29, 2020

bors commented Jul 29, 2020

bors commented Jul 29, 2020

bors commented Jul 29, 2020

richkadel commented Jul 30, 2020

wesleywiser commented Jul 30, 2020

richkadel commented Jul 30, 2020

wesleywiser commented Jul 30, 2020

richkadel commented Jul 30, 2020

richkadel commented Jul 28, 2020 •

edited

Loading