Completes support for coverage in external crates #75037
Conversation
rust-highfive
added
the
S-waiting-on-review
richkadel
force-pushed
the
llvm-coverage-map-gen-5.2
branch
from
da475bc to
37bf1c4
Compare
richkadel
force-pushed
the
llvm-coverage-map-gen-5.2
branch
from
37bf1c4 to
d55f927
Compare
|
@bors r=wesleywiser r+ rollup=always |
|
📌 Commit d55f927 has been approved by |
|
💡 This pull request was already approved, no need to approve it again.
|
bors
added
S-waiting-on-bors
|
📌 Commit d55f927 has been approved by |
…, r=wesleywiser Completes support for coverage in external crates Follow-up to rust-lang#74959 : 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. @wesleywiser FYI r? @tmandry
|
mir-opt fails in CI on instrument-coverage: https://github.com/rust-lang-ci/rust/runs/939814485#step:23:18408 The file references now in the MIR are absolute paths. The mir-opt tests replace the string value versions of absolute paths to directories with |
|
@bors r- |
bors
added
S-waiting-on-author
|
Looking for recommendations/preferences on how to resolve this. I'm not sure why the SourceMap is using absolute path references when compiling the sample code in the mir-opt test. If there's a "switch" that forces the test to somehow use relative paths instead, that might resolve it. If the MIR code requires the absolute paths, because that's what's expected in MIR encoding, then I don't think it makes sense to violate those assumptions in the librustc_* code just to get a test to pass. I could add an additional normalization to To do that, I would do a regex replace starting with It's a bit ugly since it's hardcoding a fix for this particular data structure, but I'm not sure if there's a better solution. Here's the data structure affected by the absolute path references in the SourceMap (not these are printed on one line, but for readability, I broke it into multiple lines with indent): |
|
I found a much better solution. There's a // compile-flags: -Zinstrument-coverage --remap-path-prefix={{cwd}}=/the/cwdThis should resolve the issue. I'll change in the updated test and test results in a few minutes. |
richkadel
force-pushed
the
llvm-coverage-map-gen-5.2
branch
from
d55f927 to
22161c3
Compare
| @@ -3,7 +3,7 @@ | |||
| // intrinsics, during codegen. | |||
|
|
|||
| // needs-profiler-support | |||
| // compile-flags: -Zinstrument-coverage | |||
| // compile-flags: -Zinstrument-coverage --remap-path-prefix={{cwd}}=/the/cwd | |||
There was a problem hiding this comment.
@wesleywiser @tmandry
See this change, and the resulting path changes in the mir-opt diffs (for example).
This ensures the byte array representations are also the same, no matter where the absolute build paths are.
|
One question that might come up is, will the encoded MIR be significantly larger, given each injected call to I'm more hopeful than I am sure that this will not be an issue, because the string literal is stored in an interned pub fn const_from_str(tcx: TyCtxt<'tcx>, val: &str, span: Span) -> Operand<'tcx> {
let tcx = tcx;
let allocation = Allocation::from_byte_aligned_bytes(val.as_bytes());
let allocation = tcx.intern_const_alloc(allocation);This leads me to believe that only one copy of the byte array representation of each file will be stored in the encoded MIR. Does that make sense to you as well? |
Yes, that's correct. |
|
@wesleywiser Great! The change I submitted passed in-PR. I'm fairly confident this resolves the CI issue. Can you re-submit to bors? Or if you are not as confident, we can do a |
|
Looks good! @bors r+ |
|
📌 Commit 22161c3 has been approved by |
bors
added
S-waiting-on-bors
|
Just as a data point for reference, below is a sample of file size changes (for As a reminder, the Most look "OK" I guess, though they will get bigger once we're injecting the additional calls that cover branches. A couple of notable outliers (though rare), perhaps attributable to one of the other compiler options enabled when coverage is enabled:
$ for file in $(find target-nocov -name '*.r[lm]*'); do \
covfile="${file/target-nocov/target-cov}"; \
nocov=$(stat -c%s $file); cov=$(stat -c%s $covfile); \
printf "%+7.2f%% = %5s / %5s: %s\n" \
"$( echo "scale=4; 100 * ($cov / $nocov) - 100" | bc )" \
"$(numfmt --to=si $nocov)" \
"$(numfmt --to=si $cov)" \
"${file/-nocov/}"; \
done |
|
⌛ Testing commit e0dc8de with merge 118a0185f14f7936c3edb16044cf2b903dce4963... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Random infra failure? Otherwise, I don't see any relevant error messages. |
tmandry
added
the
A-spurious
|
Looks spurious: @bors r=wesleywiser |
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit e0dc8de has been approved by |
bors
added
S-waiting-on-bors
|
@bors retry |
|
⌛ Testing commit e0dc8de with merge 92c20f72f8b3bc26a4b007afb030959f01643f68... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
Test flake I believe: 2020-08-05T00:38:09.4959609Z failures: |
|
Can we @bors retry again? Thanks |
|
@richkadel: 🔑 Insufficient privileges: not in try users |
|
@bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
Follow-up to #74959 :
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.
@wesleywiser FYI
r? @tmandry