Skip to content
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

coverage: Make #[coverage(..)] apply recursively to nested functions #126721

Merged
merged 3 commits into from
Jun 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 0 additions & 17 deletions compiler/rustc_codegen_ssa/src/codegen_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,22 +124,6 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
.emit();
}
}
sym::coverage => {
let inner = attr.meta_item_list();
match inner.as_deref() {
Some([item]) if item.has_name(sym::off) => {
codegen_fn_attrs.flags |= CodegenFnAttrFlags::NO_COVERAGE;
}
Some([item]) if item.has_name(sym::on) => {
// Allow #[coverage(on)] for being explicit, maybe also in future to enable
// coverage on a smaller scope within an excluded larger scope.
}
Some(_) | None => {
tcx.dcx()
.span_delayed_bug(attr.span, "unexpected value of coverage attribute");
}
}
}
sym::rustc_std_internal_symbol => {
codegen_fn_attrs.flags |= CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL
}
Expand Down Expand Up @@ -584,7 +568,6 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
}

if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::NAKED) {
codegen_fn_attrs.flags |= CodegenFnAttrFlags::NO_COVERAGE;
codegen_fn_attrs.inline = InlineAttr::Never;
}

Expand Down
5 changes: 1 addition & 4 deletions compiler/rustc_middle/src/middle/codegen_fn_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,7 @@ bitflags::bitflags! {
/// #[cmse_nonsecure_entry]: with a TrustZone-M extension, declare a
/// function as an entry function from Non-Secure code.
const CMSE_NONSECURE_ENTRY = 1 << 13;
/// `#[coverage(off)]`: indicates that the function should be ignored by
/// the MIR `InstrumentCoverage` pass and not added to the coverage map
/// during codegen.
const NO_COVERAGE = 1 << 14;
// (Bit 14 was used for `#[coverage(off)]`, but is now unused.)
/// `#[used(linker)]`:
/// indicates that neither LLVM nor the linker will eliminate this function.
const USED_LINKER = 1 << 15;
Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,15 @@ rustc_queries! {
separate_provide_extern
}

/// Checks for the nearest `#[coverage(off)]` or `#[coverage(on)]` on
/// this def and any enclosing defs, up to the crate root.
///
/// Returns `false` if `#[coverage(off)]` was found, or `true` if
/// either `#[coverage(on)]` or no coverage attribute was found.
query coverage_attr_on(key: LocalDefId) -> bool {
desc { |tcx| "checking for `#[coverage(..)]` on `{}`", tcx.def_path_str(key) }
}

/// Summarizes coverage IDs inserted by the `InstrumentCoverage` MIR pass
/// (for compiler option `-Cinstrument-coverage`), after MIR optimizations
/// have had a chance to potentially remove some of them.
Expand Down
33 changes: 32 additions & 1 deletion compiler/rustc_mir_transform/src/coverage/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ use rustc_middle::query::TyCtxtAt;
use rustc_middle::ty::{self, TyCtxt};
use rustc_middle::util::Providers;
use rustc_span::def_id::LocalDefId;
use rustc_span::sym;

/// Registers query/hook implementations related to coverage.
pub(crate) fn provide(providers: &mut Providers) {
providers.hooks.is_eligible_for_coverage =
|TyCtxtAt { tcx, .. }, def_id| is_eligible_for_coverage(tcx, def_id);
providers.queries.coverage_attr_on = coverage_attr_on;
providers.queries.coverage_ids_info = coverage_ids_info;
}

Expand Down Expand Up @@ -38,14 +40,43 @@ fn is_eligible_for_coverage(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool {
return false;
}

if tcx.codegen_fn_attrs(def_id).flags.contains(CodegenFnAttrFlags::NO_COVERAGE) {
if tcx.codegen_fn_attrs(def_id).flags.contains(CodegenFnAttrFlags::NAKED) {
trace!("InstrumentCoverage skipped for {def_id:?} (`#[naked]`)");
return false;
}

if !tcx.coverage_attr_on(def_id) {
trace!("InstrumentCoverage skipped for {def_id:?} (`#[coverage(off)]`)");
return false;
}

true
}

/// Query implementation for `coverage_attr_on`.
fn coverage_attr_on(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool {
// Check for annotations directly on this def.
if let Some(attr) = tcx.get_attr(def_id, sym::coverage) {
match attr.meta_item_list().as_deref() {
Some([item]) if item.has_name(sym::off) => return false,
Some([item]) if item.has_name(sym::on) => return true,
Some(_) | None => {
// Other possibilities should have been rejected by `rustc_parse::validate_attr`.
tcx.dcx().span_bug(attr.span, "unexpected value of coverage attribute");
}
}
}

match tcx.opt_local_parent(def_id) {
// Check the parent def (and so on recursively) until we find an
// enclosing attribute or reach the crate root.
Some(parent) => tcx.coverage_attr_on(parent),
Comment on lines +71 to +73
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it fine for this query to just call itself recursively (via the query system) to walk up the def tree, or is there something special I have to do here?

Copy link
Contributor

@clarfonthey clarfonthey Jun 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably it'd make sense to do some sort of path compression/memoisation so that the query is only linear in the number of items, rather than quadratic. But not quite sure what exists to do that in the compiler.

(Each item performs a number of queries equivalent to their number of parents, rather than just one to check their immediate parent which is cached.)

Copy link
Contributor Author

@Zalathar Zalathar Jun 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that I'm calling tcx.coverage_attr_on (and not recursing on fn coverage_attr_on directly), my understanding is that the query system will memoize the result.

So individual functions will only recurse until they hit something (typically the enclosing mod/impl) for which there is a previously-memoized result.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea this is totally fine. We do this for a lot of queries (e.g. generics_of)

// We reached the crate root without seeing a coverage attribute, so
// allow coverage instrumentation by default.
None => true,
}
}

/// Query implementation for `coverage_ids_info`.
fn coverage_ids_info<'tcx>(
tcx: TyCtxt<'tcx>,
Expand Down
9 changes: 6 additions & 3 deletions compiler/rustc_passes/src/check_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -369,13 +369,16 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
}
}

/// Checks that `#[coverage(..)]` is applied to a function or closure.
/// Checks that `#[coverage(..)]` is applied to a function/closure/method,
/// or to an impl block or module.
fn check_coverage(&self, attr: &Attribute, span: Span, target: Target) -> bool {
match target {
// #[coverage(..)] on function is fine
Target::Fn
| Target::Closure
| Target::Method(MethodKind::Trait { body: true } | MethodKind::Inherent) => true,
| Target::Method(MethodKind::Trait { body: true } | MethodKind::Inherent)
| Target::Impl
| Target::Mod => true,

_ => {
self.dcx().emit_err(errors::CoverageNotFnOrClosure {
attr_span: attr.span,
Expand Down
24 changes: 24 additions & 0 deletions tests/coverage/attr/impl.cov-map
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
Function name: <impl::MyStruct>::off_on (unused)
Raw bytes (9): 0x[01, 01, 00, 01, 00, 0e, 05, 00, 13]
Number of files: 1
- file 0 => global file 1
Number of expressions: 0
Number of file 0 mappings: 1
- Code(Zero) at (prev + 14, 5) to (start + 0, 19)

Function name: <impl::MyStruct>::on_inherit (unused)
Raw bytes (9): 0x[01, 01, 00, 01, 00, 16, 05, 00, 17]
Number of files: 1
- file 0 => global file 1
Number of expressions: 0
Number of file 0 mappings: 1
- Code(Zero) at (prev + 22, 5) to (start + 0, 23)

Function name: <impl::MyStruct>::on_on (unused)
Raw bytes (9): 0x[01, 01, 00, 01, 00, 19, 05, 00, 12]
Number of files: 1
- file 0 => global file 1
Number of expressions: 0
Number of file 0 mappings: 1
- Code(Zero) at (prev + 25, 5) to (start + 0, 18)

42 changes: 42 additions & 0 deletions tests/coverage/attr/impl.coverage
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
LL| |#![feature(coverage_attribute)]
LL| |//@ edition: 2021
LL| |
LL| |// Checks that `#[coverage(..)]` can be applied to impl and impl-trait blocks,
LL| |// and is inherited by any enclosed functions.
LL| |
LL| |struct MyStruct;
LL| |
LL| |#[coverage(off)]
LL| |impl MyStruct {
LL| | fn off_inherit() {}
LL| |
LL| | #[coverage(on)]
LL| 0| fn off_on() {}
LL| |
LL| | #[coverage(off)]
LL| | fn off_off() {}
LL| |}
LL| |
LL| |#[coverage(on)]
LL| |impl MyStruct {
LL| 0| fn on_inherit() {}
LL| |
LL| | #[coverage(on)]
LL| 0| fn on_on() {}
LL| |
LL| | #[coverage(off)]
LL| | fn on_off() {}
LL| |}
LL| |
LL| |trait MyTrait {
LL| | fn method();
LL| |}
LL| |
LL| |#[coverage(off)]
LL| |impl MyTrait for MyStruct {
LL| | fn method() {}
LL| |}
LL| |
LL| |#[coverage(off)]
LL| |fn main() {}

41 changes: 41 additions & 0 deletions tests/coverage/attr/impl.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
#![feature(coverage_attribute)]
//@ edition: 2021

// Checks that `#[coverage(..)]` can be applied to impl and impl-trait blocks,
// and is inherited by any enclosed functions.

struct MyStruct;

#[coverage(off)]
impl MyStruct {
fn off_inherit() {}

#[coverage(on)]
fn off_on() {}

#[coverage(off)]
fn off_off() {}
}

#[coverage(on)]
impl MyStruct {
fn on_inherit() {}

#[coverage(on)]
fn on_on() {}

#[coverage(off)]
fn on_off() {}
}

trait MyTrait {
fn method();
}

#[coverage(off)]
impl MyTrait for MyStruct {
fn method() {}
}

#[coverage(off)]
fn main() {}
24 changes: 24 additions & 0 deletions tests/coverage/attr/module.cov-map
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
Function name: module::off::on (unused)
Raw bytes (9): 0x[01, 01, 00, 01, 00, 0c, 05, 00, 0f]
Number of files: 1
- file 0 => global file 1
Number of expressions: 0
Number of file 0 mappings: 1
- Code(Zero) at (prev + 12, 5) to (start + 0, 15)

Function name: module::on::inherit (unused)
Raw bytes (9): 0x[01, 01, 00, 01, 00, 14, 05, 00, 14]
Number of files: 1
- file 0 => global file 1
Number of expressions: 0
Number of file 0 mappings: 1
- Code(Zero) at (prev + 20, 5) to (start + 0, 20)

Function name: module::on::on (unused)
Raw bytes (9): 0x[01, 01, 00, 01, 00, 17, 05, 00, 0f]
Number of files: 1
- file 0 => global file 1
Number of expressions: 0
Number of file 0 mappings: 1
- Code(Zero) at (prev + 23, 5) to (start + 0, 15)

38 changes: 38 additions & 0 deletions tests/coverage/attr/module.coverage
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
LL| |#![feature(coverage_attribute)]
LL| |//@ edition: 2021
LL| |
LL| |// Checks that `#[coverage(..)]` can be applied to modules, and is inherited
LL| |// by any enclosed functions.
LL| |
LL| |#[coverage(off)]
LL| |mod off {
LL| | fn inherit() {}
LL| |
LL| | #[coverage(on)]
LL| 0| fn on() {}
LL| |
LL| | #[coverage(off)]
LL| | fn off() {}
LL| |}
LL| |
LL| |#[coverage(on)]
LL| |mod on {
LL| 0| fn inherit() {}
LL| |
LL| | #[coverage(on)]
LL| 0| fn on() {}
LL| |
LL| | #[coverage(off)]
LL| | fn off() {}
LL| |}
LL| |
LL| |#[coverage(off)]
LL| |mod nested_a {
LL| | mod nested_b {
LL| | fn inner() {}
LL| | }
LL| |}
LL| |
LL| |#[coverage(off)]
LL| |fn main() {}

37 changes: 37 additions & 0 deletions tests/coverage/attr/module.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#![feature(coverage_attribute)]
//@ edition: 2021

// Checks that `#[coverage(..)]` can be applied to modules, and is inherited
// by any enclosed functions.

#[coverage(off)]
mod off {
fn inherit() {}

#[coverage(on)]
fn on() {}

#[coverage(off)]
fn off() {}
}

#[coverage(on)]
mod on {
fn inherit() {}

#[coverage(on)]
fn on() {}

#[coverage(off)]
fn off() {}
}

#[coverage(off)]
mod nested_a {
mod nested_b {
fn inner() {}
}
}

#[coverage(off)]
fn main() {}
Loading
Loading