-
Notifications
You must be signed in to change notification settings - Fork 13.6k
coverage: Enlarge empty spans during MIR instrumentation, not codegen #144298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This allows us to assume that coverage spans will only be discarded during codegen in very unusual situations.
rustbot has assigned @compiler-errors. Use |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
coverage: Enlarge empty spans during MIR instrumentation, not codegen
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (67f76fa): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (primary 2.2%, secondary 3.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 463.984s -> 466.318s (0.50%) |
@bors r+ rollup |
coverage: Enlarge empty spans during MIR instrumentation, not codegen This re-lands the part of rust-lang#140847 that was (hopefully) not responsible for the coverage-instrumentation regressions that caused that PR to be reverted. --- Enlarging empty spans was historically performed during MIR instrumentation, but had to be moved to codegen as part of larger changes in rust-lang#134497, leading to the status quo. But now there should be no reason not to move that step back to its more logical home in instrumentaion.
coverage: Enlarge empty spans during MIR instrumentation, not codegen This re-lands the part of rust-lang#140847 that was (hopefully) not responsible for the coverage-instrumentation regressions that caused that PR to be reverted. --- Enlarging empty spans was historically performed during MIR instrumentation, but had to be moved to codegen as part of larger changes in rust-lang#134497, leading to the status quo. But now there should be no reason not to move that step back to its more logical home in instrumentaion.
coverage: Enlarge empty spans during MIR instrumentation, not codegen This re-lands the part of rust-lang#140847 that was (hopefully) not responsible for the coverage-instrumentation regressions that caused that PR to be reverted. --- Enlarging empty spans was historically performed during MIR instrumentation, but had to be moved to codegen as part of larger changes in rust-lang#134497, leading to the status quo. But now there should be no reason not to move that step back to its more logical home in instrumentaion.
coverage: Enlarge empty spans during MIR instrumentation, not codegen This re-lands the part of rust-lang#140847 that was (hopefully) not responsible for the coverage-instrumentation regressions that caused that PR to be reverted. --- Enlarging empty spans was historically performed during MIR instrumentation, but had to be moved to codegen as part of larger changes in rust-lang#134497, leading to the status quo. But now there should be no reason not to move that step back to its more logical home in instrumentaion.
coverage: Enlarge empty spans during MIR instrumentation, not codegen This re-lands the part of rust-lang#140847 that was (hopefully) not responsible for the coverage-instrumentation regressions that caused that PR to be reverted. --- Enlarging empty spans was historically performed during MIR instrumentation, but had to be moved to codegen as part of larger changes in rust-lang#134497, leading to the status quo. But now there should be no reason not to move that step back to its more logical home in instrumentaion.
Rollup of 10 pull requests Successful merges: - #144173 (Remove tidy checks for `tests/ui/issues/`) - #144234 (Fix broken TLS destructors on 32-bit win7) - #144239 (Clean `rustc/parse/src/lexer` to improve maintainability) - #144247 (coretests/num: use ldexp instead of hard-coding a power of 2) - #144256 (Don't ICE on non-TypeId metadata within TypeId) - #144290 (update tests/ui/SUMMARY.md) - #144292 (mbe: Use concrete type for `get_unused_rule`) - #144298 (coverage: Enlarge empty spans during MIR instrumentation, not codegen) - #144311 (Add powerpc64le-unknown-linux-musl to CI rustc targets) - #144315 (bootstrap: add package.json and package-lock.json to dist tarball) r? `@ghost` `@rustbot` modify labels: rollup
coverage: Enlarge empty spans during MIR instrumentation, not codegen This re-lands the part of rust-lang#140847 that was (hopefully) not responsible for the coverage-instrumentation regressions that caused that PR to be reverted. --- Enlarging empty spans was historically performed during MIR instrumentation, but had to be moved to codegen as part of larger changes in rust-lang#134497, leading to the status quo. But now there should be no reason not to move that step back to its more logical home in instrumentaion.
Rollup of 10 pull requests Successful merges: - #144173 (Remove tidy checks for `tests/ui/issues/`) - #144234 (Fix broken TLS destructors on 32-bit win7) - #144239 (Clean `rustc/parse/src/lexer` to improve maintainability) - #144247 (coretests/num: use ldexp instead of hard-coding a power of 2) - #144256 (Don't ICE on non-TypeId metadata within TypeId) - #144290 (update tests/ui/SUMMARY.md) - #144292 (mbe: Use concrete type for `get_unused_rule`) - #144298 (coverage: Enlarge empty spans during MIR instrumentation, not codegen) - #144311 (Add powerpc64le-unknown-linux-musl to CI rustc targets) - #144315 (bootstrap: add package.json and package-lock.json to dist tarball) r? `@ghost` `@rustbot` modify labels: rollup try-job: x86_64-gnu-aux
Rollup of 9 pull requests Successful merges: - #144173 (Remove tidy checks for `tests/ui/issues/`) - #144234 (Fix broken TLS destructors on 32-bit win7) - #144239 (Clean `rustc/parse/src/lexer` to improve maintainability) - #144256 (Don't ICE on non-TypeId metadata within TypeId) - #144290 (update tests/ui/SUMMARY.md) - #144292 (mbe: Use concrete type for `get_unused_rule`) - #144298 (coverage: Enlarge empty spans during MIR instrumentation, not codegen) - #144311 (Add powerpc64le-unknown-linux-musl to CI rustc targets) - #144315 (bootstrap: add package.json and package-lock.json to dist tarball) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #144298 - Zalathar:empty-span, r=wesleywiser coverage: Enlarge empty spans during MIR instrumentation, not codegen This re-lands the part of #140847 that was (hopefully) not responsible for the coverage-instrumentation regressions that caused that PR to be reverted. --- Enlarging empty spans was historically performed during MIR instrumentation, but had to be moved to codegen as part of larger changes in #134497, leading to the status quo. But now there should be no reason not to move that step back to its more logical home in instrumentaion.
// For all empty spans, either enlarge them to be non-empty, or discard them. | ||
let source_map = tcx.sess.source_map(); | ||
covspans.retain_mut(|covspan| { | ||
let Some(span) = ensure_non_empty_span(source_map, covspan.span) else { return false }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some local testing, it is the return false
here that causes the issues of
malformed instrumentation profile data: function name is empty
So what spans are kept/discarded is changed by this PR. Perhaps due to some of the modifications to the spans that happen right above here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spans that are modified or discarded would have met the same fate later anyway, during codegen. This PR just makes that happen earlier.
So there's something mysterious going on, where discarding these spans early somehow causes us to emit malformed or incomplete instrprof metadata.
…thar Revert "coverage: Enlarge empty spans during MIR instrumentation, not codegen" Surprisingly, rust-lang#144298 alone (extracted from rust-lang#140847) was enough to re-trigger the failures observed in rust-lang#141577 (comment). --- This reverts commit f877aa7. --- r? ghost
…thar Revert "coverage: Enlarge empty spans during MIR instrumentation, not codegen" Surprisingly, rust-lang#144298 alone (extracted from rust-lang#140847) was enough to re-trigger the failures observed in rust-lang#141577 (comment). --- This reverts commit f877aa7. --- r? ghost
Rollup merge of #144480 - Zalathar:revert-empty-span, r=Zalathar Revert "coverage: Enlarge empty spans during MIR instrumentation, not codegen" Surprisingly, #144298 alone (extracted from #140847) was enough to re-trigger the failures observed in #141577 (comment). --- This reverts commit f877aa7. --- r? ghost
coverage: Infer `instances_used` from `pgo_func_name_var_map` In obscure circumstances involving macro-expanded spans, we would sometimes emit a covfun record for a function with no physical coverage counters, and therefore no corresponding entry in the “PGO names” section of the binary. The absence of that name entry causes `llvm-cov` to fail with the cryptic error message: ```text malformed instrumentation profile data: function name is empty ``` We can eliminate this mismatch by removing `instances_used` entirely, and instead inferring its contents from the keys of `pgo_func_name_var_map`. This makes it impossible for a "used" function to lack a PGO name entry. --- This is an attempt to eliminate the cause of rust-lang#141577 when re-landing changes like rust-lang#144298 in the future. I haven't been able to reproduce the underlying issue in an in-tree test, because the only known repro involves a non-trivial derive proc-macro that relies on `syn` and `proc-macro2`. But I have manually verified in a separate branch that this change would have prevented the reoccurrence of rust-lang#141577 (comment).
Rollup merge of #144530 - Zalathar:instances-used, r=lqd coverage: Infer `instances_used` from `pgo_func_name_var_map` In obscure circumstances involving macro-expanded spans, we would sometimes emit a covfun record for a function with no physical coverage counters, and therefore no corresponding entry in the “PGO names” section of the binary. The absence of that name entry causes `llvm-cov` to fail with the cryptic error message: ```text malformed instrumentation profile data: function name is empty ``` We can eliminate this mismatch by removing `instances_used` entirely, and instead inferring its contents from the keys of `pgo_func_name_var_map`. This makes it impossible for a "used" function to lack a PGO name entry. --- This is an attempt to eliminate the cause of #141577 when re-landing changes like #144298 in the future. I haven't been able to reproduce the underlying issue in an in-tree test, because the only known repro involves a non-trivial derive proc-macro that relies on `syn` and `proc-macro2`. But I have manually verified in a separate branch that this change would have prevented the reoccurrence of #141577 (comment).
This re-lands the part of #140847 that was (hopefully) not responsible for the coverage-instrumentation regressions that caused that PR to be reverted.
Enlarging empty spans was historically performed during MIR instrumentation, but had to be moved to codegen as part of larger changes in #134497, leading to the status quo. But now there should be no reason not to move that step back to its more logical home in instrumentaion.