LLVM IR coverage encoding aligns closer to Clang's
I found some areas for improvement while attempting to debug the
SegFault issue when running rust programs compiled using MSVC, with
coverage instrumentation.
I discovered that LLVM's coverage writer was generating incomplete
function name variable names (that's not a typo: the name of the
variable that holds a function name).
The existing implementation used one-up numbers to distinguish
variables, and correcting the names did not fix the MSVC coverage bug,
but the fix in this PR makes the names and resulting LLVM IR easier to
follow and more consistent with Clang's implementation.
I also changed the way the `-Zinstrument-coverage` option is supported in
symbol_export.rs. The original implementation was incorrect, and the
corrected version matches the handling for `-Zprofile-generate`, as it
turns out.
(An argument could be made that maybe `-Zinstrument-coverage` should
automatically enable `-Cprofile-generate`. In fact, if
`-Cprofile-generate` is analagous to Clang's `-fprofile-generate`, as
some documentation implies, Clang always requires this flag for its
implementation of source-based code coverage. This would require a
little more validation, and if implemented, would probably require
updating some of the user-facing messages related to
`-Cprofile-generate` to not be so specific to the PGO use case.)
None of these changes fixed the MSVC coverage problems, but they should
still be welcome improvements.
Lastly, I added some additional FIXME comments in instrument_coverage.rs
describing issues I found with the generated LLVM IR that would be
resolved if the coverage instrumentation is injected with a `Statement`
instead of as a new `BasicBlock`. I describe seven advantages of this
change, but it requires some discussion before making a change like
this.
r? @tmandry
I found some areas for improvement while attempting to debug the
SegFault issue when running rust programs compiled using MSVC, with
coverage instrumentation.
I discovered that LLVM's coverage writer was generating incomplete
function name variable names (that's not a typo: the name of the
variable that holds a function name).
The existing implementation used one-up numbers to distinguish
variables, and correcting the names did not fix the MSVC coverage bug,
but the fix in this PR makes the names and resulting LLVM IR easier to
follow and more consistent with Clang's implementation.
I also changed the way the `-Zinstrument-coverage` option is supported
in symbol_export.rs. The original implementation was incorrect, and the
corrected version matches the handling for `-Zprofile-generate`, as it
turns out.
(An argument could be made that maybe `-Zinstrument-coverage` should
automatically enable `-Cprofile-generate`. In fact, if
`-Cprofile-generate` is analagous to Clang's `-fprofile-generate`, as
some documentation implies, Clang always requires this flag for its
implementation of source-based code coverage. This would require a
little more validation, and if implemented, would probably require
updating some of the user-facing messages related to
`-Cprofile-generate` to not be so specific to the PGO use case.)
None of these changes fixed the MSVC coverage problems, but they should
still be welcome improvements.
Lastly, I added some additional FIXME comments in instrument_coverage.rs
describing issues I found with the generated LLVM IR that would be
resolved if the coverage instrumentation is injected with a `Statement`
instead of as a new `BasicBlock`. I describe seven advantages of this
change, but it requires some discussion before making a change like
this.
The prior PR corrected for errors encountered when trying to generate
the coverage map on source code inlined from external crates (including
macros and generics) by avoiding adding external DefIds to the coverage
map.
This made it possible to generate a coverage report including external
crates, but the external crate coverage was incomplete (did not include
coverage for the DefIds that were eliminated.
The root issue was that the coverage map was converting Span locations
to source file and locations, using the SourceMap for the current crate,
and this would not work for spans from external crates (compliled with a
different SourceMap).
The solution was to convert the Spans to filename and location during
MIR generation instead, so precompiled external crates would already
have the correct source code locations embedded in their MIR, when
imported into another crate.
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.
added regions with counter expressions and counters.
Added codegen_llvm/coverageinfo mod for upcoming coverage map
Move coverage region collection to CodegenCx finalization
Moved from `query coverageinfo` (renamed from `query coverage_data`),
as discussed in the PR at:
https://github.com/rust-lang/rust/pull/73684#issuecomment-649882503
Address merge conflict in MIR instrument_coverage test
The MIR test output format changed for int types.
moved debug messages out of block.rs
This makes the block.rs calls to add coverage mapping data to the
CodegenCx much more concise and readable.
move coverage intrinsic handling into llvm impl
I realized that having half of the coverage intrinsic handling in
`rustc_codegen_ssa` and half in `rustc_codegen_llvm` meant that any
non-llvm backend would be bound to the same decisions about how the
coverage-related MIR terminators should be handled.
To fix this, I moved the non-codegen portion of coverage intrinsic
handling into its own trait, and implemented it in `rustc_codegen_llvm`
alongside `codegen_intrinsic_call`.
I also added the (required?) stubs for the new intrinsics to
`IntrepretCx::emulate_intrinsic()`, to ensure calls to this function do
not fail if called with these new but known intrinsics.
address PR Feedback on 28 June 2020 2:48pm PDT
