Do not generate coverage records for compile-time-only functions - #161808
Do not generate coverage records for compile-time-only functions#161808fs-rachel wants to merge 3 commits into
Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred in coverage instrumentation. cc @Zalathar |
rustbot
added
S-waiting-on-review
|
r? @mu001999 rustbot has assigned @mu001999. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@rustbot reroll |
|
Could you add a regression test Footnotes
|
|
I'm not familiar with intrinsics, but if something is not supposed to ever exist at runtime then it makes sense to not consider it “unused”. |
| if let DefKind::Fn | DefKind::AssocFn = tcx.def_kind(def_id) { | ||
| if tcx.constness(def_id) == (Constness::Const { always: true }) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of doing two query-cache lookups for tcx.def_kind(def_id) (above and here), we should call it once and stash the result in a local variable.
There was a problem hiding this comment.
Style-wise, I think this condition would be a little clearer as:
if matches!(def_kind, DefKind::Fn | DefKind::AssocFn)
&& matches!(tcx.constness(def_id), Constness::Const { always: true })
{
return false;
}There was a problem hiding this comment.
That does look a lot nicer, I've incorporated that in the new version of the PR.
Though ideally it should match the check in compute_symbol_name, which IMHO looks a lot less intuitive with that change
Current:
if let DefKind::Fn | DefKind::AssocFn = def_kind {
debug_assert!(tcx.constness(instance.def_id()) != hir::Constness::Const { always: true });
}Applying that change and running ./x fmt:
if matches!(def_kind, DefKind::Fn | DefKind::AssocFn) {
debug_assert!(!matches!(
tcx.constness(instance.def_id()),
hir::Constness::Const { always: true }
));
}What are your thoughts if I split the difference like this?
if matches!(def_kind, DefKind::Fn | DefKind::AssocFn) {
debug_assert!(tcx.constness(instance.def_id()) != hir::Constness::Const { always: true });
}
b455046 to
90fc450
Compare
Done! I was worried it would be difficult because actually generating coverage for |
|
Also I rewrote the comment + commit message to hopefully be clearer about how this all fits together |
fs-rachel
changed the title
This comment has been minimized.
This comment has been minimized.
|
Wow, that clippy suggestion is awful. I guess we can ignore the lint at the function level. |
This test intentionally fails on this commit, and will be fixed by the following commit
These functions (which are generally, though not exclusively, intrinsics) should always resolve to a definite value at compile time, and so should never be called at runtime. We ensure that these functions never reach codegen using a check in `compute_symbol_name` in `compiler/rustc_symbol_mangling/src/lib.rs`. However, when generating coverage, `prepare_covfun_records_for_unused_functions` will detect them as unused functions (since every call has been replaced by a constant by this point) and try to generate dummy coverage records. This will trip the above check; to avoid that, skip generating coverage for such functions. This is necessary to correctly generate coverage data for `core`.
6a26779 to
23019ce
Compare
|
Realized the test only needs |
…ouwer Change `is_eligible_for_coverage` from a hook to a query - Inspired by seeing rust-lang#161808 add more eligibility conditions --- This check is called from a few different places when coverage is enabled, so we should probably let the query system take care of memoizing results and tracking dependencies. (It was made a hook in rust-lang#122322, but I didn't have strong reasons for making it a hook and not a query, other than it being relatively small and simple.) There should be no user-visible change to compiler behaviour.
…ouwer Change `is_eligible_for_coverage` from a hook to a query - Inspired by seeing rust-lang#161808 add more eligibility conditions --- This check is called from a few different places when coverage is enabled, so we should probably let the query system take care of memoizing results and tracking dependencies. (It was made a hook in rust-lang#122322, but I didn't have strong reasons for making it a hook and not a query, other than it being relatively small and simple.) There should be no user-visible change to compiler behaviour.
…ouwer Change `is_eligible_for_coverage` from a hook to a query - Inspired by seeing rust-lang#161808 add more eligibility conditions --- This check is called from a few different places when coverage is enabled, so we should probably let the query system take care of memoizing results and tracking dependencies. (It was made a hook in rust-lang#122322, but I didn't have strong reasons for making it a hook and not a query, other than it being relatively small and simple.) There should be no user-visible change to compiler behaviour.
7 participants
These functions (which are generally, though not exclusively, intrinsics) should always
resolve to a definite value at compile time, and so should never be called at runtime.
We ensure that these functions never reach codegen using a check in
compute_symbol_namein
compiler/rustc_symbol_mangling/src/lib.rs.However, when generating coverage,
prepare_covfun_records_for_unused_functionswilldetect them as unused functions (since every call has been replaced by a constant by
this point) and try to generate dummy coverage records. This will trip the above check;
to avoid that, skip generating coverage for such functions.
This is necessary to correctly generate coverage data for
core.